484542 - l10n builds fire for every changeset in a push

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P3)

Description

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

In contrast to en-US builds, if a localizer sends many changesets in push, like the top block here
 http://hg.mozilla.org/l10n-central/sv-SE/pushloghtml?fromchange=f813ef345241&tochange=37031d081111
then l10n builds are fired for each changeset, eg
 http://production-master.build.mozilla.org:8010/builders/WINNT%205.2%20mozilla-central%20l10n/builds/9492
through 9500. A single build on the latest change is enough isn't it ?

Comment 1

[Armen [:armenzg]]

I believe it is enough with one build.

Taking this bug but will deal with after done with other bugs I have. Anyone can take if it has higher priority than what I have given to it.

Comment 2

[Axel Hecht [:Pike]]

In my head, this depends on flexible buildrequest merging in 0.7.10, bug 481886.

The alternative way would be to merge the changes, but that sounds hackier.

Using the tipsonly option on the pushlog hook won't work, because that doesn't return the right list of files.

Comment 3

[Axel Hecht [:Pike]]

Attachment: make the use of NoMergeStamp conditional

This is the first half for the 0.7.10-based version.

This goes along with a merge function in master.cfg like

def mergeRequests(builder, req1, req2):
    return req1.properties == req2.properties
c['mergeRequests'] = mergeRequests

(Somewhat oversimplified, you need to check the builder name if it's an l10n build, and otherwise fall back to merging of the source stamps in the requests)

Comment 4

[Armen [:armenzg]]

Comment on attachment 369059 [details] [diff] [review]
make the use of NoMergeStamp conditional

Axel, there is something weird about this patch:
>-  def __init__(self, name, inipath, treeStableTimer = None, buildOnEnUS=True):
>+  def __init__(self, name, inipath, treeStableTimer = None, buildOnEnUS=True,
>+               nomerge=True):

It says that you remove a line that already has buildOnEnUs when I don't see that parameter in our repo.
http://hg.mozilla.org/build/buildbotcustom/file/tip/l10n.py#l709

I didn't know if to give r- or r+ due to that since I believe the patch should be against the relEng's repo (I don't see that parameter in your repo either)
The patch is syntactically correct and does what you say it does correctly.

Another thing, could you please give patches with -u8p? From the patch I could not know which class was being modified.

Comment 5

[Axel Hecht [:Pike]]

This patch is coming after the one in bug 482968 on my patch queue. I'm kinda hopeful that that's gonna be tip soon :-)

To put this patch into context, 

0 A bug398954.patch
1 A bug482968.patch
2 A defaultdict.patch
3 A bug484542.patch

is what I'm using locally right now. Splendid, huh?

I've converted the repo to use -u8p patches now, want an updated patch?

Sorry, I should cross-check more thoroughly if patches depend on each other.

Comment 6

[Armen [:armenzg]]

Comment on attachment 369059 [details] [diff] [review]
make the use of NoMergeStamp conditional

Don't worry. It happens.

An updated patch should help coop when applying and commiting it for you unless you have commit access to the build repos (I can't remember).

The logic is correct and therefore r+.

Comment 7

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

Comment on attachment 369059 [details] [diff] [review]
make the use of NoMergeStamp conditional

Please make sure buildOnEnUS is set to False in __init__ prior to check-in, but looks fine otherwise.

Comment 8

[Axel Hecht [:Pike]]

Attachment: updated patch as landing

Updated patch including -pu8, and False as default for buildOnEnUS and making sure that the default for nomerge is right.

Comment 9

[Axel Hecht [:Pike]]

Landed, http://hg.mozilla.org/build/buildbotcustom/rev/e05e46778ab4.

Comment 10

[[e-mail] (:bhearsum)]

In the future, please pay attention to the check-in policies here: https://wiki.mozilla.org/ReleaseEngineering:BuildbotBestPractices

I know it's not the most convenient for you, but it eases the administration of our master instances...

Comment 11

[Axel Hecht [:Pike]]

Mind pointing out where I didn't?

Comment 12

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

bhearsum didn't know about our out-of-band conversation in #L10n. I'm planning to reconfig with this change after the current build backlog clears.

Comment 13

[[e-mail] (:bhearsum)]

(In reply to comment #11)
> Mind pointing out where I didn't?

Sure. Sorry, I should've done that in the first place.

"Patches should not be checked in until you are ready to update the master with them. This helps to avoid situations where an urgent fix needs to go in, and a random unrelated patch ends up getting enabled at the same time."

Comment 14

[Axel Hecht [:Pike]]

Clearly shows that the policy doesn't work. Can you get one that's enabling external contributions?

Comment 15

[[e-mail] (:bhearsum)]

(In reply to comment #14)
> Clearly shows that the policy doesn't work. Can you get one that's enabling
> external contributions?

Let's take this elsewhere.

Comment 16

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

production-master has been reconfig-ed with this patch.