668724 - Bug 654152 broke the ability of all-locales to be read without specifying an path on web.

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P3)

Description

[Justin Wood (:Callek)]

Bug 654152 broke the backwards-compat way of having all-locales in source repo, unless you specify the hg url by hand to the builder.

This also breaks the ability for buildbot to poll the all-locales file based on the sourcestamp rev we are using.

From Bug 654152

(In reply to comment #27)
> Comment on attachment 533772 [details] [diff] [review] [review]
> [buildbotcustom] allowing passing localesURL to L10nMixin
> 
> 
> >     def __init__(self, platform, repo='http://hg.mozilla.org/', branch=None,
> >             baseTag='default', localesFile="browser/locales/all-locales",
> >-            locales=None):
> >+            locales=None, localesURL=None):
> >+        """
> >+        You can call this class with either a defined list of locales or
> >+        a URL that contains the list of locales
> >+        """
> >         self.branch = branch
> >         self.baseTag = baseTag
> >-        self.repo = repo
> >-        self.localesFile = localesFile
> >+        self.localesURL = localesURL if localesURL else "%s%s/raw-file/%s/%s" \
> >+                          % (repo, branch, revision or self.baseTag, localesFile)
> 
> >@@ -141,25 +145,22 @@ class L10nMixin(object):
> >         in the scheduler OR it returns a Deferred.
> > 
> >         You want to call this method via defer.maybeDeferred().
> >         """
> >         if self.locales:
> >             log.msg('L10nMixin.getLocales():: The user has set a list of locales')
> >             return self.locales
> >         else:
> >-            revision = revision or self.baseTag
> >-            localesURL = "%s%s/raw-file/%s/%s" \
> >-                                            % (self.repo, self.branch, revision, self.localesFile)
> >-            log.msg("L10nMixin:: Getting locales from: "+localesURL)
> >+            log.msg("L10nMixin:: Getting locales from: "+self.localesURL)
> >             # we expect that getPage will return the output of "all-locales"
> >             # or "shipped-locales" or any file that contains a locale per line
> >             # in the begining of the line e.g. "en-GB" or "ja linux win32"
> >             # getPage returns a defered that will return a string
> >-            d = getPage(localesURL, timeout = 5 * 60)
> >+            d = getPage(self.localesURL, timeout = 5 * 60)
> >             d.addCallback(lambda data: ParseLocalesFile(data))
> >             return d
> 
> Since this is a defferred section, moving the localesURL generation up to
> init, actually drops the potential to use a source-stamps "revision" here.
> Which used to exist.
> 
> I also haven't verified, but I suspect the else clause that creates the
> localesURL would throw since revision is not in scope there.
> 
> What I would suggest/have suggested is to "generate" the localesURL where it
> is now, but allow us to replace with %s{revision} even in the config based
> url. But at least fixing the basic generation case is requested. (That said,
> it is fairly easy for me to import this code in a way that won't break
> SeaMonkey, so if it is desired to drop the support for using a sourcestamp
> revision, I'm ok with that.)

(In reply to comment #28)
> Sorry Callek this already went live a while ago but thanks for the review.
> Feel free to file a bug for any follow up work and we'll deal with it there.

Comment 1

[Justin Wood (:Callek)]

Attachment: v1.0

Fix it.

This may look odd at first glance, but if I try and do this without splitting the self.localesURL assignment to multi lines, any attempt at % formatting throws (due to revision not being in scope), but by leaving that format char in the localesURL until its used there, we have the option of expanding later.

This also has the benefit that if we ever wish to go back to in-repo localesURL we have the option of easily specifying %(revision)s in the URL we pass to this class to have it properly expanded.

If its desired to _DROP_ this backwards compat ability, lets do that instead, but I don't forsee me doing it. (since it has slightly further reaching stuff, (dead 'revision', unneeded defer.maybeDeferred etc.

Comment 2

[Justin Wood (:Callek)]

Attachment: v1.1

Updating per IRC suggestion by nthomas

Comment 3

[Armen [:armenzg]]

Comment on attachment 545586 [details] [diff] [review]
v1.1

Looks good to me.

Comment 5

[Justin Wood (:Callek)]

http://hg.mozilla.org/build/buildbotcustom/rev/d3db2b54da4a

Comment 6

[Rail Aliiev [:rail]]

Typo fixed: http://hg.mozilla.org/build/buildbotcustom/rev/b20fd70d049a

Comment 7

[Armen [:armenzg]]

Landed on production and reconfigured.

Comment 8

[Justin Wood (:Callek)]

Attachment: don't use branch=None

When deploying this to seamonkey I noticed one thing I didn't catch in my brief testing earlier. Caught with some buildbot master weird Unhandled Execptions (from a Deferred) that I had to expand out to notice the path was looking for "None" as a branch.

This should correct that, and ensure that if anyone wants to do it this way they are sure to pass in a branch or get reconfig errors

Comment 9

[Armen [:armenzg]]

Comment on attachment 564318 [details] [diff] [review]
don't use branch=None

Good catch!