Closed
Bug 1385227
Opened 7 years ago
Closed 7 years ago
Make l10n repack builds do what they're supposed to do
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox59 fixed)
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: Pike, Assigned: Pike)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 1 obsolete file)
In bug 1370506, we're getting a clear definition on what l10n builds are supposed to do.
This bug is about actually implementing that. I'm putting this into a separate bug as the work in bug 1370506 is usable as is, and has the least side-effects.
My idea for this bug is to make the stuff the l10n builds should do actually happen. Might be a bigger and not so incremental change.
Here's my thinking, from the revelations I had in bug 1370506.
Remove most of the dependencies, and replace them with build steps.
This is going to leave us with these phases:
- unpack
- pre-libs phase
- libs
- pre-packaging
- packaging
- post-clobber
These are done in sequence, the steps within those can be done in parallel, possibly.
unpack does what it does today, INNER_UNPACKAGE. Note, I intend to drop the requirement to download and have a windows installer, which might modify code here. I can't see us using the exe at all in our builds right now.
pre-libs does l10n-merge, which does the VCS clone when needed. Not much parallism here, but we're only doing this once as part of the build. This might also do pre-clobber or staging, in parallel.
libs will do libs (go figure), but in parallel. Optionally, drop hyphenation here.
pre-packaging is where I think bug 1362617 with the multilocale.json generation should be.
packaging will package the langpack, the package, and on windows, generate the installer bits and package that up. These packaging steps can again be done in parallel.
post-clobber might be needed? Not sure, doesn't hurt to have it on the list.
Multi-locale builds would do the build, and then do a bunch of chrome-% targets, and join the rest with the pre-package phases.
libs-% and chrome-% will get a shared directory list, use run over them in parallel.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
I've got two paths down today, one was totally running out of scope, and the other was just narrowing the scope down to almost-silly.
After bug 1370506, I've submitted the latter.
I'm waiting for try to come back up before I request formal review on this.
Callek, seems this might have implications mostly around automation, is this something you should review?
Comment 3•7 years ago
|
||
(In reply to Axel Hecht [:Pike] from comment #2)
> Callek, seems this might have implications mostly around automation, is this
> something you should review?
I probably should be an additional reviewer, yes. But lets not allow me to mistake myself for a build peer :-).
The largest current concern I have is that L10n is still run by buildbot for beta/release with no ETA yet on switching it to taskcluster. So we still have to avoid breaking changes there.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
The attached patch brings in a couple of opinions:
Most of the patch is easy-peasy, just moving things from wrong places to right places.
Windows get's a whack, mostly because there's been a ton of hacks to make the double packaging create a zip, an .exe (or two, thank you stub), and a xpi.
Those hacks bleed all over the place. Bug 1388026 is a symptom, which I removed in the first patch-let of this set.
Things this patch requires:
The zip and the exe actually don't have anything different in terms of binary components. Set aside setup.exe and stub.exe, which are just makensis outputs, and not actual compiles.
Calling repack-l10n.py is only required once, and one can safely package the zip, the mar, and the exe from that.
That comes with a few fall-outs which, IMHO, make this patch kinda nice to read (if you focus on the green lines).
There's only a wget for the en-US zip. You can set the variables for the .exe, but it won't even try to grab that.
There's one phase that goes through libs.
Then we package that as a langpack.
Then we ensure the unpacked zip, and do the repack-l10n.py, and package teh zip/tar/dmg. This also does the .mar, if it's enabled. I stopped doing that in the exe on windows, because I can.
For windows, we build (un-)installer, taking the branding directly from the source branding dir.
This is not a patch queue (aside from the branding fix) on purpose. To do a real patch queue, one would need to explain the removed code, and that's crazypants.
Tested locally for mac and windows desktop, including stub installer. Waiting for try to not be scared of l10n patches to actually push to that.
Assignee | ||
Comment 7•7 years ago
|
||
Try has green bits on https://treeherder.mozilla.org/#/jobs?repo=try&revision=df08b54837e252b4a3b3b97ea04c1d56d81052fa and https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e6603f20acef731d17439ee1fd7e86567299dba
Try l10n still needs a patches to work, and I'm pretty sure the windows failure is one of the hickups that need more patching on the infra side.
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8893849 [details]
bug 1385227, use proper make steps to put l10n repacks in sequence,
https://reviewboard.mozilla.org/r/164948/#review170972
Assignee | ||
Updated•7 years ago
|
Attachment #8894544 -
Flags: review?(gps)
Attachment #8893849 -
Flags: review?(gps)
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8894544 [details]
bug 1385227, pick up (un-)installer branding from src instead dist/branding
https://reviewboard.mozilla.org/r/165702/#review174100
This seems reasonable.
Attachment #8894544 -
Flags: review?(gps) → review+
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8893849 [details]
bug 1385227, use proper make steps to put l10n repacks in sequence,
https://reviewboard.mozilla.org/r/164948/#review175444
Sorry for the long delay. These make files are insanely hard to review. For future reference, I find it easier to review files like this if things are broken up into as small of chunks as possible.
::: browser/locales/Makefile.in:132
(Diff revision 2)
> - AB_CD=$(AB_CD) \
> - MOZ_PKG_FORMAT=SFX7Z \
> - ZIP_IN='$(WIN32_INSTALLER_IN)' \
> - ZIP_OUT='$(WIN32_INSTALLER_OUT)' \
> - SFX_HEADER='$(PWD)/../installer/windows/l10ngen/7zSD.sfx \
> - $(topsrcdir)/browser/installer/windows/app.tag'
> + $(NSINSTALL) -D $(STAGEDIST)/uninstall
> + cp ../installer/windows/l10ngen/helper.exe $(STAGEDIST)/uninstall
> + $(RM) $(ABS_DIST)/l10n-stage/setup.exe
> + cp ../installer/windows/l10ngen/setup.exe $(ABS_DIST)/l10n-stage
> + $(NSINSTALL) -D '$(ABS_DIST)/$(PKG_INST_PATH)'
> + cd $(DIST)/l10n-stage; \
It would be nice if this `cd` were avoided because it impacts the commands that run after. If `$(MAKE_PACKAGE)` needs to be run from this directory, you can do `(cd $(DIST)/l10n-starge && $(MAKE_PACKAGE))`.
Attachment #8893849 -
Flags: review?(gps) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8893849 [details]
bug 1385227, use proper make steps to put l10n repacks in sequence,
https://reviewboard.mozilla.org/r/164948/#review175444
> It would be nice if this `cd` were avoided because it impacts the commands that run after. If `$(MAKE_PACKAGE)` needs to be run from this directory, you can do `(cd $(DIST)/l10n-starge && $(MAKE_PACKAGE))`.
Fixed here, and in l10n.mk where I had copied this from.
Comment 14•7 years ago
|
||
Pushed by axel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/082978a77728
pick up (un-)installer branding from src instead dist/branding r=gps
https://hg.mozilla.org/integration/autoland/rev/70bc0e060dd6
use proper make steps to put l10n repacks in sequence, r=gps
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/082978a77728
https://hg.mozilla.org/mozilla-central/rev/70bc0e060dd6
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 16•7 years ago
|
||
Backed out for breaking L10n nightlies on Windows:
https://hg.mozilla.org/mozilla-central/rev/1867d7931c0a70ab90edf4aa84876525773a7139
https://hg.mozilla.org/mozilla-central/rev/bdf687e404dc68503e668328dbcaf76667d794fe
Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=eb72c8c077518c3fdc0dca5c0a14070bd25fe3cb&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Status: RESOLVED → REOPENED
Flags: needinfo?(l10n)
Resolution: FIXED → ---
Assignee | ||
Comment 17•7 years ago
|
||
This broke windows l10n nightlies in full-update generation. Drats.
Notably, bug 313956 back in the days changed full-update generation to work by extracting the exe installer first.
Now, that's pretty pointless for a repack, as it's just unpackaging the build we just packed from l10n-stage into l10n-stage. So I'm tempted to just drop that step for repacks in http://searchfox.org/mozilla-central/source/tools/update-packaging/Makefile.in#49-53.
Rail, does that sound like a sound idea? Are you a good person to ask that question?
Full detail: I changed the full mar step to happen as part of the zip instead of as part of the exe, and as the exe is packaged after the zip now, the exe isn't there yet to unpack.
Flags: needinfo?(l10n) → needinfo?(rail)
Comment 18•7 years ago
|
||
Aiui, we don't use the complete update mar from the build; we use the complete mar from the repackage. That's because signing doesn't happen until build-signing (which signs the zip); repackage then takes the zip and creates the installer.exe and complete.mar, and repackage-signing signs those.
So, I *believe*:
- build doesn't need to create the installer.exe or complete.mar; it just needs to create the .zip, setup.exe, and setup-stub.exe for win32
- build-signing signs the zip and setup*.exe files
- repackage downloads those and creates installer.exe and complete.mar
- repackage signing downloads those and signs them
Comment 19•7 years ago
|
||
Callek correctly points out that we need the mar in the build/repack for release/beta l10n, until we get that off buildbot and in tc.
Comment 20•7 years ago
|
||
Sorry for the tardy reply, I have been slow after a long PTO. :)
I'm not the greatest expert in this domain, tbh. There is one thing that I would like to mention though.
Our release automation sill uses buildbot for l10n repacks and we should be careful to not break it.
Flags: needinfo?(rail)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 24•7 years ago
|
||
Aki managed to get a working try build for this on https://treeherder.mozilla.org/#/jobs?repo=try&revision=6dc64dc2900283b8125462fc6b84d48106afc51d as part of bug 1393190.
I just rebased the patch, and reordered them for the packager change to come before the installers-% change, as it's a dependency.
Comment 25•7 years ago
|
||
mozreview-review |
Comment on attachment 8901077 [details]
bug 1385227, generate full update from l10n-stage directly,
https://reviewboard.mozilla.org/r/172572/#review177840
lgtm
Attachment #8901077 -
Flags: review?(rail) → review+
Comment 26•7 years ago
|
||
Pushed by axel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5b1abdb8b2fb
pick up (un-)installer branding from src instead dist/branding r=gps
https://hg.mozilla.org/integration/autoland/rev/84091f931dff
generate full update from l10n-stage directly, r=rail
https://hg.mozilla.org/integration/autoland/rev/e95f8bd7b519
use proper make steps to put l10n repacks in sequence, r=gps
Comment 27•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5b1abdb8b2fb
https://hg.mozilla.org/mozilla-central/rev/84091f931dff
https://hg.mozilla.org/mozilla-central/rev/e95f8bd7b519
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Comment 28•7 years ago
|
||
It looks like these patches are breaking devedition 57.0b1 (go to build on Friday Sep15). My current understanding is:
- this works on central because we're using taskcluster, which does l10n repack -> l10n-signing -> l10n-repackage -> l10n-repackage-signing
- this is busted on beta because we're using buildbot, which does everything in the l10n repack
I believe the bustage relates to the installer. In taskcluster we ignore the installer in the l10n repack and rebuild the installer from the zipfile in the repackage task. In buildbot we use the installer from the l10n repack.
Right now we're strongly considering backing out these patches, as well as Zibi's patches that rely on these patches, from beta. Aiui these patches are needed for 58, not 57, so that gives us ~6 weeks to either migrate from buildbot to taskcluster for release l10n, or to get the buildbot repacks working with these patches. TC is strongly preferred.
Comment 29•7 years ago
|
||
Flagging Axel on NI.
:aki - can you share a link to the break log? Is there a chance that we may be able to fix the builds without having to backout patches?
Flags: needinfo?(l10n)
Comment 30•7 years ago
|
||
I think update verify and balrog submission are busted:
https://mozilla-release-logs.s3.amazonaws.com/mozilla-beta/devedition-57.0b1/build2/win64_aurora_update_verification_12_12-win64-AMGW_A1bSZ2VxCvFoAOL_w-0
https://mozilla-release-logs.s3.amazonaws.com/mozilla-beta/devedition-57.0b1/build2/%5Bfunsize%5D_Publish_to_Balrog_win32_chunk_10_for_56.0b10-win32-DHHNHzxlQam6M6M4H-8IzA-0
Comment 31•7 years ago
|
||
18:53 <•Callek> The key piece of info for zibi and axel is that the root folder in the installer is now firefox/ instead of core/
Assignee | ||
Comment 32•7 years ago
|
||
(In reply to Aki Sasaki [:aki] from comment #31)
> 18:53 <•Callek> The key piece of info for zibi and axel is that the root
> folder in the installer is now firefox/ instead of core/
I don't know what the test actually tests, but I can see that coming from this patch. If that's bad, let's back this out, both on central and beta.
Flags: needinfo?(l10n)
Assignee | ||
Comment 33•7 years ago
|
||
I've given this a https://treeherder.mozilla.org/#/jobs?repo=try&revision=aca3764fdc09e925cba02f90fd38e1972847767d.
That's two of the patches from this bug, I left the branding one alone. Both applied cleanly.
We might not have to back out the langpack changes from gandalf?
Not sure how to verify that this would work for release automation.
Assignee | ||
Comment 34•7 years ago
|
||
That didn't work, also backed out gandalf's patches on https://treeherder.mozilla.org/#/jobs?repo=try&revision=232b83769aa9a6d7c5afa0707f6f1dda43b41131
Comment 35•7 years ago
|
||
(In reply to Axel Hecht [:Pike] from comment #32)
> (In reply to Aki Sasaki [:aki] from comment #31)
> > 18:53 <•Callek> The key piece of info for zibi and axel is that the root
> > folder in the installer is now firefox/ instead of core/
>
> I don't know what the test actually tests, but I can see that coming from
> this patch. If that's bad, let's back this out, both on central and beta.
I think that's just the ultimate symptom of the problem.
The l10n repack jobs themselves don't fail, but seem to produce corrupted output. The installers don't seem to be signed properly, and can't be installed on my windows laptop.
Here's an example repack log:
https://archive.mozilla.org/pub/devedition/tinderbox-builds/mozilla-beta-l10n/release-mozilla-beta_devedition_win64_l10n_repack-bm74-build1-build110.txt.gz
And here's one of installers that's produced:
http://releases.mozilla.com/pub/devedition/candidates/57.0b1-candidates/build2/win64/ach/Firefox%20Setup%2057.0b1.exe
Assignee | ||
Comment 36•7 years ago
|
||
That try-run finished. The windows builds fail on bug 1393190 again, though.
I applied the hacks from there, and triggered https://treeherder.mozilla.org/#/jobs?repo=try&revision=661ad51795539e522b9b811cf94aa396a013487a
Comment 37•7 years ago
|
||
I'm planning to merge m-c tip over to m-b later today. I can push these backouts while I'm at it if that's the agreed-upon path forward here.
Flags: needinfo?(catlee)
Comment 38•7 years ago
|
||
Can you please don't back out the langpacks patches from Central?
If I understand correctly, the request is to back out of beta and then try to move l10n out of buikdbot over the next 6 weeks.
Comment 39•7 years ago
|
||
m-c and m-b are both tracking Fx57 until the second merge on the 21st (see dev-platform) and I'm strongly inclined to minimize any unnecessary divergence between the two for the time-being in the interests of not making keeping the two in sync a massive PITA. The plan would be to backout for now and you can re-land once m-c gets bumped to 58 later next week.
Comment 40•7 years ago
|
||
Ok, I understand.
If you can wait until later tonight I should be able to provide you a patch that backs out pike's patch without backing out the langpacks patch so that we can keep building new langpacks on Central.
If you can't wait then go ahead and back out.
Comment 41•7 years ago
|
||
Works for me.
Assignee | ||
Comment 42•7 years ago
|
||
I strongly prefer to have a consistent state on m-c and m-b right now.
Also, if this bug proves to be the culprit, we shouldn't land it again as is, because it's not creating the right installers locally for developers.
I also don't think we should land anything on m-c that hasn't proven itself on try to not break nightlies, and that's hard and time consuming as can be seen on the landings I did over the course of the day today.
Hopefully try will return some results in the next 30 mins.
Comment 43•7 years ago
|
||
I agree Axel. My only thing is that langpacks code is not causing the bug iiuc. So we should be able to isolate the blackout to not affect our work on langpacks.
Comment 44•7 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #37)
> I'm planning to merge m-c tip over to m-b later today. I can push these
> backouts while I'm at it if that's the agreed-upon path forward here.
yes, please!
Flags: needinfo?(catlee)
Assignee | ||
Comment 45•7 years ago
|
||
Try is green and I checked that the target.installer.exe from the N8 task has it's data in the core directory if I unpack it with 7z.
Comment 46•7 years ago
|
||
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #43)
> I agree Axel. My only thing is that langpacks code is not causing the bug
> iiuc. So we should be able to isolate the blackout to not affect our work on
> langpacks.
Any ETA on when this patch might be ready?
Assignee: nobody → l10n
Flags: needinfo?(gandalf)
Comment 47•7 years ago
|
||
I will be on my laptop in 3 hours. If you need to land it, go for it.
I'll write a patch to reland the langpacks bits on Central.
Flags: needinfo?(gandalf)
Comment hidden (mozreview-request) |
Comment 49•7 years ago
|
||
I'm still testing the code, but this seems to be the right backout of those patches without having to revert the langpack patches that landed on top.
I'm not sure how to test it, but if this works, I believe it would be equally valuable for fixing the regression without having to impact our work on langpacks.
If this doesn't work, let's revert everything and I'll write the new patch for langpacks to land them on central.
Comment 50•7 years ago
|
||
I verified that the patch works to build the product, package it and produce langpacks both old and new.
I only tested on linux, so not sure if this fixes the problem.
Comment 51•7 years ago
|
||
I ran the attached backout patch through Try with Pike's hackery, but it appears to be broken:
https://treeherder.mozilla.org/logviewer.html#?job_id=131548201&repo=try
I'm going to go the full backout route for now so we can get b1 built and we can sort out what to re-land later.
Comment 52•7 years ago
|
||
backout bugherder |
Backed out for breaking Beta release automation per bug 1385227.
https://hg.mozilla.org/mozilla-central/rev/1a2f0dc86126
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•7 years ago
|
Target Milestone: mozilla57 → ---
Comment 53•7 years ago
|
||
Sylvestre, the backout has been merged to Beta. Want to attempt a build3?
Flags: needinfo?(sledru)
Comment 56•7 years ago
|
||
Callek said he'll take a look at it after 57 is done.
Flags: needinfo?(bugspam.Callek)
Assignee | ||
Comment 57•7 years ago
|
||
FWIW, I did a naive attempt to rebase this to m-c, and pushed to try, https://treeherder.mozilla.org/#/jobs?repo=try&revision=884ca6e285b062b321d7125307f63de0f2e253d0. Should still break windows installers, though.
Assignee | ||
Comment 58•7 years ago
|
||
Quoting from the build log of one of my try pushes in the past few hours:
01:43:12 INFO - mv -f '../../dist/l10n-stage/firefox-58.0a1.de.win64.zip' 'z:/task_1510275267/build/src/obj-firefox/dist/install/sea/firefox-58.0a1.de.win64.installer.exe'
Wait, I broke the installer by just renaming the zip to exe? New push on its way.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8894544 -
Attachment is obsolete: true
Assignee | ||
Comment 61•7 years ago
|
||
The funk in the interdiff https://reviewboard.mozilla.org/r/164946/diff/4-5/ is that I moved the MOZ_PKG_FORMAT=SFX7Z from the rule definition to the make invocation when packaging the installer.
In the rule definition, it's not set when the MAKE_PACKAGE variable is evaluated, so it was actually still packaging a zip, and then renaming it to .exe.
Now that I set it on invocation, the package format is actually 7z, and that works.
Pushed to try, https://treeherder.mozilla.org/#/jobs?repo=try&revision=66a93f5dfe243c0d71226c741477b8db0f61d4e9&selectedJob=143705871, downloaded the .exe for German, installed, got a German install experience, a German browser, and a German uninstall experience. And if I 7z x target.installer.exe, I have setup.exe and core as top-level entries.
Unflagging Callek, this is ready for review.
Flags: needinfo?(bugspam.Callek)
Updated•7 years ago
|
Attachment #8893849 -
Flags: review?(core-build-config-reviews)
Updated•7 years ago
|
Attachment #8893849 -
Flags: review?(gps)
Comment 62•7 years ago
|
||
mozreview-review |
Comment on attachment 8893849 [details]
bug 1385227, use proper make steps to put l10n repacks in sequence,
https://reviewboard.mozilla.org/r/164948/#review206502
I'll repeat what I stated a few months ago: this is insanely hard to review. I'm not confident this is correct. But I can't say it is wrong either. But it looks like an improvement. So let's move forward.
Attachment #8893849 -
Flags: review?(gps) → review+
Comment 63•7 years ago
|
||
Pushed by axel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a3e00fccf660
generate full update from l10n-stage directly, r=rail
https://hg.mozilla.org/integration/autoland/rev/0abbf75bd0ec
use proper make steps to put l10n repacks in sequence, r=gps
Comment 64•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a3e00fccf660
https://hg.mozilla.org/mozilla-central/rev/0abbf75bd0ec
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Updated•7 years ago
|
status-firefox57:
affected → ---
Updated•7 years ago
|
Product: Core → Firefox Build System
Comment 65•7 years ago
|
||
For my own notes: the first patch landed here, https://hg.mozilla.org/mozilla-central/rev/5b1abdb8b2fb, is equivalent to https://bugzilla.mozilla.org/show_bug.cgi?id=1430313. The same idea in two related systems.
You need to log in
before you can comment on or make changes to this bug.
Description
•