482968 - l10n repacks shouldn't trigger on en-US check-ins

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Axel Hecht [:Pike]]

en-US check-ins don't make it into builds yet to repack, so there's no need to really run repacks on en-US landings.

This is a slight shift in plan, and we'll keep the logic to run on en-US check-ins in the scheduler, as we continue to run compare-locales on a cross-platform builder. That one is going to run on the l10n server for a while, though.

Comment 1

[Axel Hecht [:Pike]]

Attachment: one possible buildbotcustom way of doing it

Not sure if I like this patch.

It binds us to have only enUS-triggered builds on the scheduler or not. What we really want is some matrix like

      | linux | win | mac | compare-locales
------+-------+-----+-----+----------------
en-US |       |     |     |     X
l10n  |  X    |  X  |  X  |     X
linux |  X    |     |     |
win   |       |  X  |     |
mac   |       |     |  X  |

where the column is what triggered the build, en-US check-in, l10n check-in, linux, win, mac nightly, and the row is the triggered build. linux, win, mac repacks and compare-locales database reporting pseudo build.

If we kept the the compare-locales builder on a separate master, this patch would work, as the builder-triggered builds will be covered by trigger steps, which can be made platform-specific rather easily.

Going for a review request on Armen with this patch, tested locally with both enUS on and off.

Comment 2

[Armen [:armenzg]]

Comment on attachment 367069 [details] [diff] [review]
one possible buildbotcustom way of doing it

The patch looks good and having the parameter will allow us to have it off until we decide what is the way to go.

Let's see what coop and joduinn think of this way to go; Having a separate master does not sound to me as the long run solution.

Comment 3

[Axel Hecht [:Pike]]

Coop, joduinn, what's your take?

Comment 4

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

Comment on attachment 367069 [details] [diff] [review]
one possible buildbotcustom way of doing it

This is running on staging-master right now.

Comment 5

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

(In reply to comment #3)
> Coop, joduinn, what's your take?

Is there any reason why we'd need an entirely separate master vs. a new factory to handle the compare-locales-only logic? Maybe I'm not following something here.

Comment 6

[Axel Hecht [:Pike]]

With this patch, we're handling buildOnEnUS per l10nbuilds.ini, which is a tad coarse if we wanted to run compare-locales-only logic on the same master.

Maybe we can do this piecewise, and use buildOnEnUS as default, and add an option to overwrite from l10nbuilds.ini later? Or I could try to come up with a revised patch.

It's not totally bad, though, as the compare-locales-only build needs a rather special slave anyway to get the rich reporting out.

Comment 7

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

Comment on attachment 367069 [details] [diff] [review]
one possible buildbotcustom way of doing it

No issues on staging, this one can land when ready.

Comment 8

[Axel Hecht [:Pike]]

http://hg.mozilla.org/build/buildbotcustom/rev/30740a1c1041, marking FIXED.

Comment 9

[Armen [:armenzg]]

It might be a silly question. If we want to trigger nothing when an en-US entity changes, shouldn't we pass *false* for buildOnEnUs to avoid adding the EnUsDispatcher?
26 +        if buildOnEnUS:
27 +          log2.msg('adding EnDispatcher for %s on %s for %s' %
28 +                   (section, branch, ' '.join(endirs)))
29 +          scheduler.addDispatcher(EnDispatcher(endirs, branch, section))

Is there a following patch to change the master configuration to pass that value?

(I might be completely off with this)

Comment 10

[Axel Hecht [:Pike]]

Either that or make the default value of the argument False.

Thanks for catching that.

Comment 11

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

(In reply to comment #10)
> Either that or make the default value of the argument False.

I think the sensible default is False (off) so that we need to explicitly enable it if we want it, given previous discussions.

Comment 12

[Axel Hecht [:Pike]]

Agreed. Wanna just change that with rs=me?

Comment 13

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

(In reply to comment #12)
> Agreed. Wanna just change that with rs=me?

Done.

changeset:   231:cdda843619ea