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)

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

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: Pike, Assigned: Pike)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

bug 1385227, use proper make steps to put l10n repacks in sequence,
59 bytes, text/x-review-board-request
gps
: review+
Details
bug 1385227, generate full update from l10n-stage directly,
59 bytes, text/x-review-board-request
rail
: review+
Details
Bug 1385227 - Backout of the patches.
59 bytes, text/x-review-board-request
Details 
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.
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?
Blocks: 1387485
(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.
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.
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.
Comment on attachment 8893849 [details]
bug 1385227, use proper make steps to put l10n repacks in sequence,

https://reviewboard.mozilla.org/r/164948/#review170972
Attachment #8894544 - Flags: review?(gps)
Attachment #8893849 - Flags: review?(gps)
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 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 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.
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
Blocks: 1392245
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)
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
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.
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)
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 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+
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
Blocks: 1399523
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.
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)
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/
(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)
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.
(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
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
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)
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.
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.
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.
Works for me.
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.
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.
(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)
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.
(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)
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)
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.
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.
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.
Backed out for breaking Beta release automation per bug 1385227.
https://hg.mozilla.org/mozilla-central/rev/1a2f0dc86126
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
status-firefox57: fixed → affected
Target Milestone: mozilla57 → ---
Sylvestre, the backout has been merged to Beta. Want to attempt a build3?
Flags: needinfo?(sledru)
Thanks, just started!
Flags: needinfo?(sledru)
Blocks: 1400285
Callek said he'll take a look at it after 57 is done.
Flags: needinfo?(bugspam.Callek)
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.
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.
Attachment #8894544 - Attachment is obsolete: true
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)
Attachment #8893849 - Flags: review?(core-build-config-reviews)
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+
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
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
Blocks: 1436662
Product: Core → Firefox Build System
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.

Attachment

General

Creator:
Created:
Updated:
Size: