434289 - Create l10n builds for mozilla-central

Status: RESOLVED DUPLICATE of bug 460791

(Release Engineering :: General, defect, P2)

Description

[Axel Hecht [:Pike]]

We'll need to get l10n builds for mozilla-central.

I'm not convinced that we should 1:1 mirror what we do on 1.9 and 1.8.1 right now. Maybe it's a good idea to experiment a bit with the lessons we learned on mozilla-central, and back port them to 1.9 as they get ready for that.

It might be good to unbreak the strong link between mozilla and l10n directories here, currently we hard-code ../l10n as the one dir to have localizations. Having that loosely would give us more flexibility for revision control systems and names of repositories.

l10n-merge and l10n-fork would benefit if we could add more than one source dir per locale, and even fallback to en-US for missing files.

We might want to unbreak the "building once for all locales", too, and move over to build-on-landing.

Some of that is already running experimentally on the l10n server, but it will need more work. Some of these are RFEs on the make files, and make-jars in particular.

Comment 1

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

Per phone call with Axel just now, this is not blocking the opening of mozilla-central. Marking as P2 and assigning to Axel as requested.

Comment 2

[chris hofmann]

whats a good next step here?

do we need one local setup in hg and landed in the http://hg.mozilla.org/ repository to start to think about how that locale and others would be 'grabbed' and built?

seems like adding 50+ l10n repositories into http://hg.mozilla.org/ would create a lot of noise in the repository list.   we might want to figure out some naming convention or try bury the locales one layer deep if something like that is possibile.

Comment 3

[Robert Kaiser]

It's surely possible to have a "l10n" subdirectory under hg.mozilla.org, just like we have "build" or "users" subdirs, we just still need to figure out how to make the dirs themselves how up on the main page, but I think reed or bhearsum are looking into that.

Comment 4

[Axel Hecht [:Pike]]

I actually envisioned the repositories to be at

http://hg.mozilla.org/l10n-central/ab-CD

I'm not sure if having an mxr spanning all localizations is a counter indication to using separate repos.

I'm not sure which content we should convert either. I'd like to import some history at least, which shouldn't be too bad for l10n repos compared to mailnews. Not sure if we should restrict to trunk, and if so, how to.

Comment 5

[Benjamin Smedberg]

Yeah, I was thinking either

http://hg.mozilla.org/l10n/mozilla-central-es-AR
http://hg.mozilla.org/l10n/mailsuite-es-AR

or

http://hg.mozilla.org/l10n/es-AR/mozilla-central
http://hg.mozilla.org/l10n/es-AR/mailsuite-central

I think we definitely should not share a repo across locales: a commit in any locale would require a merge/update/rebase in other locales, which is complicated and annoying.

Comment 6

[Robert Kaiser]

I'd be very much for having all of one locale in one repo and not having mozilla-central and comm-central (this is the 90% likely name for the calendar/mail/suite repo) separated.

Comment 7

[Axel Hecht [:Pike]]

That looks like multiple repos per locale, is there a good reason for that?

Comment 8

[Benjamin Smedberg]

Well, I was keeping the locales for mozilla-central in a parallel repo. This means that you can manage branching and trees in exact parallel, rather than having mozilla-central locales be closed for a release, but mailsuite locales still be open for development.

I really don't think we want to share repos between multiple apps when the main repository is separate.

Comment 9

[Axel Hecht [:Pike]]

The whole tree closure doesn't port over to l10n anyway, let's not put that into focus. In the end, we'll need to do something that lives up to the Firefox3Frozen cvs module on the cvs repository. Which I see involving the pushlog database. Note that the pushlog db will bet simpler by having fewer repos, and I expect the same to be true for the build architecture. Once we get the pushlog/poller combo to look at files. We can much more reliably work on that level within buildbot than associating changes with repos, buildbot doesn't like that at all.

Comment 10

[Benjamin Smedberg]

* It's simple to have buildbot associate changes with repos... you have a custom Change type, and a custom Mercurial class which can interpret that change usefully.
* What pushlog db will be simpler? There's one pushlog DB per repository.
* Why do we need pushlog/poller to look at files? That's a lot of work

Comment 11

[Robert Kaiser]

Why do localizers need to deal with multiple different repositories that contain their files?
Note that we even have SeaMonkey/Thunderbird files in mozilla-central right now (editor/ui) that will need to have their localization in the mozilla L10n repo as well if we follow your split, and maybe also have them move to the comm one later on, when we do that for the source repos.

Comment 12

[Benjamin Smedberg]

Why do we have a separate mozilla-central repository in the first place? So that we can change the platform in more radical ways without worrying so much about breaking or maintaining other projects, and so it's perfectly clear to distributors and new hackers what files are part of which projects.

Comment 13

[Axel Hecht [:Pike]]

(In reply to comment #5)
> Yeah, I was thinking either
> 
> http://hg.mozilla.org/l10n/mozilla-central-es-AR
> http://hg.mozilla.org/l10n/mailsuite-es-AR
> 
> or
> 
> http://hg.mozilla.org/l10n/es-AR/mozilla-central
> http://hg.mozilla.org/l10n/es-AR/mailsuite-central

Can you put that proposal into .l10n with a bit more detail on which files you actually anticipate where?

I don't think that this bug (or any bug for that matter) is really the right place to discuss this.

Comment 14

[[e-mail] (:bhearsum)]

(In reply to comment #10)
> * It's simple to have buildbot associate changes with repos... you have a
> custom Change type, and a custom Mercurial class which can interpret that
> change usefully.

Nit: This is do-able, but less than ideal because it will require core-buildbot code changes. I bet we could do this in a Scheduler instead.

Comment 15

[[e-mail] (:bhearsum)]

(In reply to comment #14)
> (In reply to comment #10)
> > * It's simple to have buildbot associate changes with repos... you have a
> > custom Change type, and a custom Mercurial class which can interpret that
> > change usefully.
> 
> Nit: This is do-able, but less than ideal because it will require core-buildbot
> code changes. I bet we could do this in a Scheduler instead.
> 

After further thought, bsmedberg is right. We *can* use a custom Change. I'd forgotten that these objects are created in a ChangeSource...

Comment 16

[Axel Hecht [:Pike]]

Not that that makes running a hundred different changesources for l10n attractive for either the master nor the hg server.

The fact that hgpoller currently runs the http request synchronously is just making that worse, though that's easiest to fix.

We'd still have to have files though, unless we modularize our repos to the extent that for example toolkit and firefox and thunderbird/suite-shared and thunderbird-extra and suite-extra are all in different repos.

Comment 17

[Robert Kaiser]

SeaMonkey on hg will have to pull its source from all of mozilla-central, comm-central, dom-inspector and whatever repositories venkman and ChatZilla end up in (CVS atm).
It's no problem to do such things for developers or on build machines with client.py, but it would be good to avoid such splitting of things for localizers, as it's enough challenge for most of them to even use a source code management tool, probably even more to clone and push to a collection of different repos.

Comment 18

[Benjamin Smedberg]

I have a working proof of concept master at http://hg.mozilla.org/users/bsmedberg_mozilla.com/index.cgi/l10n-buildbot/

It asynchronously loads all-locales, polls those locales for changes, and builds locales which have changed. It downloads the build from pub/mozilla.org/firefox/latest-mozilla-central for repacking, and uses mozilla-central tip.

Obviously there's a fair bit that can be improved, but it at least is something that can be put into production without any changes to buildbot.

It does involve one change in mozilla-central: I've moved the localized files from topsrcdir/../l10n to topsrcdir/l10n. Axel, if you're ok with this change I can push it: http://hg.mozilla.org/users/bsmedberg_mozilla.com/index.cgi/l10n-integration/rev/023c26731d4c

For testing I did a flag import of de and fr... I don't really care about how that import happens (with or without history) as long as it can be done quickly and with little fuss. I will post to the newsgroups.

Right now it only builds; I didn't write stage-upload or compare-locales steps... Axel if you can point me at the current correct way to run compare-locales, I will add a step to do that.

Bhearsum, how do you expect to deploy this? I figure that this could be deployed as part of the mozilla2 master, so it can share slaves with normal builds; that also gives us some future opportunities, such as chaining nightly l10n-repacks to the end of the main mozilla2 nightly builds.

Comment 19

[[e-mail] (:bhearsum)]

(In reply to comment #18)
> Obviously there's a fair bit that can be improved, but it at least is something
> that can be put into production without any changes to buildbot.
> 

Thanks for running with this.

> Right now it only builds; I didn't write stage-upload or compare-locales
> steps... Axel if you can point me at the current correct way to run
> compare-locales, I will add a step to do that.
> 

When we add this to production-master I or someone else from releng can deal with some of that.

> Bhearsum, how do you expect to deploy this? I figure that this could be
> deployed as part of the mozilla2 master, so it can share slaves with normal
> builds; that also gives us some future opportunities, such as chaining nightly
> l10n-repacks to the end of the main mozilla2 nightly builds.
> 

I think that makes a lot of sense. There's no reason I can see to keep them separate.


I had a quick look at the code and it seems OK. It sucks that we need to subclass TinderboxMailNotifier but I can't think of an alternative right now. afaict, all of the things in there should be imported into buildbotcustom before we deploy.

Comment 20

[Axel Hecht [:Pike]]

That's cool stuff, and I haven't seen anything that we couldn't merge in to the work that Armen is doing to make l10n builds better, too.

Release and I will come up with a proposal to run compare-locales.

Re the top l10n src dir, I'd love to get that into a configure flag and into autoconf.mk instead of config.mk. New repos are the new branch, I didn't try that yet, but I'd expect that to just work. I'd go as far as to error in the repackaging if the l10n-top-src dir wasn't given.

You get my personal "yay" on asynching HgPoller. I for whatever reason prefer twisted.web.client.getPage to spawning on a thread and use urlopen. By default, getPage is a pretty noisy bitch, but a 
import twisted.web.client
twisted.web.client.HTTPClientFactory.noisy = False
in master.cfg seems to fix that.

I'm OK with pushing out optimisations to how hard we punch the hg server and the master for the hg poller. We'd need joduinn to OK that, probably. We're talking about launching some 50+ connections in parallel between our most precious buildmaster and our hg server. I filed bug 443600 to discuss that.

I preferred the subclassing of changes.Change to communicate the locale than to mangling the who. I used to mess with reason at one time in my scripts and didn't get happy with that, too.

L10nRepackFactory needs a cross-platform weggie. I'm not sure if Armen has something to that extent.

Regarding the upload steps, one reason why the l10n server only does language packs is that I fear those steps, I hope that release has those. bhearsum, Armen?

I expect the deployment of this to happen as part of buildbotcustom, so we should try to get the mozillal10n.py classes moved in there.

I have a more lengthy comment on the actual code pieces in mozillal10n.py, any preferred way and time to get those?

Comment 21

[Benjamin Smedberg]

Code comments here, I suppose.

I didn't know about twisted.web.client... that makes life simpler! Currently I don't think it polls all 50 locales at once: the thread pool is only 5-10 threads by default, and it queues the requests to the available threads... do you know if twisted.web.client does the same sort of thing?

I'm going to split apart mozillal10n.py to integrate it with buildbotcustom: e.g. HgLocalePoller should live with HgPoller, etc.

What are the right buildbotcustom sources to write patches against? CVS mozilla/tools/buildbotcustom on HEAD?

Comment 22

[Axel Hecht [:Pike]]

(In reply to comment #21)
> I didn't know about twisted.web.client... that makes life simpler! Currently I
> don't think it polls all 50 locales at once: the thread pool is only 5-10
> threads by default, and it queues the requests to the available threads... do
> you know if twisted.web.client does the same sort of thing?

I don't know, and I failed to find out. I didn't see anything to that extent, but I wouldn't even remotely claim that I grok what's happening.

Comment 23

[Axel Hecht [:Pike]]

More comments on mozillal10n.py:

allLocalesURL should be in master.cfg

pair getLocale with decorateChange to abstract away whether we subclass Change or mangle who? I'd favour subclassing Change, or just setting the locale property like you do for allLocales on the SourceStamp.

        elif not source.changes:
            raise Exception("Source with no changes... don't know what to do.")

no idea why that doesn't break.

I'm not sure if SetLocalesStep is easier than subclassing Build. Whichever, I think that the locales should be a build property rather than a python property. So 

build.setProperty('locales', self.locales)
instead of
build.build_status.locales = self.locales

That you SKIPPED on start makes me favour subclassing Build even more, but that might just be because I do that myself rather easily.

In CompileLocale, I'd love to be able to get away without having to mangle the status object in a way that doesn't persist in buildbot, but we might change that logic at later point in time. (*)

L10nRepackFactory

Kinda scary to not subclass the original factory, though there's really nothing in there for you to reuse right now. Not sure if that would stay that way in later versions of buildbot :-?
The TODO is fixed?
Shorter linelengths would be nice here, and ongoing

In mergeResults, I'd rely on the numerical ordering of the results, and just use max(). You're not handling EXCEPTION yet, either. Which you'd fix in one go.

Looking at the full log of the main builder, I wonder if you want to skip out on the getTextWithHeaders and print the environment once for both the locale and the main build only.

The locale poller should have its URL in master.cfg again. As mentioned above, I'd put the mangling and demangling of the locale in the Change into two functions close to each other.


(*) For better performance and reporting, it could be better to create a build per locale, but that's tough to set up on the scheduler, and Armen and I are on it. So let's just defer that to a different bug.

Comment 25

[Benjamin Smedberg]

Attachment: TinderboxLocaleMailNotifier, rev. 1

Comment 26

[Benjamin Smedberg]

Ignore the leftover comment about not interpolating locale into binaryurl... it's a stale comment and I'll remove it before checkin.

Comment 27

[Benjamin Smedberg]

Attachment: HgAllLocalesPoller, rev. 1

This factors HgAllLocalesPoller alongside HgPoller, and shares as much code as reasonably practical.

Comment 28

[Benjamin Smedberg]

Attachment: HgAllLocalesPoller, rev. 1.1

Minor update: we need to propagate branch information across the locales poller so that we can disambiguate changes between the main tree (mozilla-central) and the localized changes. The branch is not actually used to poll the l10n repositories because it's not a simple repourl->branch mapping. And... boy, the way the Mercurial buildstep handles branches is mysterious, and kinda stupid. Oh well.

Comment 29

[Benjamin Smedberg]

Attachment: Localized buildsteps, rev. 1

Comment 30

[Benjamin Smedberg]

Attachment: Localized buildsteps, rev. 2

This is an update to the localized build steps that allows you to run the main build (mozilla-central) and localized builds (mozilla-central-l10n) in the same tree and trigger one from the other... it will check out the correct matching revision of the main-tree sources, or if triggered from a localized checkin, of the localized tree.

Comment 31

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

afaict, the options being discussed above can be summarized as:

1) create one hg l10n repo for all locales (like we had in l10n cvs repo).
2) create one hg l10n repo per locale
3) create one hg l10n repo per locale per product

Given the short timelines involved in the 3.1 release, option (1) seems the least disruptive, and most predictably easy way to get l10n into production on hg. Once we have tried (1) for a while, we could then review if/how we want to further refine our setup. After all, we still have to:
- get localizers up to speed with using hg
- write the release automation to work on hg for en-US builds
- write the release automation to work on hg for whatever l10n approach we decide
...all before we can start building FF3.1b1. 

Given our current schedule, and all these prerequisites, option (1) seems most practical imho.

$0.02.

Comment 32

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

(In reply to comment #30)
> Created an attachment (id=328565) [details]
> Localized buildsteps, rev. 2
> 
> This is an update to the localized build steps that allows you to run the main
> build (mozilla-central) and localized builds (mozilla-central-l10n) in the same
> tree and trigger one from the other... it will check out the correct matching
> revision of the main-tree sources, or if triggered from a localized checkin, of
> the localized tree.
> 


Urgh.


I note that Attachment328565 [details] [diff] implements one-repo-per-locale build process logic in buildbot custom classes. This concerns me because it:
a) makes it harder for us to do drop-in-upgrades to newer versions of buildbot and 
b) makes it harder to migrate away from buildbot to any newer improved-replacement-product in a few years time - the exact scenario we are currently trying to extract ourselves from with build logic embedded in tinderbox today. 

If we do decide to go with one-repo-per-locale, can this logic be implemented inside of the Makefiles instead?

(If I'm mis-reading the patch, pls clarify)

Comment 33

[Benjamin Smedberg]

One repo for all locales would be a huge pain for localizers:

* you have to pull all locales to change any of them (because hg doesn't support partial pull). This historically has been a much bigger deal for localizers, who are often on slow network connections and have much more limited disk space than developers
* You have to merge changes from other locales when committing. I think this is the biggest problem, because it is confusing and very different from the way CVS works.

I don't know what you mean by "logic implemented inside of the makefile"... the classes here are entirely build automation:

1) polling the pushlogs for changes
2) pulling specific locales that have changed and building them
3) notifying tinderbox

I don't see how any of these functions could be done by makefiles... they are intrinsically part of build automation, not the build system itself.

The build classes I've written are mostly targeted subclasses of existing steps... the one exception is the tinderbox-locale-status class, because the builtin tinderbox notifier is very difficult to subclass.

Comment 34

[Robert Kaiser]

Being a localizer myself (for German SeaMonkey, doing lots of platform L10n as well in the 1.9.0 timeframe), I agree with Benjamin that one repo for all locales is a pain for localizers, as they have to pull and merge all other locales' changes.
For localizers, one repo per locale but not per product (option 2 in comment #31) is probably the easiest way to do things, and directly mirrors current cvs structure in hg for them.
I also believe that we should replace the current magic to find the top l10n/ directory with a configure switch like --with-l10n-topsrcdir=path, as Axel said in comment #20.

Comment 35

[Axel Hecht [:Pike]]

(In reply to comment #32)
> I note that Attachment328565 [details] [diff] implements one-repo-per-locale build process logic
> in buildbot custom classes. This concerns me because it:
> a) makes it harder for us to do drop-in-upgrades to newer versions of buildbot
> and 
> b) makes it harder to migrate away from buildbot to any newer
> improved-replacement-product in a few years time - the exact scenario we are
> currently trying to extract ourselves from with build logic embedded in
> tinderbox today. 

Both of these are plain theory, I don't see any reason why should make l10n even a iota harder for localizers. If you can point at particular patches or issues filed on buildbot.net that conflict with the scheme proposed, I'd be happy to see those, but the mere possibility of those happening at some point in time years out is not an argument today.

One repo for l10n is not an option.

Comment 36

[[e-mail] (:bhearsum)]

Comment on attachment 328565 [details] [diff] [review]
Localized buildsteps, rev. 2

>Index: steps/l10n.py
>===================================================================
>+    def setBuild(self, build):
>+        BuildStep.setBuild(self, build)
>+        build.build_status.locales = self.locales

Since you're setting a Build level variable, just use a BuildProperty.


>+class NonLocaleMercurial(Mercurial):

OK.

>+class LocaleMercurial(Mercurial):

OK.

>+def getLocalesForRequests(requests):
>+        for c in r.source.changes:
>+            if not hasattr(c, 'locale'):
>+                raise ValueError('allLocales was not set, and a change has no locale attribute.')

This looks ok, I just want to make sure I'm reading it right. As I see it, this works in one of two ways:
1) When a build is triggered from L10nPeriodic() we simply grab the locales from the SourceStamp it provides.
2) Otherwise, we look for locales in the Changes for this build.

Is that right?


>+class RepackFactory(buildbot.util.ComparableMixin):
>+    repackFile = 'firefox-3.1a1pre.en-US.linux-i686.tar.bz2'

I don't suppose there's a way we can retrieve this at build-time, is there? Just trying to avoid an additional version bump.

>+    def newBuild(self, requests):
>+        """Create a list of steps to build these possibly coalesced requests.
>+        After initial steps, loop over the locales which need building
>+        and insert steps for each locale."""
>+
>+        locales = getLocalesForRequests(requests)
>+

I think we should bail out here, or just run a Dummy step, if len(locales) == 0. While this shouldn't happen under normal circumstances, if someone were to 'Force Build' this tree it would unnecessarily checkout, ./configure, etc.

>+        steps.append(ShellCommand(command=['mkdir', '-p', 'l10n', 'obj/dist'],
>+                                  description='running mkdirs',
>+                                  descriptionDone='ran mkdirs',

nit: the command is actually more descriptive than the descriptions :). Can you change it to 'creating obj/dist dir', or even just leaving out the description altogether so the command shows up on the waterfall.

>+        for locale in locales:
>+            steps.append(LocaleMercurial(locale=locale,
>+                                         localesBranch=self.localesBranch,
>+                                         workdir='build/l10n/%s' % locale,
>+                                         repourl=self.localesRepoURL % {'locale': locale}))
>+            steps.append(LocaleCompile(locale=locale,
>+                                       command=['make', '-C', 'obj/browser/locales', 'installers-%s' % locale]))

At some point I think it'd be great to have a separate Build for each locale, especially in the allLocales situation. It would let us parallelize this loop. This is perfectly fine for now, though.



Overall this looks pretty good. When there is time (ha) I would like to try and get rid of some of the custom code. I *feel* like we don't need custom Mercurial classes and Pollers for l10n, but maybe that's just wishful thinking.


r- because of the comments re: SetLocalesStep and bailing out if there's no locales to build.

Comment 37

[[e-mail] (:bhearsum)]

Comment on attachment 328336 [details] [diff] [review]
HgAllLocalesPoller, rev. 1.1

This looks OK. I really don't like using things like 'Change.locale' but I don't have a better solution.

Comment 38

[[e-mail] (:bhearsum)]

Comment on attachment 328154 [details] [diff] [review]
TinderboxLocaleMailNotifier, rev. 1

>Index: status/tinderbox.py
>===================================================================
>+def getLocaleResults(build, locale):
>+    for result in build.getSteps():
>+        if hasattr(result, 'locale') and result.locale == locale:
>+            yield result
>+

The 'result' variable confuses me. You're looping through and returning a BuildStepStatus, aren't you?

>+def mergeResults(r1, r2):
>+    """Find the worse of two results."""
>+    if r1 == FAILURE or r2 == FAILURE:
>+        return FAILURE
>+    if r1 == WARNINGS or r2 == WARNINGS:
>+        return WARNINGS
>+    if r1 == SUCCESS or r2 == SUCCESS:
>+        return SUCCESS
>+    return SKIPPED

Can you handle EXCEPTION here, too? I would guess it'd be analogous to FAILURE for Tinderbox mailing purposes.

>+
>+def printBuildResult(br, fd):
>+    """Take a BuildResult and turn it into a tinderbox-appropriate log."""
>+    shortText = (' '.join(br.getText()) + '\n').replace('TinderboxPrint',
>+                                                        'Tinderbox Print')
>+
>+    print >>fd, "===== BuildStep started ====="

nit: Can you use the same amount of '='s as the existing TinderboxMailNotifier?



>+class TinderboxLocaleMailNotifier(TinderboxMailNotifier):

>+    compare_attrs = list(TinderboxMailNotifier.compare_attrs).extend(['localeTree', 'localeColumnName'])

I don't think this works...extend() never returns anything:
>>> list([1,2,3,4]).extend([5,6])
>>> 

>+
>+    def __init__(self, localeColumnName, localeTree=None,
>+                 localeBinaryURL=None, **kwargs):

Can you add docstrings for these?

>+        TinderboxMailNotifier.__init__(self, **kwargs)
>+        if localeTree is None:
>+            localeTree = self.tree
>+        self.localeTree = localeTree
>+        self.localeColumnName = localeColumnName
>+        self.localeBinaryURL = localeBinaryURL
>+
>+    def buildMessage(self, name, buildstatus, result):
>+        # first send the 'main' message
>+        TinderboxMailNotifier.buildMessage(self, name, buildstatus, result)
>+
>+        # now loop through locales and build a message for each
>+        for locale in buildstatus.locales:

As with SetLocalesStep, it'd be better to use a getProperty() here.


>+        print >>mailBody, "tinderbox: tree: %s" % render_properties(self.localeTree, localeBuild)


This is going to break when we upgrade Buildbot. The properties interface has been revamped a fair bit (see http://buildbot.net/trac/ticket/87 and http://buildbot.net/trac/ticket/124). Obviously you can't code against that version since we haven't imported it yet, but is there a way you can do this without render_properties?

>+        print >>mailBody, "tinderbox: builddate: %s" % int(buildstatus.getTimes()[0])
>+        print >>mailBody, "tinderbox: status: %s" % rvtext
>+        print >>mailBody, "tinderbox: build: %s" % render_properties(self.localeColumnName, localeBuild)

See above.



r-'ing for the specific things noted inline. This seems good overall, though.

Comment 39

[Mike Schroepfer]

(In reply to comment #35)
> (In reply to comment #32)
> > I note that Attachment328565 [details] [diff] [details] implements one-repo-per-locale build process logic
> > in buildbot custom classes. This concerns me because it:
> > a) makes it harder for us to do drop-in-upgrades to newer versions of buildbot
> > and 
> > b) makes it harder to migrate away from buildbot to any newer
> > improved-replacement-product in a few years time - the exact scenario we are
> > currently trying to extract ourselves from with build logic embedded in
> > tinderbox today. 
> 
> Both of these are plain theory,


Sorry - what's theory here?  That the contract for backwards compatibilty is less for custom classes than it is for the config file?  Or that having too much logic in the build automation is bad?

> I don't see any reason why should make l10n
> even a iota harder for localizers.

In general yes we should absolutely not make life harder for localizers...

> If you can point at particular patches or
> issues filed on buildbot.net that conflict with the scheme proposed, I'd be
> happy to see those, but the mere possibility of those happening at some point
> in time years out is not an argument today.

I think the statement that looser coupling to tools is better is good enough.  BB releases tend to try hard to not break config files (or automatically upgrade them) but make no promises for custom classes. 

> One repo for l10n is not an option.

Ok but:

1) Doing it in .cfg files instead of custom classes (e.g. watch the progression of bug 434878)
2) Doing it as just straight not BB python code

Are both options.  Is #1 a possibility?

Comment 40

[Axel Hecht [:Pike]]

In the end, we need reporting per locale. The challenge to factor the shared infrastructure between those builds on the one side, and report on locale-specific problems to locale-specific channels on the other side is to be solved within the build integration.

Some of the efficiency problems can be solved in shared code. See bug 439050, for example. Others like building the locales that need to be built when they need to be built are build integration foo by definition.

The customizations that Benjamin did are mostly standard within buildbot installations, modifying the 'locale' property on the Change class withstanding. To break that, the concept of a Change had to drastically change (pun). I don't see that happening in any other scenario than a total rewrite or dropping of buildbot. Before doing that, we'll need to evaluate if that architecture can do what we need, i.e., create l10n builds and produce reports on them. Asides from all the other stuff we have in buildbotcustom.

Anyway, if there's any real worry about that, we can strip the flow down to a testcase and get that landed upstream. We shouldn't block on that, though.

Comment 41

[[e-mail] (:bhearsum)]

(In reply to comment #40)
> In the end, we need reporting per locale. The challenge to factor the shared
> infrastructure between those builds on the one side, and report on
> locale-specific problems to locale-specific channels on the other side is to be
> solved within the build integration.
> 

No arguments here.

> Some of the efficiency problems can be solved in shared code. See bug 439050,
> for example. Others like building the locales that need to be built when they
> need to be built are build integration foo by definition.
> 
> The customizations that Benjamin did are mostly standard within buildbot
> installations, modifying the 'locale' property on the Change class
> withstanding. To break that, the concept of a Change had to drastically change
> (pun). I don't see that happening in any other scenario than a total rewrite or
> dropping of buildbot. Before doing that, we'll need to evaluate if that
> architecture can do what we need, i.e., create l10n builds and produce reports
> on them. Asides from all the other stuff we have in buildbotcustom.

I feel like I should explain my stance on this a bit more...To be quite honest, I don't care if they are "mostly standard"...In the past, we've (myself included) erred far too much on the side of "subclass it" for Buildbot code. This has led to multiple instances of the same classes throughout our tree, multiple classes that basically do the same thing, and made upgrading to newer versions (Talos and 1.9 unittests are still on a 0.7.5+ version!).

I've already cited comment#38 as an example of something that we will have to change upgrading. We've had other examples of this in the past, eg,0.7.7 (iirc), which required (or strongly encouraged, at a minimum), BuildSteps to be passed as instances on the master.cfg -- that required us to patch many custom classes before upgrading. Although I cannot cite any examples like that that I *know* are incoming, I am certain we will have them again in the future.

One of the reasons we decided to move to Buildbot is because it is an upstream project. Other people drive releases, contribute bug fixes, and generally make the product better. We do not have this with Tinderbox. The more custom Buildbot code we use, the less we gain here. Is it more difficult to maneuver our build logic into Buildbot? Yes. But that extra effort is worth it, IMHO. I could go on and on about this, but I think I've said enough.

For the record, I am perfectly fine with this l10n system for the time being. I do want to make an effort to improve it and factor out custom code in the future though. The further with reach inside Buildbot, the more of a maintenance burden it is.

Comment 42

[Benjamin Smedberg]

Let's not overreact to the tinderbox mess: a large part of the tinderbox problem was not only that it was custom code, but that it was poorly factored custom code which made hacking on it extremely error-prone. We should use upstream buildbot facilities where we can, but we just have release needs which are beyond the capabilities of the present-day builtin buildbot classes.

The primary reason for my custom subclasses is threefold:

* The normal buildbot factories don't have support for a variable list of steps (repetition through a list of locales that isn't baked into the master config)

* The normal buildbot source and steps systems don't really understand the concept of multiple repositories being combined to build a single product. This is the case whether there are only two (mozilla-central and mozilla-l10n) or one for each locale

* The usual buildbot status (tinderboxstatus and the builtin waterfall) don't have a system for a single "build" to report status to multiple locations, so that localizers can see their own build results

All of these are inherent limitations of the current buildbot class hierarchy. Many of them could be rectified with upstream patching, and it may be worthwhile to try and design a generic system and send patches upstream. But in the meantime, I tend to think the current set of classes I've written use subclassing in the most minimal way practical, and are certainly much better factored and easier to understand than anything we had in tinderbox.

Comment 43

[[e-mail] (:bhearsum)]

(In reply to comment #42)
> All of these are inherent limitations of the current buildbot class hierarchy.
> Many of them could be rectified with upstream patching, and it may be
> worthwhile to try and design a generic system and send patches upstream. But in
> the meantime, I tend to think the current set of classes I've written use
> subclassing in the most minimal way practical, and are certainly much better
> factored and easier to understand than anything we had in tinderbox.
> 

No disagreement here.

Comment 44

[Axel Hecht [:Pike]]

/me nods on comments 41 - 43.

As for upstreaming, I picture that we can do something more worthwhile upstreaming in our l10n setup when hg forests are real, I somehow think that'll be the best up-stream use case. Maybe add colors or properties to Changes generically, which might aid in "partinioning" masters. We should take that discussion into the buildbot list, though.

Comment 45

[Armen [:armenzg]]

(In reply to comment #36)
> (From update of attachment 328565 [details] [diff] [review])

> >+def getLocalesForRequests(requests):
> >+        for c in r.source.changes:
> >+            if not hasattr(c, 'locale'):
> >+                raise ValueError('allLocales was not set, and a change has no locale attribute.')
> 
> This looks ok, I just want to make sure I'm reading it right. As I see it, this
> works in one of two ways:
> 1) When a build is triggered from L10nPeriodic() we simply grab the locales
> from the SourceStamp it provides.
> 2) Otherwise, we look for locales in the Changes for this build.
> 
> Is that right?
> 
How many requests we would be having? Always all?

> 
> >+class RepackFactory(buildbot.util.ComparableMixin):
> >+    repackFile = 'firefox-3.1a1pre.en-US.linux-i686.tar.bz2'
> 
> I don't suppose there's a way we can retrieve this at build-time, is there?
> Just trying to avoid an additional version bump.
> 
The package to download per platform is in the tinder-config.pl file ("l10n" branch - http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/tools/tinderbox-configs/firefox/linux/tinder-config.pl&rev=1.1.2.13) under %WGetFiles
"http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla1.9.0/firefox-%version%.en-US.linux-i686.tar.bz2" => 

One option could be that after an en-US nightly to keep the latest binary in the master as firefox.tar.bz2 (without the version) and the slave would always FileDownload the latest en-US from the master without having to specify the version. We would only keep the latest binary in the master of 3 platforms

> 
> At some point I think it'd be great to have a separate Build for each locale,
> especially in the allLocales situation. It would let us parallelize this loop.
> This is perfectly fine for now, though.
> 
We can parallelize in the code that I have been working on:
Inside the Build class that we associate to the factory in the setupBuild function, we do this:
 bd = self.buildQueue.pop()
 self.setProperty('locale', bd.locale)
The scheduler has the queue of requests that are not mergeable

Comment 46

[Mike Schroepfer]

(In reply to comment #42)
> Let's not overreact to the tinderbox mess: a large part of the tinderbox
> problem was not only that it was custom code, but that it was poorly factored
> custom code which made hacking on it extremely error-prone. We should use
> upstream buildbot facilities where we can, but we just have release needs which
> are beyond the capabilities of the present-day builtin buildbot classes.
> 
> The primary reason for my custom subclasses is threefold:

This is really helpful - questions below:

> 
> * The normal buildbot factories don't have support for a variable list of steps
> (repetition through a list of locales that isn't baked into the master config)
> 

I think Armen had solved this a different way in the other l10n cleanup he's doing - is that right?

> * The normal buildbot source and steps systems don't really understand the
> concept of multiple repositories being combined to build a single product. This
> is the case whether there are only two (mozilla-central and mozilla-l10n) or
> one for each locale

We also have to solve this for build automation don't we? Ben H. weren't you working on this?

> 
> * The usual buildbot status (tinderboxstatus and the builtin waterfall) don't
> have a system for a single "build" to report status to multiple locations, so
> that localizers can see their own build results
> 

I though we could syndicate out the status reports and/or get them from the waterfall to other locations.  Do I have that wrong?

Comment 47

[Armen [:armenzg]]

(In reply to comment #46)
> (In reply to comment #42)

> > * The normal buildbot factories don't have support for a variable list of steps
> > (repetition through a list of locales that isn't baked into the master config)
> > 
> 
> I think Armen had solved this a different way in the other l10n cleanup he's
> doing - is that right?
> 
I do not use a custom made factory but the buildClass of the factory gets assigned with a custom made Build class that knows how to pop build description objects from the custom made scheduler.
In the queue there are as many objects with a locale property set instead of setting the locale in the Change object

Comment 48

[[e-mail] (:bhearsum)]

(In reply to comment #46)
> (In reply to comment #42)
> > * The normal buildbot source and steps systems don't really understand the
> > concept of multiple repositories being combined to build a single product. This
> > is the case whether there are only two (mozilla-central and mozilla-l10n) or
> > one for each locale
> 
> We also have to solve this for build automation don't we? Ben H. weren't you
> working on this?
> 

For deps/nightlies:
To expand on what Benjamin said, Buildbot has a concept of a 'revision' for each Build. If you use multiple "Source" classes they step on each other. This is probably something that should be fixed upstream. For now, what this means to us is that we can only support building from tip. This really isn't a huge problem though, IMHO. This is the case most of the time on the m-c Buildbot. It may also mean using a ShellCommand for the second repository - I haven't tested that yet.

Either way, it's not a *big* deal, but a bit inconvenient.

For releases this isn't a problem. While it'd be nice to use Source steps for them it is not a requirement. Source steps make a lot of sense when you're polling for changes - they don't matter much when you're building from a tag.

> > 
> > * The usual buildbot status (tinderboxstatus and the builtin waterfall) don't
> > have a system for a single "build" to report status to multiple locations, so
> > that localizers can see their own build results
> > 
> 
> I though we could syndicate out the status reports and/or get them from the
> waterfall to other locations.  Do I have that wrong?
> 

We could have one TinderboxMailNotifier for every locale, or even modify the existing one to support sending to multiple trees, but neither of those will allow us to extract _only_ logs for a single locale without subclassing. Buildbot status modules (Waterfall excluded) look at the Build as a whole.

Comment 49

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

Added dependent bugs from dup'd bug#433846.

Comment 51

[Mike Schroepfer]

So do we have an agreed solution then Ben and Benjamin?

Comment 52

[[e-mail] (:bhearsum)]

I would like to know if render_properties can be replaced with something that won't immediately break, but I'm fine with this system overall, for now (as hinted at in previous comments, I'd like to work at removing custom code in the future).

Comment 53

[Axel Hecht [:Pike]]

Looking at the 0.7.8pre I'm testing right now, render_properties will fail, but not severely worse than BaseScheduler.submit(). I don't see a way to work around this though, other than to recreate a render_properties() in 0.7.8.

def render_properties(val, build):
  return build.buildstatus.properties.render(val)

could work. Untested, anyone care to suggest this upstream while I'm having dinner?

See http://thread.gmane.org/gmane.comp.python.buildbot.devel/3226/focus=3260 on my testing results so far.

Comment 54

[Axel Hecht [:Pike]]

Actually, I'd recommend build.getProperties().render(val). Tested locally on 0.7.8pre.

Comment 55

[Axel Hecht [:Pike]]

This should really block 1.9.1.

Comment 56

[Benjamin Smedberg]

Attachment: Localized buildsteps, rev. 3

Comment 57

[Benjamin Smedberg]

Note: obviously the hardcoded version number in steps/l10n.py is bad. But I don't have the time to factor/test it properly, so I'm hoping that it can land as is and whoever actually sets this up on staging can factor it for multiple platforms.

Comment 58

[Armen [:armenzg]]

(In reply to comment #57)
> Note: obviously the hardcoded version number in steps/l10n.py is bad. But I
> don't have the time to factor/test it properly, so I'm hoping that it can land
> as is and whoever actually sets this up on staging can factor it for multiple
> platforms.
> 
I have a patch that allows you to know which package to wget
https://bugzilla.mozilla.org/attachment.cgi?id=331331&action=edit
more review and work needed but it could help

Comment 59

[Axel Hecht [:Pike]]

I took a good deal of Benjamin's stuff to fix up my dashboard for hg builds. The code is in http://hg.mozilla.org/users/axel_mozilla.com/tooling/, running on l10n.m.o and reporting to http://l10n.mozilla.org/buildbot/waterfall.

I changed a few things, most notably, I added support for WithProperties at a few places. I silenced the locale pollers a bit, too.

The hardest piece right now is that I haven't found a way to test the setup without actually pushing something on the live server. I was fortunate enough to have a overall change (removing hellp) to land, so I did that one by one, and luckily seemed to run out of bugs before I ran out of locales :-)

Most importantly though, it works :-), and I can pound the hg server with some 50 locale pushlog requests with pretty prompt feedback, so the scaling stuff doesn't seem to be an immediate problem.

I opened the l10n repos to localizers just now, and they're ready for RelEng, too.

Comment 60

[Axel Hecht [:Pike]]

PS: This only softly depends on bug 449817, most of the repositories are up, we just can't create localized builds for all locales we have in cvs just yet.

Comment 61

[[e-mail] (:bhearsum)]

Comment on attachment 332728 [details] [diff] [review]
Localized buildsteps, rev. 3

I still do not like this approach as a long term solution, but this patch addresses the issues I had with the last one.

Comment 62

[Benjamin Smedberg]

Attachment: TinderboxLocaleMailNotifier, rev. 2

ok, I think I have all the nits addressed here. I haven't tested the 0.7.8-specific code at all... I just wrote the code against the documented behavior

Comment 63

[Benjamin Smedberg]

Attachment: Support extra configure arguments, rev. 1

This adds on to the "localized buildsteps" patch, which I landed... it allows the master.cfg to specify additional configure arguments, e.g. --target=i686-pc-linux on my x86-64 machine

Comment 64

[Benjamin Smedberg]

Attachment: TriggerLocalesStep, rev. 1

This is the last of the essential patches: it triggers a locales build from the end of the main build.

Comment 65

[Axel Hecht [:Pike]]

I just got hg poller to report files, diffs against what Benjamin did can be found on http://hg.mozilla.org/users/axel_mozilla.com/tooling/index.cgi/log/tip/mozilla/tools/buildbotcustom/buildbotcustom/changes/hgpoller.py. The first one is just a tad less noisy in the logs, the second one is actually using the new feature from pushlog to expose the files.

I'm not yet doing any optimizations to only pull for new changes from pushlog, I'll wait with that 'til I can pull up to tip, and/or I can query by UTC time stamps.

Comment 67

[Robert Kaiser]

Do you have a diff against http://hg.mozilla.org/build/buildbot-configs/index.cgi/file/tip/mozilla2/ that would show how to deploy this? I'd love to try all this stuff in SeaMonkey boxes even if it's not fully reviewed - that is, if we have a consistent set of patches at least that can be (incrementally?) applied to currently used code.

Comment 68

[Benjamin Smedberg]

I've got this up on the staging boxes... reassigning back to default for subsequent deployment in production.

Comment 69

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

Found in triage, revisiting after beta1.

Comment 70

[[e-mail] (:bhearsum)]

Comment on attachment 333946 [details] [diff] [review]
TinderboxLocaleMailNotifier, rev. 2

AFAIK we're going to go with the solution in bug 460791. Given that, I'm dropping this review.

Comment 71

[[e-mail] (:bhearsum)]

Comment on attachment 333990 [details] [diff] [review]
TriggerLocalesStep, rev. 1

AFAIK we're going to go with the solution in bug 460791. Given that, I'm dropping this review.