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.

Realistically, this won't happen for awhile. It's blocked on bug 600832, anyways.

This bit us hard in b11, we should do this soon.

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.

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-%.

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.

How does this overlap with the changes in bug 525438 ?

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.

(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.

(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.

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.

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.

(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

(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.

Comment on attachment 519920 [details] [diff] [review] update buildbot-configs patch Landed on default

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

(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".

(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?

(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.

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.

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.

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

Comment on attachment 519191 [details] [diff] [review] allow override It's looking like this probably won't be necessary.

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

Turns out we can workaround bug 600832, removing dependency.

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.

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.

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.

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

(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.

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.

(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.

(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.

I tested these calls by hand on one platform, needs more testing.

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.

(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.

(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?

Yes.

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.

Comment on attachment 522671 [details] [diff] [review] add l10n-check calls Need to drop all of the RepackFactory stuff from this.

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?

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.

Comment on attachment 521894 [details] [diff] [review] enable pretty names tests on mac, but not on mozilla-2.0 changeset: 4346:d4610fb766b6

Required a bustage fix, which was tracked in bug 647264. All fixed now, so we're done here.

More problems with this, backing out :(

(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...

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.

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.

Comment on attachment 524751 [details] [diff] [review] fix l10n check tests Landed on default

Comment on attachment 524756 [details] [diff] [review] enable/disable tests properly Landed on default

Landed cleanly this time.