478436 - Integrate compare-locales with l10n mercurial repack builds

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P2)

Description

[Chris Cooper [:coop] (he/him)]

Axel's setup/code already does this, and Armen omitted it from his repack-on-change work (bug 464163) for want of a version of compare-locales in the build repo to use (bug 458938).

We'll be working from Axel's buildbotcustom repo (http://hg.mozilla.org/users/axel_mozilla.com/buildbotcustom/) until we've tested and are ready to push this back into the build repo.

Comment 1

[Axel Hecht [:Pike]]

The reporting we do on compare-locales is good enough for regular repacks, I intend to run db-aware rich reporting on separate builders from the repacks.

Seems like we could get away with just cloning, update -r RELEASE_AUTOMATION, and to the call into scripts/compare-locales with lib added to the PYTHONPATH.

No tweaking of the slave commands list required at this level, and thus, no requirement to have compare-locales deployed at slave start time. Fun.

Comment 2

[Chris Cooper [:coop] (he/him)]

Still planning on working on this, but am not actively working on it at present.

Axel has discussed using a separate cross-platform builder for running compare-locales (and doing repacks) in bug 472197.

Comment 3

[Axel Hecht [:Pike]]

We still need compare-locales to get l10n-merge going. What we don't need is the json-based reporting and database backend stuff, which the setup on the l10n server does.

Comment 4

[Armen [:armenzg]]

Axel, I have few questions:
* is l10n-merge involved with adding missing entities when doing a repackage?
* is l10n-merge part of the compare-locales repository?

Comment 5

[Axel Hecht [:Pike]]

yes, and yes.

Comment 6

[Armen [:armenzg]]

(In reply to comment #5)
> yes, and yes.
>
I have been looking at your repositories and I see a lot of new repos that I had not seen before. I guess some of them like buildbotcustom and buildbot-configs are there to help you when you want to upstream your work to our repos.

I am trying to look for the master.cfg file that you are running at the l10n.m.o server. I want to see what were the extra steps that made a repack have the missing entities. 
http://hg.mozilla.org/users/axel_mozilla.com/l10n-master/file/4f95b95163a8/master.cfg 
or this one:
http://hg.mozilla.org/users/axel_mozilla.com/tooling/file/a31e2fc2dc70/buildbot-configs/l10n/master.cfg#l153

Anyways, all of these questions is that I am trying to find out what were the steps that added to a repack the missing entities.

Wow, it feels like ages since I worked with your code structure.

Comment 7

[Axel Hecht [:Pike]]

compare-locales and buildbotcustom are on the releng repos now. compare-locales has one pending patch to support in-source runs, but that's only in a local mq, and bug 486111.

I could possibly delete my clones of buildbotcustom and buildbot-configs, the latest patches never went there.

master.cfg is of no use for you, you want to look at the factories, like the one I linked to in the fennec builds. Not that those really tell you how things should work on the releng builds.

What this bug should do is:

- make the main compare-locales script sys.exit(1) on missing entities.
- add a hg clone/pull step for the compare-locales repo
- hg update that to the releng tag (which needs to be bumped to the version that does sys.exit(1))
- run compare-locales -m absolute-path-to-mergedir ...., after the update to binary revs.
- feed that to dir to make installers-ab-CD.

Comment 8

[Axel Hecht [:Pike]]

I did the fix to compare-locales to fail on missing entities, coop, can I get a rs to bump RELEASE_AUTOMATION over to http://hg.mozilla.org/build/compare-locales/rev/fb9cba887364?

Comment 9

[Chris Cooper [:coop] (he/him)]

(In reply to comment #8)
> I did the fix to compare-locales to fail on missing entities, coop, can I get a
> rs to bump RELEASE_AUTOMATION over to
> http://hg.mozilla.org/build/compare-locales/rev/fb9cba887364?

r+ from me.

Comment 10

[Chris Cooper [:coop] (he/him)]

(In reply to comment #9)
> (In reply to comment #8)
> > I did the fix to compare-locales to fail on missing entities, coop, can I get a
> > rs to bump RELEASE_AUTOMATION over to
> > http://hg.mozilla.org/build/compare-locales/rev/fb9cba887364?

Sorry, maybe I was confused. Axel: were you looking for someone to bump the tag, or approval to do it yourself?

Comment 11

[Axel Hecht [:Pike]]

Landing this got bumped in my vacation time, just did.

http://hg.mozilla.org/build/compare-locales/rev/36c0ca554ff9

Comment 12

[Chris Cooper [:coop] (he/him)]

(In reply to comment #7)
> - feed that to dir to make installers-ab-CD.

I have the rest hooked up correctly now in my local setup AFAICT, but I'm unsure how to pass the merged dir into "make installers-ab-CD." Do I just need to set LOCALE_MERGEDIR? How does this work if no merging was required, i.e. the merge dir was not created?

Comment 13

[Axel Hecht [:Pike]]

The mergedir needs to be created and clobbered by the build setup. But it doesn't matter if it's empty, or non-existant for the actual installers step. So you just do 

make installers-ab-CD LOCALE_MERGEDIR=/absolute/path/to/merge/dir/base.

http://l10n.mozilla.org/buildbot/builders/m-c%20linux%20repack/builds/14547/steps/shell_9/logs/stdio is the stdio of how I'm calling it on the l10n server.

To emphasize, I had that busted on my initial setup, make sure that you clobber the mergedir before running compare-locales, otherwise you end up with junk from the previous merges in your repacks. I'm not doing that on purpose inside compare-locales as I don't want to break the file system of localizers if they make a mistake in the arguments. Thus removal of files is not done implicitly.

Comment 14

[Chris Cooper [:coop] (he/him)]

Attachment: Adding back in the compare-locales step (with merging)

I have the compare step turning red right now when it has non-zero exit status, i.e. merging happens. Not sure if that's what we want or not.

Currently running on staging-master: http://staging-master.build.mozilla.org:8010/waterfall

Comment 15

[Chris Cooper [:coop] (he/him)]

Attachment: Config changes required for compare-locales

Comment 16

[Axel Hecht [:Pike]]

Comment on attachment 380159 [details] [diff] [review]
Adding back in the compare-locales step (with merging)

There seems to be something funny in the merging here, Talos factory pieces, clobber changes, drop of the tinderboxprint stuff.

The other piece is that the stunt for PYTHONPATH is already done on the slaveside, according to http://hg.mozilla.org/build/buildbot/file/b1bef83b3c4f/buildbot/slave/commands.py#l301, so just setting that should add to the python path on the slave instead of setting things. http://djmitche.github.com/buildbot/docs/0.7.10/#ShellCommand has docs.

Regarding the success value: For builds that are not release builds, I'd like to warn on failure and merge. For release builds, we shouldn't merge and fail on failure.

Comment 17

[Axel Hecht [:Pike]]

Comment on attachment 380161 [details] [diff] [review]
Config changes required for compare-locales

r- on the release master running merge, there should be a corresponding change to what I commented on on the buildbotcustom patch.

Comment 18

[Chris Cooper [:coop] (he/him)]

Attachment: Adding back in the compare-locales step (with merging), v2

Fixed issues raised in comment #16.

I've decoupled the compare-locales setup from the actual running of compare-locales so that it is trivial to override for the release case. Not sure whether we need to override for mobile as well.

Comment 19

[Axel Hecht [:Pike]]

Comment on attachment 380230 [details] [diff] [review]
Adding back in the compare-locales step (with merging), v2

r=me.

The compare-locales step without the merge dir but with the merge option on should be good enough.

Comment 20

[Axel Hecht [:Pike]]

Comment on attachment 380161 [details] [diff] [review]
Config changes required for compare-locales

Based on the buildbotcustom patch, r=me.

Comment 21

[Chris Cooper [:coop] (he/him)]

(In reply to comment #19) 
> The compare-locales step without the merge dir but with the merge option on
> should be good enough.

Is this in reference to mobile, i.e. comment 18? 

I'm not sure exactly what you mean here. Are you suggesting we run compare-locales with a merged dir, i.e "compare-locales -m merged l10n.ini ..." but then not pass that merged dir to the repack step?

Comment 22

[Chris Cooper [:coop] (he/him)]

Nick offered in IRC to test the release portion of this patch. Since no one else (save bhearsum) has done a 3.5-era release yet, I may just take him up on that.

Comment 23

[Chris Cooper [:coop] (he/him)]

Attachment: Config changes for both staging and production

Staging portion was already OKed by Axel in attachment 380161 [details] [diff] [review], this simply rolls-in the equivalent changes for production.

Comment 24

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 380322 [details] [diff] [review]
Config changes for both staging and production

>diff -r b11abe9661a7 mozilla2/release_master.py
>+        compareLocalesRepoPath=COMPARE_LOCALES_REPO_PATH,
>+        compareLocalesTag=COMPARE_LOCALES_TAGbuildSpace=2,

'spect you meant to put the nightly_config prefix on there, like the staging config. Testing release side in staging. compare-locales is just expected to report the l10n status, right ?

Comment 25

[Axel Hecht [:Pike]]

(In reply to comment #21)
> (In reply to comment #19) 
> > The compare-locales step without the merge dir but with the merge option on
> > should be good enough.
> 
> Is this in reference to mobile, i.e. comment 18? 
> 
> I'm not sure exactly what you mean here. Are you suggesting we run
> compare-locales with a merged dir, i.e "compare-locales -m merged l10n.ini ..."
> but then not pass that merged dir to the repack step?

Oops, got confused there, you overwrite the actual compare-locales step for the release factory.

Yes, in release, we just want to see the status, and bail on failure.

The production config patch is borked, there's at least a buildSpace=2 that's left over from copy and paste or something.

Comment 26

[Nick Thomas [:nthomas] (UTC+12)]

Working fine on the staging config:

http://staging-master.build.mozilla.org:8010/builders/linux_repack/builds/1544
http://staging-master.build.mozilla.org:8010/builders/win32_repack/builds/933
http://staging-master.build.mozilla.org:8010/builders/macosx_repack/builds/1076
(final upload failure is deliberate)

If I had a nit (and you know I can't help myself) it's that the 'rm -rf merged' step doesn't need to happen for releases, and so could move from BaseRepackFactory.compareLocalesSetup to BaseRepackFactory.compareLocales. Looks pretty harmless though.

Comment 27

[Axel Hecht [:Pike]]

Comment on attachment 380322 [details] [diff] [review]
Config changes for both staging and production

>diff -r b11abe9661a7 mozilla2/master.cfg
>+                    compareLocalesTag=COMPARE_LOCALES_TAGbuildSpace=2,
>diff -r b11abe9661a7 mozilla2/mobile_master.py
>+                    compareLocalesTag=COMPARE_LOCALES_TAGbuildSpace=2,
>diff -r b11abe9661a7 mozilla2/release_master.py
>+        compareLocalesTag=COMPARE_LOCALES_TAGbuildSpace=2,

r- on the trailing buildSpace=2 in the non-staging configs, as per IRC with Nick.

Comment 28

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 380322 [details] [diff] [review]
Config changes for both staging and production

I'm happy if you make the same set of changes to both sets of configs. I can't speak to how mobile l10n will go though.

Comment 29

[Chris Cooper [:coop] (he/him)]

(In reply to comment #28)
> (From update of attachment 380322 [details] [diff] [review])
> I'm happy if you make the same set of changes to both sets of configs. I can't
> speak to how mobile l10n will go though.

I want to run watch a mobile run today before I push this out. 

Thanks for the release run last night.

Comment 30

[Chris Cooper [:coop] (he/him)]

So I just completed a nightly repack series in staging. We had a mix of results, e.g.:

* success:
http://staging-master.build.mozilla.org:8010/builders/Maemo%20mozilla-central%20l10n/builds/482
* warning:
http://staging-master.build.mozilla.org:8010/builders/Maemo%20mozilla-central%20l10n/builds/483
* failure:
http://staging-master.build.mozilla.org:8010/builders/Maemo%20mozilla-central%20l10n/builds/485

From what I can tell, we only see outright failures if the locale is missing an entire file. 

Axel: is this the expected result?

Comment 31

[Axel Hecht [:Pike]]

Not really, missing files shouldn't bust. For updater.ini, that's currently broken, I filed bug 495467 on that.

I'm currently trying to verify that the merged strings show up in the generated builds, which I fail to for the lt build.

Comment 32

[Axel Hecht [:Pike]]

Now that the upload issues are fixed, I see merged builds for mobile.

With the config changes and fixes, this should be good to go.

Comment 33

[Chris Cooper [:coop] (he/him)]

Comment on attachment 380230 [details] [diff] [review]
Adding back in the compare-locales step (with merging), v2

changeset:   322:06ead23a9a42

Took Nick's suggestion and moved the rm step into the base compareLocales def.

Comment 34

[Robert Kaiser]

Wow, from what I see that forces us to run compare-locales and have merge fully working in all comm-central builds that use the repack factories already. I just hope that doesn't bust our builds completely, esp. given that I see a buildbotcustom patch landed that has no according config changes landed, right? mean that anyone updating to current buildbotcustom tip is busted in any case, I suspect.

Comment 35

[Chris Cooper [:coop] (he/him)]

Comment on attachment 380322 [details] [diff] [review]
Config changes for both staging and production

changeset:   1177:6161ad7c7e01

Comment 36

[Chris Cooper [:coop] (he/him)]

Just reconfig-ed production-master with these changes.

Comment 37

[Chris Cooper [:coop] (he/him)]

(In reply to comment #34)
> Wow, from what I see that forces us to run compare-locales and have merge fully
> working in all comm-central builds that use the repack factories already. I
> just hope that doesn't bust our builds completely, esp. given that I see a
> buildbotcustom patch landed that has no according config changes landed, right?
> mean that anyone updating to current buildbotcustom tip is busted in any case,
> I suspect.

To encapsulate the IRC discussion from this afternoon:

* the corresponding config changes landed shortly thereafter and contained the fixes suggested by Nick/Axel. We were waiting on a mobile review (bug 495475) that also needed to land at the same time to keep mobile working.

* we'll be sure to keep KaiRo in the loop on future core class changes