run make package/installer/update-packaging with MOZ_PKG_PRETTYNAMES=1 on dep/nightly builds

RESOLVED FIXED

Status

Release Engineering
General
P3
normal
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: bhearsum, Assigned: bhearsum)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 8 obsolete attachments)

1.03 KB, patch
catlee
: review+
bhearsum
: checked-in+
Details | Diff | Splinter Review
3.22 KB, patch
catlee
: review+
bhearsum
: checked-in+
Details | Diff | Splinter Review
1.04 KB, patch
catlee
: review+
bhearsum
: checked-in-
Details | Diff | Splinter Review
5.33 KB, patch
catlee
: review+
bhearsum
: checked-in-
Details | Diff | Splinter Review
7.56 KB, patch
catlee
: review+
bhearsum
: checked-in+
Details | Diff | Splinter Review
3.86 KB, patch
catlee
: review+
bhearsum
: checked-in+
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
In order to get better testing to this code path we should start running the packaging targets with it on for dep and nightly opt builds. This should be done in addition to running it without pretty names, and upload should be run without it. Doing so should test the code paths without affecting what we upload.

Probably should avoid doing this on debug builds.
(Assignee)

Comment 1

8 years ago
Realistically, this won't happen for awhile. It's blocked on bug 600832, anyways.
Depends on: 600832
Priority: -- → P5
(Assignee)

Comment 2

8 years ago
This bit us hard in b11, we should do this soon.
Assignee: nobody → bhearsum
Priority: P5 → P3
(Assignee)

Comment 3

7 years ago
Created attachment 519191 [details] [diff] [review]
allow override

I started poking at this on Friday. As a first pass, I'm going to focus on Linux (32 and 64) and Windows en-US only, because they don't have anything immediately blocking us AFAICT. Once bug 600832 is fixed we should be able to turn this on for Mac, too.

We should do this for l10n, too, but it's more of a challenge because our nightly/release l10n jobs are very different both in terms of the harness (BuildFactory vs scripts) as well as the Makefile targets we use. For nightlies we use wget-en-US and unpack to prepare to run installer-%, for releases we manually wget and don't do any unpacking. This makes this bug challenging because we can't run wget-en-US/unpack with pretty names enabled, because the paths that get set in package-name.mk doesn't match what's on FTP. But, we must run installers-% with pretty names to do this test, which doesn't work unless we manually specify ZIP_IN/WIN32_INSTALLER_IN.

I'm thinking of adding support to the l10n Makefiles to override where wget-en-US downloads the files to, by having it put them in $(ZIP_IN)/$(WIN32_INSTALLER_IN). By doing this, I believe we could run wget-en-US/unpack with them set (but *not* pretty names), and that should allow us to then run installers-% with pretty names.

I've attached a completely untested patch along the lines of above. Axel, any thoughts on this? I can use different variables for this if it makes sense.
Attachment #519191 - Flags: feedback?(l10n)
(Assignee)

Comment 4

7 years ago
I gave this patch a test on Linux. It seemed to work perfectly fine with the existing process (eg, no ZIP_IN/WIN32_INSTALLER_IN overriding). It also worked fine with ZIP_IN being set for wget-en-US/unpack, and then ZIP_IN and MOZ_PKG_PRETTYNAMES being set for installers-%.
(Assignee)

Comment 5

7 years ago
Created attachment 519215 [details] [diff] [review]
enable pretty names tests on en-US builds

This, combined with a buildbot-configs patch that enables them for linux/windows, seems to work fine. I still need to do additional verification to make sure subsequent runs aren't affected in any way, but I've verified that the right steps are run on dep/nightlies on linux/windows, and *not* run on leak test builds.
Attachment #519215 - Flags: feedback?(catlee)
(Assignee)

Comment 6

7 years ago
Created attachment 519216 [details] [diff] [review]
enable pretty names tests some places
Attachment #519216 - Flags: feedback?(catlee)
How does this overlap with the changes in bug 525438 ?
(Assignee)

Comment 8

7 years ago
I'm not seeing any overlap between this bug and bug 525438. Should I be?
Comment on attachment 519215 [details] [diff] [review]
enable pretty names tests on en-US builds

only comment is that I have an aversion to steps with spaces in their names.
Attachment #519215 - Flags: feedback?(catlee) → feedback+

Updated

7 years ago
Attachment #519216 - Flags: feedback?(catlee) → feedback+
(In reply to comment #8)
> I'm not seeing any overlap between this bug and bug 525438. Should I be?


For one the patch here conflicts with attachment 519443 [details] [diff] [review] (easier to see now that I have a single patch for it). Also, that patch makes the wget-en-US target work for pretty names, maybe hacking around that not being the case isn't needed.
(Assignee)

Comment 11

7 years ago
(In reply to comment #10)
> (In reply to comment #8)
> > I'm not seeing any overlap between this bug and bug 525438. Should I be?
> 
> 
> For one the patch here conflicts with attachment 519443 [details] [diff] [review] (easier to see now that
> I have a single patch for it). Also, that patch makes the wget-en-US target
> work for pretty names, maybe hacking around that not being the case isn't
> needed.

Ah, ok. Maybe we'll leave the l10n-side of this until that lands, then, assuming it lands in the near future.
(Assignee)

Comment 12

7 years ago
Axel, any idea how far off bug 525438 is? Given that this is a very simple patch, I'd rather get this in sooner if that bug isn't going to be done in the next week or so.
(Assignee)

Comment 13

7 years ago
Created attachment 519920 [details] [diff] [review]
update buildbot-configs patch

This patch has been tested for days in staging now. Everything looks fine: pretty names tests are passing, subsequent dep builds' packages look fine, upload fine. I had to disable pretty names tests for old-style mac universals (1.9.1 and 1.9.2) because they're broken there in a different way from bug 600832. Not sure if we care to debug it.
Attachment #519215 - Attachment is obsolete: true
Attachment #519216 - Attachment is obsolete: true
Attachment #519920 - Flags: review?(catlee)
(Assignee)

Comment 14

7 years ago
Created attachment 519921 [details] [diff] [review]
update configs patch that uses 'win' instead of 'win32' to debug windows
Attachment #519921 - Flags: review?(catlee)

Updated

7 years ago
Attachment #519920 - Flags: review?(catlee) → review+

Updated

7 years ago
Attachment #519921 - Flags: review?(catlee) → review+
(In reply to comment #12)
> Axel, any idea how far off bug 525438 is? Given that this is a very simple
> patch, I'd rather get this in sooner if that bug isn't going to be done in the
> next week or so.

The l10n-merge bug is now fixed-in-bs, http://hg.mozilla.org/projects/build-system/rev/fff61d7cc077
(Assignee)

Comment 16

7 years ago
(In reply to comment #15)
> (In reply to comment #12)
> > Axel, any idea how far off bug 525438 is? Given that this is a very simple
> > patch, I'd rather get this in sooner if that bug isn't going to be done in the
> > next week or so.
> 
> The l10n-merge bug is now fixed-in-bs,
> http://hg.mozilla.org/projects/build-system/rev/fff61d7cc077

Cool, I'll likely land what I need to in there, first. I'll still need equivalent patches for 1.9.2 and maybe 1.9.1 though.
(Assignee)

Comment 17

7 years ago
Comment on attachment 519920 [details] [diff] [review]
update buildbot-configs patch

Landed on default
Attachment #519920 - Flags: checked-in+
(Assignee)

Comment 18

7 years ago
Comment on attachment 519921 [details] [diff] [review]
update configs patch that uses 'win' instead of 'win32' to debug windows

Landed on default
Attachment #519921 - Flags: checked-in+
(In reply to comment #16)
> 
> Cool, I'll likely land what I need to in there, first. I'll still need
> equivalent patches for 1.9.2 and maybe 1.9.1 though.

IMHO, it'd make a lot of sense to try to get the whole train on 1.9.1/2, I would love for that to happen for the merge patch, too.

I know that my merge patch will look slightly different, because some things from central haven't been backported already, maybe it makes sense to just have an extra bug for "bring 1.9.2/1.9.1 up to speed with central".
(Assignee)

Comment 20

7 years ago
(In reply to comment #19)
> (In reply to comment #16)
> > 
> > Cool, I'll likely land what I need to in there, first. I'll still need
> > equivalent patches for 1.9.2 and maybe 1.9.1 though.
> 
> IMHO, it'd make a lot of sense to try to get the whole train on 1.9.1/2, I
> would love for that to happen for the merge patch, too.

Sure, I don't have the time to commit to doing that though. I'm happy to wait a short period of time for someone else to do it, but I think the value of having these tests is too high to block for very long. The patch required for this bug is very small, anyways, so it shouldn't make a big difference to porting that work back, right?
(Assignee)

Comment 21

7 years ago
(In reply to comment #13)
> Created attachment 519920 [details] [diff] [review]
> update buildbot-configs patch
> 
> This patch has been tested for days in staging now. Everything looks fine:
> pretty names tests are passing, subsequent dep builds' packages look fine,
> upload fine. I had to disable pretty names tests for old-style mac universals
> (1.9.1 and 1.9.2) because they're broken there in a different way from bug
> 600832. Not sure if we care to debug it.

Looks like the old style universals fail due to bug 544928.
(Assignee)

Comment 22

7 years ago
Created attachment 520712 [details] [diff] [review]
patch to l10n repack code to use wget/unpack targets

I tested this against build-system on Linux, worked fine. If other platforms test out ok this will make the pretty names tests in the nightly l10n builds a lot easier.
(Assignee)

Comment 23

7 years ago
Tested out fine on windows and Mac, too. Looks like we'll be able to get this going across all platforms on mozilla-central pretty soon.
(Assignee)

Comment 24

7 years ago
The first two patches (turning on pretty names tests for linux/win32 en-US builds) went into production on Monday.
(Assignee)

Comment 25

7 years ago
Comment on attachment 519191 [details] [diff] [review]
allow override

It's looking like this probably won't be necessary.
Attachment #519191 - Attachment is obsolete: true
Attachment #519191 - Flags: feedback?(l10n)
(Assignee)

Updated

7 years ago
Attachment #520712 - Attachment is obsolete: true
(Assignee)

Comment 26

7 years ago
Created attachment 521562 [details] [diff] [review]
enable pretty names tests for l10n, make mac ones possible

This has tested fine for Linux and Mac so far, still waiting on Windows l10n results.
(Assignee)

Comment 27

7 years ago
Created attachment 521563 [details] [diff] [review]
enable mac pretty names tests
(Assignee)

Comment 28

7 years ago
Turns out we can workaround bug 600832, removing dependency.
No longer depends on: 600832
(Assignee)

Comment 29

7 years ago
Comment on attachment 521563 [details] [diff] [review]
enable mac pretty names tests

Oh, I still need to test 1.9.1 and 1.9.2, too.
(Assignee)

Comment 30

7 years ago
Created attachment 521894 [details] [diff] [review]
enable pretty names tests on mac, but not on mozilla-2.0

Can't enable these for mozilla-2.0 Mac until bug 525438 lands on mozilla-2.0. Technically, it works for en-US, but the flag doesn't distinguish. The patch applies cleanly to mozilla-2.0 so I don't think it's worth creating a second flag.
Attachment #521562 - Attachment is obsolete: true
Attachment #521563 - Attachment is obsolete: true
Attachment #521894 - Flags: review?(catlee)
(Assignee)

Comment 31

7 years ago
Created attachment 521895 [details] [diff] [review]
fix mac, l10n pretty names tests

This fixes Mac pretty names tests by re-running postflight_all prior to doing the test. The target takes just over 5 minutes to run, which is really sucky, but until bug 600832 is fixed this is our workaround.

On the l10n side, things are a bit confusing. We have to run the download/unpack targets without pretty names being set, because we're downloading nightly builds. Then, we have to store the ZIP_IN/WIN32_INSTALLER_IN values without pretty names set, and override them in the installers-% target. If we don't do this, it looks for pretty-style en-US builds that don't exist.

I've tested this in staging across 1.9.1, 1.9.2, m-c, and 2.0. It works everywhere except for Mac on 2.0, as described in my previous comment.
Attachment #521895 - Flags: review?(catlee)
Comment on attachment 521895 [details] [diff] [review]
fix mac, l10n pretty names tests

Having to re-download the file isn't ideal...can we back it up somewhere?

Looks good otherwise
Attachment #521895 - Flags: review?(catlee) → review+

Updated

7 years ago
Attachment #521894 - Flags: review?(catlee) → review+
(Assignee)

Comment 33

7 years ago
(In reply to comment #32)
> Comment on attachment 521895 [details] [diff] [review]
> fix mac, l10n pretty names tests
> 
> Having to re-download the file isn't ideal...can we back it up somewhere?

I was considering that, but it would mean adding things that are conditional on testPrettyNames in one of the other add*Steps methods, which I thought was a bit confusing. On Mac, the platform with the largest package size, the download takes between 2 and 13 seconds in staging. I'd bet it'll be consistently on the low side in production where we don't have to content with a really slow staging-stage machine. Do you still want me to address this?
What are we testing here exactly? Just trying to get a grip on our testing coverage here.

From my hacking on the merge bug, I found four stooges to be funny on pretty:

- wget
- unpack
- upload

The forth thing I see covered so far, from what I can tell in the patch, is 

- packaging command breaks due to spaces in target paths.

I'm wondering if this is the right cost-benefit ratio. Or if it'd be more bang for the buck to get the factory used for try to support pretty names, and together with l10n-check of the merge bug be able to just occasionally test this on request.

Side note, I can't reproduce this right now, but I overwrote the version variable to be something non-pre in my tests quite frequently to get the full space fun. But I can't reproduce that right now. To some extent, I needed to to download the candidates, but I thought there was more.
(Assignee)

Comment 35

7 years ago
The primary thing we're testing here is "do the pretty name packaging targets work". Specifically, the "package", "installer", "complete-patch", and "installers-%" ones. Anything else is a bonus. Getting l10n nightlies and releases back using the same code paths is also desired, but separate from this bug.
Then I guess running 

make package
make l10n-check

is the better test, both with MOZ_PKG_PRETTYNAMES=1. l10n-check verifies that repacks work on the assumption that the binary is where make package would put it, just having a toolkit/defines.inc. Does so by doing a x-test locale with just that, and then installers-x-test with LOCALE_MERGEDIR to something empty.

As an aside, different bug, I guess, I'd favour having l10n-check run as part of build automation anyway, for the trees that have support for it.

I don't think that we're on the right track to test this in repacks, thinking about it some more. If localizers had the chance to break this, we're having a code problem. Also, if we have a problem in the main code that breaks repacks with pretty, it shouldn't show up on the l10n builds, but on the core builds.
(Assignee)

Comment 37

7 years ago
(In reply to comment #36)
> Then I guess running 
> 
> make package
> make l10n-check
> 
> is the better test, both with MOZ_PKG_PRETTYNAMES=1. l10n-check verifies that
> repacks work on the assumption that the binary is where make package would put
> it, just having a toolkit/defines.inc. Does so by doing a x-test locale with
> just that, and then installers-x-test with LOCALE_MERGEDIR to something empty.

To be clear, you're talking about running these on an en-US build, right?

> I don't think that we're on the right track to test this in repacks, thinking
> about it some more. If localizers had the chance to break this, we're having a
> code problem. Also, if we have a problem in the main code that breaks repacks
> with pretty, it shouldn't show up on the l10n builds, but on the core builds.

Good point. I'd still like to run the tests as part of l10n builds however, because they more closely match what happens during a release than l10n-check.
(In reply to comment #37)
> 
> To be clear, you're talking about running these on an en-US build, right?

Yep.

<...>

> Good point. I'd still like to run the tests as part of l10n builds however,
> because they more closely match what happens during a release than l10n-check.

Serious question, "more closely match" how?

My intention with l10n-check was to establish a contract between the core repos and l10n. Anything that is in l10n shouldn't be able to break l10n-check. Trying to anticipate edge-cases, this could be violated by repack rules that under the condition that a file is in a localization, break, and only for pretty.

While I'm side-tracking things, different branding dirs are another difference between nightlies and release.
(Assignee)

Comment 39

7 years ago
(In reply to comment #38)
> (In reply to comment #37)
> > 
> > To be clear, you're talking about running these on an en-US build, right?
> 
> Yep.
> 
> <...>
> 
> > Good point. I'd still like to run the tests as part of l10n builds however,
> > because they more closely match what happens during a release than l10n-check.
> 
> Serious question, "more closely match" how?

By calling the exact same target that does the repack work. I don't doubt that l10n check covers most things, but it still seems beneficial to be running installers-% with pretty names, too, to further cover that base.
(Assignee)

Comment 40

7 years ago
Created attachment 522671 [details] [diff] [review]
add l10n-check calls

I tested these calls by hand on one platform, needs more testing.
Attachment #521895 - Attachment is obsolete: true
l10n-check does run installers-%, see http://hg.mozilla.org/mozilla-central/file/fff61d7cc077/browser/locales/Makefile.in#l268. That's hooked up through the browser/build.mk in the top-level makefile, too.

I really don't see the value in running pretty tests on repacks, the people that could fix anything aren't going to look there. If anyone looks, really. Given how often I see folks complaining how expensive our repacks are, I don't see a cost-benefit ratio, too.
(Assignee)

Comment 42

7 years ago
(In reply to comment #41)
> l10n-check does run installers-%, see
> http://hg.mozilla.org/mozilla-central/file/fff61d7cc077/browser/locales/Makefile.in#l268.
> That's hooked up through the browser/build.mk in the top-level makefile, too.
> 
> I really don't see the value in running pretty tests on repacks, the people
> that could fix anything aren't going to look there. If anyone looks, really.
> Given how often I see folks complaining how expensive our repacks are, I don't
> see a cost-benefit ratio, too.

Even under ideal circumstances (same scripts/buildbot integration) running l10n-check against an en-US build is different than running pretty tests against l10n nightly/dep builds:
- Different mozconfig used
- Different automation driving it

Your point about the people able to do anything not watching these builds is well taken, though, and given that the l10n-check does run the installers-% target, I don't feel as strong of a need to run tests on repacks immediately. However, when we figure out a way to test repacks in a way that's visible to the pusher (possibly as part of bug 588746, we should start running those tests there.

Can you get on board with that?
oh, heavens, that's a bug I filed :-).

Actually, bug 588746 didn't intend anything else than running l10n-check on the main Firefox tree, just that I didn't have a well-defined minimal set of data for a fake locale, nor a target to test it at the time.

If we'd end up with running l10n-check on regular naming and pretty here, bug 588746 is just a dupe of this one.

Doing that test with repack configs is probably easy-ish to do for the non-pretty test, with a trigger or something. For the pretty case, we'd easily end up with pretty packages on ftp tinderboxbuilds, not sure if that's good or bad. You would test the full monty that way, so maybe it's good.

I expect that to have quite some fallout on the repack factories, though. They'd need to tweak themselves to report differently, and skip hg steps, etc.
(Assignee)

Comment 44

7 years ago
(In reply to comment #43)
> Actually, bug 588746 didn't intend anything else than running l10n-check on the
> main Firefox tree, just that I didn't have a well-defined minimal set of data
> for a fake locale, nor a target to test it at the time.
> 
> If we'd end up with running l10n-check on regular naming and pretty here, bug
> 588746 is just a dupe of this one.

Okay, ignore the bug number then. I'll file a new one about end to end l10n testing on check-in.

So, to summarize the last few comments, the work remaining in this bug relating to l10n is:
- Enable l10n-check tests on en-US builds in pretty and non-pretty scenarios
- Explicitly do not enable pretty names tests in l10n dep/nightly builds.

Are we on the same page?
(Assignee)

Comment 46

7 years ago
OK, great. Thanks for working through this with me.

I filed bug 646159 about running l10n dep builds on check in, which would be a good place to do end to end pretty names tests *and* have notifications go to the person actually at fault.
(Assignee)

Comment 47

7 years ago
Comment on attachment 522671 [details] [diff] [review]
add l10n-check calls

Need to drop all of the RepackFactory stuff from this.
Attachment #522671 - Attachment is obsolete: true
(Assignee)

Comment 48

7 years ago
Created attachment 523007 [details] [diff] [review]
turn off pretty names tests for l10n; keep factory code

Same as before, without the misc.py bits that enable it. The code is tested, so I figure there's no point in dropping it entirely.

I didn't address the re-downloading bit because I didn't see a response to this:
> > Having to re-download the file isn't ideal...can we back it up somewhere?
> 
> I was considering that, but it would mean adding things that are conditional on
> testPrettyNames in one of the other add*Steps methods, which I thought was a
> bit confusing. On Mac, the platform with the largest package size, the download
> takes between 2 and 13 seconds in staging. I'd bet it'll be consistently on the
> low side in production where we don't have to content with a really slow
> staging-stage machine. Do you still want me to address this?
Attachment #523007 - Flags: review?(catlee)

Updated

7 years ago
Blocks: 646732

Updated

7 years ago
Attachment #523007 - Flags: review?(catlee) → review+
(Assignee)

Comment 49

7 years ago
Comment on attachment 523007 [details] [diff] [review]
turn off pretty names tests for l10n; keep factory code

changeset:   1630:257dd825e333

Will get merged to production in the next reconfig.
Attachment #523007 - Flags: checked-in+
(Assignee)

Comment 50

7 years ago
Comment on attachment 521894 [details] [diff] [review]
enable pretty names tests on mac, but not on mozilla-2.0

changeset:   4346:d4610fb766b6
Attachment #521894 - Flags: checked-in+
(Assignee)

Comment 51

7 years ago
Required a bustage fix, which was tracked in bug 647264.

All fixed now, so we're done here.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Depends on: 647264
Resolution: --- → FIXED
(Assignee)

Comment 52

7 years ago
More problems with this, backing out :(
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 53

7 years ago
(In reply to comment #52)
> More problems with this, backing out :(

Specifically, we're trying to run these tests on debug builds, for some reason...
(Assignee)

Updated

7 years ago
Attachment #523007 - Flags: checked-in+ → checked-in-
(Assignee)

Updated

7 years ago
Attachment #521894 - Flags: checked-in+ → checked-in-
(Assignee)

Comment 54

7 years ago
Created attachment 524751 [details] [diff] [review]
fix l10n check tests

This patch fixes the problems that were found when the previous one landed, specifically: that l10n check was run on debug builds and also on branches where it doesn't exist.
Attachment #524751 - Flags: review?(catlee)
(Assignee)

Comment 55

7 years ago
Created attachment 524756 [details] [diff] [review]
enable/disable tests properly

So, we can't run pretty names tests on Mac on 1.9.1 or 1.9.2. We can't run them on 2.0 either, until bug 525438 lands there. We also can't do any l10n check tests on 1.9.1 or 1.9.2. Again, we can't do them on 2.0 either, until the same bug is landed.

All other platforms/branches run them fine. I tested explicitly on all platforms on 1.9.1, 1.9.2, 2.0, and mozilla-central.
Attachment #524756 - Flags: review?(catlee)

Updated

7 years ago
Attachment #524751 - Flags: review?(catlee) → review+

Updated

7 years ago
Attachment #524756 - Flags: review?(catlee) → review+
(Assignee)

Comment 56

7 years ago
Comment on attachment 524751 [details] [diff] [review]
fix l10n check tests

Landed on default
Attachment #524751 - Flags: checked-in+
(Assignee)

Comment 57

7 years ago
Comment on attachment 524756 [details] [diff] [review]
enable/disable tests properly

Landed on default
Attachment #524756 - Flags: checked-in+
(Assignee)

Comment 58

7 years ago
Landed cleanly this time.
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Blocks: 670496

Updated

7 years ago
Blocks: 670497
Product: mozilla.org → Release Engineering

Updated

5 years ago
No longer blocks: 670496
You need to log in before you can comment on or make changes to this bug.