Closed Bug 1295375 Opened 8 years ago Closed 8 years ago

Drop support for universal Mac builds in Firefox 53

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(relnote-firefox 53+, firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
relnote-firefox --- 53+
firefox53 --- fixed

People

(Reporter: gps, Assigned: mshal)

References

Details

Attachments

(2 files)

Bug 1295375 - Drop universal Mac builds;
58 bytes, text/x-review-board-request
coop
: review+
gps
: review+
Details
fix-tests
5.20 KB, patch
jgraham
: feedback+
Details | Diff | Splinter Review 
Per bug 1183613 comment 16, we can drop universal mac builds in Firefox 53.

mozilla-central becomes Firefox 53 on November 7.
The question is: how far do we want to go with dropping universal mac builds. There are several level:
- Stop building them
- Remove the corresponding mozconfigs
- Remove the corresponding packaging code

The first only has an impact on Firefox.
The second might have an impact on third-parties relying on those mozconfigs to build their own universal builds (I'm thinking comm-central and other third-parties like bluegriffon)
The third would remove any support for third-parties to build their own universal builds.
Daniel, I remember we talked about universal builds some time ago, I don't remember if you'd be bothered if they were dropped entirely.
Flags: needinfo?(daniel)
Q: do Gecko/Firefox developers have to spend time making OS X code work on 32-bit Macs (excluding general 32-bit compatibility for other platforms like Windows and Linux)? If so, then I'm inclined to say we should drop all support for 32-bit Mac compatibility so developers don't need to worry about it.
I think that we should first just switch the existing universal builds to build single-architecture x86-64 opt builds, leaving all the build machinery in place for a full release cycle. Assuming that nothing goes awry, in the next release cycle we should start removing the support code (including mozconfigs, etc).
bsmedberg: can we proceed with this?
Flags: needinfo?(benjamin)
I don't know why but I never received the earlier needinfo...

I have no problem with step 1, of course, and I think I am ok with step 2. BlueGriffon still has OS X 32bits users but they're vanishing at rather fast pace. I can live with that negligible loss.

Mike, thanks a lot for asking.
Flags: needinfo?(daniel)
Yes gogogo!
Flags: needinfo?(benjamin)
I assume comment #7 means we can turn off universal builds in automation. So let's proceed with that.

bsmedberg: can you please clarify how far we should go with dropping in-tree *support* for universal builds (per comment #1)?
Flags: needinfo?(benjamin)
rail: do any balrog changes need to be made before we turn off universal os x builds? If so, could you please get bugs on file and have them block this bug?
Flags: needinfo?(rail)
302 jlund|releaseduty
Flags: needinfo?(rail) → needinfo?(jlund)
I believe we should feel free to remove build support for universal builds, if doing so would make your life easier. I'm sure that soon after we stop building UB in automation, things will start to break.

I think we should attempt to keep Mac x86 building, as a tier-3 config maintained by the community, if somebody steps forward to periodically build/maintain it.
Flags: needinfo?(benjamin)
So, funny thing I just noticed in bug 1321065: browser/config/mozconfigs/macosx64/nightly and browser/config/mozconfigs/macosx-universal/nightly are different. Whoever picks this bug should look into this.
(In reply to Mike Hommey [:glandium] from comment #12)
> So, funny thing I just noticed in bug 1321065:
> browser/config/mozconfigs/macosx64/nightly and
> browser/config/mozconfigs/macosx-universal/nightly are different. Whoever
> picks this bug should look into this.

I would guess it's just "someone forgot to update the other mozconfig that wasn't actually used for anything currently".
(In reply to Benjamin Smedberg [:bsmedberg] from comment #11)
> I think we should attempt to keep Mac x86 building, as a tier-3 config
> maintained by the community, if somebody steps forward to periodically
> build/maintain it.

Only if it's the cross-compiled variant. We're not keeping old Mac build hardware around for a tier 3 project.
(In reply to Gregory Szorc [:gps] from comment #9)
> rail: do any balrog changes need to be made before we turn off universal os
> x builds? If so, could you please get bugs on file and have them block this
> bug?

We haven't tested updating a nightly user from universal -> 64-bit opt. We should do that.
(In reply to Chris Cooper [:coop] from comment #15) 
> We haven't tested updating a nightly user from universal -> 64-bit opt. We
> should do that.

This is bug 1310849.
Depends on: 1310849
I have some of this ready to go - I just need to sort out the mozconfigs (in addition to the nightly differences, we need beta & release configs).
Assignee: nobody → mshal
(In reply to Chris Cooper [:coop] from comment #14)
> Only if it's the cross-compiled variant. We're not keeping old Mac build
> hardware around for a tier 3 project.

FWIW, Tier 3 has always meant "we don't run CI, but we'll take patches to keep it working".
If x86 Mac is tier 3 and the only thing we want to support is that x86 Mac users can still run Firefox, in my opinion as build module owner, that means universal builds no longer serve a useful function and all code and functionality related to universal builds can be removed.

If universal build support were easy to keep around, I'd say "why not." But the reality is the code for supporting universal builds is pretty messy and it gets in the way of various refactorings. It's in the best interest of build system maintainers for that code and associated complexity to disappear.
Comment on attachment 8815878 [details]
Bug 1295375 - Drop universal Mac builds;

https://reviewboard.mozilla.org/r/96662/#review96918

This is mostly good (and exciting!). Just a VCS nit and a question about the nightly config file, which contained some non-obvious changes.

::: browser/config/mozconfigs/macosx64/beta:1
(Diff revision 1)
> +MOZ_AUTOMATION_SDK=${MOZ_AUTOMATION_SDK-1}

Please `hg copy` these files from the macosx-universal versions so VCS history is preserved.

::: browser/config/mozconfigs/macosx64/nightly:1
(Diff revision 1)
> -. $topsrcdir/build/macosx/mozconfig.common
> +. "$topsrcdir/browser/config/mozconfigs/macosx64/common-opt"

I'm curious why this change was made. mozconfig.common handles cross vs native builds. This change seems unrelated to disabling universal builds?
Attachment #8815878 - Flags: review?(gps) → review-
Comment on attachment 8815878 [details]
Bug 1295375 - Drop universal Mac builds;

https://reviewboard.mozilla.org/r/96662/#review97258

::: browser/config/mozconfigs/macosx64/nightly:1
(Diff revision 1)
> -. $topsrcdir/build/macosx/mozconfig.common
> +. "$topsrcdir/browser/config/mozconfigs/macosx64/common-opt"

Sorry, I should have explained the changes to the nightly config. In macosx-universal, the common-opt file shared some config among the nightly/beta/release mozconfigs. In macosx64, we only had nightly, so some of the things that were in common-opt were just added directly to the nightly config. For example, the universal common-opt file specifies MOZ_TELEMETRY_REPORTING=1, but in macosx64 this was put into the nightly config.

But now we have beta & release mozconfigs in macosx64, so it makes sense to introduce the common-opt file there. So the nightly mozconfig changed since I removed the lines that are duplicated with common-opt. Including common-opt also includes build/macosx/mozconfig.common, so the cross-build logic is still intact.

It should be a bit more clear with the 'hg copy' for beta & release, and if you diff the new nightly config with the macosx-universal/nightly config, you'll see they are identical save for the common-opt include.
Comment on attachment 8815878 [details]
Bug 1295375 - Drop universal Mac builds;

https://reviewboard.mozilla.org/r/96662/#review97548

Good riddance.
Attachment #8815878 - Flags: review?(gps) → review+
Hmm, it seems the Wr and W(1) test suites (as well as the e10s counterparts) are failing. ted: do any of these look obvious to you? I'm not sure what I'm missing:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce0d646eb244aca032fbff8e8e0edaab1548b55b
Flags: needinfo?(ted)
Wr looks like a TEST-UNEXPECTED-PASS, which seems like a win in my book ;-) You probably just need to update testing/web-platform/meta/mathml/relations/css-styling/color-1.html.ini because the hardcoding of specific windows and mac versions in there is ugly-ass-terrible

http://searchfox.org/mozilla-central/source/testing/web-platform/meta/encrypted-media/clearkey-mp4-playback-temporary-encrypted-clear.html.ini#1 seems to be in a similarly-terrible state for W1
Flags: needinfo?(ted)
Comment on attachment 8815878 [details]
Bug 1295375 - Drop universal Mac builds;

https://reviewboard.mozilla.org/r/96662/#review97736

::: testing/mozharness/configs/merge_day/aurora_to_beta.py:39
(Diff revision 2)
>           "ac_add_options --enable-official-branding")
>          for f in ["browser/config/mozconfigs/linux32/l10n-mozconfig",
>                    "browser/config/mozconfigs/linux64/l10n-mozconfig",
>                    "browser/config/mozconfigs/win32/l10n-mozconfig",
>                    "browser/config/mozconfigs/win64/l10n-mozconfig",
> -                  "browser/config/mozconfigs/macosx-universal/l10n-mozconfig",
> +                  "browser/config/mozconfigs/macosx64/l10n-mozconfig",

This file is in the list twice.
Attachment #8815878 - Flags: review?(coop) → review+
Comment on attachment 8815878 [details]
Bug 1295375 - Drop universal Mac builds;

https://reviewboard.mozilla.org/r/96662/#review97736

> This file is in the list twice.

Good catch - fixed.
I also fixed the mozconfig whitelist, since the compare-mozconfigs check now fail properly from bug 1320761.
Attached patch fix-tests — — Splinter Review 
:jgraham, this seems to be the minimum needed to fix these tests for OSX 64-bit only builds (see also #c25, #c26). Is this the right way to fix them? It's not obvious to me why there are several cases spelled out that appear like they could be collapsed, such as an 'if e10s' and an 'if not e10s' line that have the same result. Are the lines auto-generated from somewhere?

Also by grepping through *.ini, I can see a number of other tests that still reference '(processor == "x86") and (bits == "32")', but I don't know why they don't currently fail for non-universal builds. Do they all need to be updated as well?

I think the reason these changes are required is that the opt builds will now be x86_64 and 64-bits similar to the debug builds, whereas the universal opt builds seem to identify themselves as x86 and 32-bits.
Attachment #8817214 - Flags: feedback?(james)
Comment on attachment 8817214 [details] [diff] [review]
fix-tests

Review of attachment 8817214 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like a reasonable change if the opt builds are now identifying as x86_64 rather than x86, but like you I'm pretty surprised that there aren't more cases that need to be updated.

The reason the metadata format is so awful is indeed that the files are autogenerated when we update the web-platform-tests from upstream, and the script that generates them doesn't know the full matrix of possible values (which in any case changes relatively often as we change platform versions and so on), so it doesn't do much optimisation of the output.
Attachment #8817214 - Flags: feedback?(james) → feedback+
(In reply to James Graham [:jgraham] from comment #32)
> The reason the metadata format is so awful is indeed that the files are
> autogenerated when we update the web-platform-tests from upstream, and the
> script that generates them doesn't know the full matrix of possible values
> (which in any case changes relatively often as we change platform versions
> and so on), so it doesn't do much optimisation of the output.

Which script is this? Should I be updating that instead and re-running it instead of making these changes manually?
For such a small number of changes, making the alterations manually is fine. If you want to run the script you need to download the raw logs from treeherder e.g. using testing/web-platform/update/fetchlogs.py (with arguments <repo>, <short sha1>) and then run

mach wpt-update path/to/logs/*.log

Which will slowly churn through the logs and rewrite the metadata files. But this process has a few rough edges and limited benefits in this case.
Blocks: 1322402
Alrighty, I've just made these manual changes to get the tests green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=052319a84a5625db4af4acf6e17ae5d813ee2be2
Pushed by mshal@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f82cd0199ab7
Drop universal Mac builds; r=coop,gps
Yay this landed! Boo, we didn't do bug 1310849 yet. I've locked updates to Firefox-mozilla-central-nightly-20161208030206 for the nightly channel, where the OS Version contains Darwin and buildID is < 20161208075029 (see rule id 481). ie all Mac updates won't go past 20161208075029.

After testing we can change this to just a Build Target of Darwin_x86-gcc3-u-i386-x86_64, which is what 32bit queries with. The fisrt build after this patch landed is using Darwin_x86_64-gcc3.
(In reply to Nick Thomas [:nthomas] from comment #38)
> Yay this landed! Boo, we didn't do bug 1310849 yet. I've locked updates to
> Firefox-mozilla-central-nightly-20161208030206 for the nightly channel,
> where the OS Version contains Darwin and buildID is < 20161208075029 (see
> rule id 481). ie all Mac updates won't go past 20161208075029.

Doh, somehow I got into my head that we were going to sort out update issues for aurora and beyond. Thanks for sorting this out!
https://hg.mozilla.org/mozilla-central/rev/f82cd0199ab7
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Ben/Jordan, could you have a look at the Balrog rules for Firefox on the nightly channel, to see why https://aus5.mozilla.org/update/6/Firefox/53.0a1/20161208075029/Darwin_x86_64-gcc3-u-i386-x86_64/en-US/nightly/Darwin%2015.6.0/NA/default/default/update.xml gets an update ?  Comment #38 is my attempt to block it with rule 461.

It’s not a problem right now, since the dropping-of-universal builds missed today’s m-c nightly, but it’ll be in the next one so I'd like to resolve it asap, and I'm have some technology issues right now.
Flags: needinfo?(bhearsum)
(In reply to Nick Thomas [:nthomas] from comment #41)
> Ben/Jordan, could you have a look at the Balrog rules for Firefox on the
> nightly channel, to see why
> https://aus5.mozilla.org/update/6/Firefox/53.0a1/20161208075029/
> Darwin_x86_64-gcc3-u-i386-x86_64/en-US/nightly/Darwin%2015.6.0/NA/default/
> default/update.xml gets an update ?  Comment #38 is my attempt to block it
> with rule 461.
> 
> It’s not a problem right now, since the dropping-of-universal builds missed
> today’s m-c nightly, but it’ll be in the next one so I'd like to resolve it
> asap, and I'm have some technology issues right now.

I think this is because the rule sets version to '<20161208075029', and the request you've pasted has that exact buildid. Presumably this should be '<=20161208075029', which (in combination with the OS Version field) catches all mac users on that build or older? Hoping to double check this with you before making the change, because I feel like I don't have full context.
Flags: needinfo?(bhearsum) → needinfo?(nthomas)
Thanks for looking Ben. I realized I needed to add a No-Update rule once mac users get to 20161208075029 - with that all mac updates are blocked.
Flags: needinfo?(nthomas)
Flags: needinfo?(jlund)
mshal and I finished the testing in bug 1310849 and we've re-enabled updates for 64bit builds. 32bit users stop at a buildID of 20161208075029. There's one more nightly after that, but it's on the same revision.
This probably is the final step to close Bug 787510?
Blocks: 787510
and some small (2%) talos improvements in ts_paint (browser startup):
https://treeherder.mozilla.org/perf.html#/alerts?id=4478
Blocks: 1278812
See Also: → 1325582
See Also: → 1325583
See Also: → 1325584
See bug 1325582, bug 1325583, and bug 1325584 for the update server work as this change merges upwards.
Depends on: 1333443
See Also: → 1339182
Blocks: 1339182
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: