Closed Bug 1329355 Opened 3 years ago Closed 3 years ago

Remove pretty-* automation steps if they're no longer used

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox54 fixed)

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: chmanchester, Assigned: mshal)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

`MOZ_AUTOMATION_PRETTY` doesn't appear to be in use anywhere. If not, we should remove the associated automation steps.
It looks like we stopped using MOZ_AUTOMATION_PRETTY in bug 1121000 when non-unified builds were removed. IIRC we were using it there as a way to check that all the pretty targets worked, so things wouldn't break for release builds where the pretty targets were actually used. So at the very least we can get rid of the automation/pretty-foo stuff in moz-automation.mk

What I can't quite figure out is if the top-level 'make pretty-foo' targets or 'make foo MOZ_PKG_PRETTYNAMES=1' are still used anywhere. From buildbot-configs it looks like two windows build types set test_pretty_names=True, though I can't find any logs that seem to actually contain these steps. The ReleaseBuildFactory also sets MOZ_PKG_PRETTYNAMES=1, though I think now that we have release promotion, that factory is no longer used - :rail, can you confirm whether we still need these targets and/or MOZ_PKG_PRETTYNAMES=1 support? If not, we can probably delete a lot more things than just the moz-automation.mk rules.
Flags: needinfo?(rail)
Actually, the top-level 'make pretty-package-names' targets were specifically added for the moz-automation stuff, so that can be removed. But I'm still curious if the MOZ_PKG_PRETTYNAMES logic is still used in buildbot.
I think we stopped using pretty names in release automation, but we may still use them for nightly builds on m-a and m-c. Also, we run pretty name tests, see https://dxr.mozilla.org/build-central/source/buildbot-configs/mozilla/config.py#2617-2622
Flags: needinfo?(rail)
Also, I'm sure that Thunderbird still uses them. We should let them know if we decide to drop support.
I chatted with rail in IRC - the pretty name tests steps are in factories that we no longer use. I couldn't find any instance of them in the logs where they appeared to be enabled, so I think we can remove both the moz-automation steps as well as the MOZ_PKG_PRETTYNAMES=1 logic.
CC'd some tb folks since it looks like tb still uses MOZ_PKG_PRETTYNAMES=1. If we remove it from m-c, how will that impact you? Since it's just removing some dead (to firefox) code, we can wait to land it.
Thunderbird uses MOZ_PKG_PRETTYNAMES=1 because Firefox used to, so we should do anything short of migrating away from buildbot to follow (although that is surely also an option, for another day). Can you let us know what you changed in Firefox to continue building with MOZ_PKG_PRETTYNAMES removed?
(In reply to Philipp Kewisch [:Fallen] from comment #9)
> Thunderbird uses MOZ_PKG_PRETTYNAMES=1 because Firefox used to, so we should
> do anything short of migrating away from buildbot to follow (although that
> is surely also an option, for another day). Can you let us know what you
> changed in Firefox to continue building with MOZ_PKG_PRETTYNAMES removed?

I believe it is less a change in Firefox and more a change in our release pipeline. Previously we used to do:

1) Build the tree with non-pretty package names
2) Test the package from 1)
3) Build the tree again with MOZ_PKG_PRETTYNAMES=1
4) Release the stuff in 3)

But with Release Promotion we now do something like:

1) Build the tree with non-pretty package names
2) Test the package from 1)
3) Promote the packages built in 1 to "released" versions (which maybe involves renaming them to match the pretty style?)

In the new pipeline, the MOZ_PKG_PRETTYNAMES logic in the build system is unused, so we are looking to remove it.

rail, did I sum things up approximately correctly? Anything I left out?
Flags: needinfo?(rail)
Comment on attachment 8826744 [details]
Bug 1329355 - Remove MOZ_PKG_PRETTYNAMES;

https://reviewboard.mozilla.org/r/104620/#review106056

fwiw, I am pretty sure that Thunderbird and SeaMonkey both use PRETTY NAMES for release packaging. And removing this stuff from toolkit could break the automation for those.

"we" still have an obligation to keep Thunderbird releases working...

I did not code-dive to verify this memory though.
(In reply to Michael Shal [:mshal] from comment #10)
> rail, did I sum things up approximately correctly? Anything I left out?

Sounds sane to me.

Just as an additional point, current release automation uses in-tree templates to rename regular (non pretty) file names to their "pretty" equivalents, see https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/configs/beetmover/en_us.yml.tmpl#42
Flags: needinfo?(rail)
Comment on attachment 8826743 [details]
Bug 1329355 - Remove MOZ_AUTOMATION_PRETTY*;

https://reviewboard.mozilla.org/r/104618/#review110696

Whee!
Attachment #8826743 - Flags: review?(ted) → review+
Comment on attachment 8826744 [details]
Bug 1329355 - Remove MOZ_PKG_PRETTYNAMES;

https://reviewboard.mozilla.org/r/104620/#review110700

I love patches that are nothing but code removal. :)

::: toolkit/mozapps/installer/package-name.mk
(Diff revision 1)
>  
>  # assemble package names, see convention at
>  # http://developer.mozilla.org/index.php?title=En/Package_Filename_Convention
> -# for (at least Firefox) releases we use a different format with directories,
> -# e.g. win32/de/Firefox Setup 3.0.1.exe
> -# the latter format is triggered with MOZ_PKG_PRETTYNAMES=1

You might want to mention that release packages get named differently, but that happens in the release pipeline, not the build system nowadays.
Attachment #8826744 - Flags: review?(ted) → review+
Fallen, will this cause you trouble if we land now? Or, when would a good time be to do so?
Flags: needinfo?(philipp)
Given I don't know what part of or if our builds will actually break, I think it is best for you to land this now and we'll handle the fallout during our next build. The only reason I'd hold off is if we have a build coming up in which case I'd ask you to land this shortly after.

I'm ni'ing Wayne who has a better overview on our schedule.
Flags: needinfo?(philipp) → needinfo?(vseerror)
I'd say run with it assuming we can fix the fallout = manpower :)
I think the next earliest beta will be Wed/Thurs
Flags: needinfo?(vseerror)
(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #19)
> I'd say run with it assuming we can fix the fallout = manpower :)
> I think the next earliest beta will be Wed/Thurs

change of plans - today I expect to build 45.7.1.

Is there an urgency to landing this patch for Firefox purposes?  Or can it wait to a time of our choosing?
(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #20)
> change of plans - today I expect to build 45.7.1.
> 
> Is there an urgency to landing this patch for Firefox purposes?  Or can it
> wait to a time of our choosing?

There's no urgency for this bug. Please just ping me or ni? me on this bug when it would cause the least disruption for you. Thanks!
Assignee: nobody → mshal
Just a reminder to let me know when you are ready.
Flags: needinfo?(vseerror)
Our beta built and is shipping today. If you don't hear from me by Monday noon EST that we need to fix something, please assume you can proceed.
Flags: needinfo?(vseerror)
https://hg.mozilla.org/mozilla-central/rev/68d491c00355
https://hg.mozilla.org/mozilla-central/rev/318f1bcd336e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1360700
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.