775236 - need a python tool that can generate update snippets from a patcher config

Status: RESOLVED FIXED

(Release Engineering :: Release Automation: Other, defect)

Description

[[e-mail] (:bhearsum)]

Attachment: patcher config parser for python

Part of bug 575317 is generating AUS snippets for the additional partials we'll be generating. The patcher config format needs to be extended to support this (described in more detail in https://bugzilla.mozilla.org/show_bug.cgi?id=773290#c3). We decided it's not worthwhile hacking on the Perl patcher scripts, and instead have opted to write a Python-based tool that understands them enough to generate snippets.

Attached is a start on that, which is capable of understanding any of the currently used patcher configs (beta, release, esr). I'd love to get some feedback on it before I work on extending it to support the proposed format in bug 773290, and working on the snippet generation bits.

Comment 1

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

Comment on attachment 643518 [details] [diff] [review]
patcher config parser for python

Looks good to me. There are some other settings we can make in current-update that will need adding (showPrompt etc). Some attribution on the apache logparser would be good to. Good luck on the next bit.

Comment 2

[[e-mail] (:bhearsum)]

Attachment: patcher config library + snippet creation script

OK, I think this is feature-complete at this point. I think the code is pretty well documented/readable, but here's a high level summary:
* Adds a parser for .checksums files
* Removes old (and dead AFAICT) Darwin_Universal-gcc3/Darwin_x86_64-gcc3 update platforms
* Adds ftp -> bouncer/update platform mappings, so we don't have to go through Buildbot
* Adds a library for dealing with patcher configs that can: parse them (including the new <partials> section), substitute paths, appropriately figure out optional attributes based on schema version, and generate a list of update paths
* Adds a snippet library that defines the schema2 optional attributes, can create schema1+schema2 snippets, and figure out real snippet paths based on product/version/platform/buildid/locale/channel/type (including handling multiple update platforms for one build)
* Includes an import of apache_conf_parser w/ my tiny tweak to make it work with patcher configs (from https://github.com/bhearsum/apache_conf_parser)
* Adds a snippet creation script that fully replaces patcher2.pl --create-patchinfo mode.

I've done reruns of Firefox 15.0b2, 10.0.6esr and Thunderbird 14.0, 15.0b1, and 10.0.6esr to verify my work. Comparing against the actual pushed snippets shows no differences. I had to add <partials> to the patcher configs and also change the schema version for a bunch of Thunderbird versions to make it work. It seems like patcher2.pl doesn't look at the schema for older versions when making decisions about how to generate snippets.

I couldn't do great tests against Firefox 14.0.1 in the end because the 13.0.1 -> 14.0.1 checksums weren't in the .checksums files. I was able to hack the script a bit to do it, and the only differences ended up being in checksums/filesize of those partials.

Comment 3

[[e-mail] (:bhearsum)]

Attachment: necessary patcher config adjustments

Comment 4

[Rail Aliiev [:rail]]

Comment on attachment 646693 [details] [diff] [review]
patcher config library + snippet creation script

Review of attachment 646693 [details] [diff] [review]:
-----------------------------------------------------------------

Great job!

I really liked this patch -- well documented and covered by tests!

However, since this piece is very important, sensitive and may be harmful for us, I prefer to be a little bit paranoid and find quarrel in a straw! :)

::: lib/python/build/checksums.py
@@ +2,5 @@
> +    """Parses checksums files that the build system generates and uploads:
> +        https://github.com/mozilla/mozilla-central/blob/master/build/checksums.py"""
> +    fileInfo = {}
> +    for line in contents.splitlines():
> +        hash_, type_, size, file_ = line.split(" ", 3)

Can you use line.split(None, 3) instead so you split regardless of type of white space (spaces or tabs)?

::: lib/python/release/updates/patcher.py
@@ +6,5 @@
> +from release.updates.snippets import SCHEMA_2_OPTIONAL_ATTRIBUTES
> +
> +log = logging.getLogger()
> +
> +def substitutePath(path, platform='UNDEFINED', locale='UNDEFINED', version='UNDEFINED'):

Why do you need default 'UNDEFINED' parameters here?

@@ +222,5 @@
> +        return cu
> +
> +    def parseRelease(self, release):
> +        r = {}
> +        for node in release:

Can we somehow validate for mandatory sections/variables? This loop iterates over all existing variables but doesn't make sure that the current release section has all mandatory nodes. Having a test won't be bad too.

::: lib/python/release/updates/snippets.py
@@ +6,5 @@
> +    'showPrompt', 'showNeverForVersion', 'showSurvey', 'actions', 'licenseUrl',
> +    'billboardURL', 'openURL', 'notificationURL', 'alertURL',
> +)
> +
> +schema1_snippet = """\

can you rename this to something like schema1_snippet_template?

@@ +19,5 @@
> +extv=%(appVersion)s
> +detailsUrl=%(detailsUrl)s
> +"""
> +
> +schema2_snippet = """\

the same here

@@ +43,5 @@
> +    hashFunction = hashFunction.upper()
> +    if schema == 1:
> +        if other:
> +            raise SnippetError("Optional attributes are not supported for schema version 1")
> +        return schema1_snippet % locals()

Can you explicitly specify variables instead of locals()?

@@ +48,5 @@
> +    elif schema == 2:
> +        for k in other:
> +            if k not in SCHEMA_2_OPTIONAL_ATTRIBUTES:
> +                raise SnippetError("Invalid optional attribute: '%s'" % k)
> +        snippet = schema2_snippet % locals()

the same here

::: scripts/updates/create-snippets.py
@@ +31,5 @@
> +
> +    log_level = logging.INFO
> +    if options.verbose:
> +        log_level = logging.DEBUG
> +    logging.basicConfig(level=log_level, format="%(message)s")

How about using something like this?
format="%(asctime)s - %(levelname)s - %(message)s"

@@ +63,5 @@
> +        fromAppVersion = fromRelease['extension-version']
> +        detailsUrl = substitutePath(pc['current-update']['details'], platform, locale, appVersion)
> +        optionalAttrs = pc.getOptionalAttrs(fromVersion)
> +        checksumsUrl = substitutePath(pc['release'][version]['checksumsurl'], platform, locale)
> +        checksumsFile = path.join(options.checksumsDir, '%(appName)s-%(platform)s-%(locale)s-%(version)s' % locals())

Can you explicitly pass variables instead of locals() to prevent run time errors?

@@ +77,5 @@
> +            try:
> +                cacheChecksums(platform, locale, parseChecksumsFile(open(checksumsFile).read()))
> +                log.debug("Using on-disk checksums for %s %s" % (platform, locale))
> +            except (IOError, ValueError):
> +                contents = requests.get(checksumsUrl).content

I think, that you need to pass config=dict(danger_mode=True) to make it raise exceptions and wrap it with retry or pass config=dict(max_retries=5) and check if requests.get(checksumsUrl).ok is True before using its content. Not sure if the latter method supports sleeping between requests.

@@ +83,5 @@
> +                # Cache the sums in-memory and on-disk.
> +                cacheChecksums(platform, locale, parseChecksumsFile(contents))
> +                f = open(checksumsFile, 'w')
> +                f.write(contents)
> +                f.close()

Since we rely on python 2.6 due to requests module, can you use "with" statement here?

@@ +112,5 @@
> +                    if not path.exists(path.dirname(snippet)):
> +                        makedirs(path.dirname(snippet))
> +                    f = open(snippet, 'w')
> +                    f.write(contents)
> +                    f.close()

Can you use "with" here as well?

Comment 5

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

Comment on attachment 646693 [details] [diff] [review]
patcher config library + snippet creation script

A couple of nits from a brief skim, but broadly this looks good. Still need to wrap my head around it properly though.

>diff --git a/lib/python/build/checksums.py b/lib/python/build/checksums.py
>+def parseChecksumsFile(contents):
>+    """Parses checksums files that the build system generates and uploads:
>+        https://github.com/mozilla/mozilla-central/blob/master/build/checksums.py"""

Nit: point to hg.m.o.

>diff --git a/lib/python/release/platforms.py b/lib/python/release/platforms.py
> update_platform_map = {
...
>+    'macosx': ['Darwin_x86-gcc3-u-ppc-i386', 'Darwin_ppc-gcc3-u-ppc-i386'],

I suspect this can go too, seems pretty unlikely we'll do another 3.6 -> latest update now that we have a permanent background update in place.

Comment 6

[[e-mail] (:bhearsum)]

(In reply to Rail Aliiev [:rail] from comment #4)
> ::: lib/python/release/updates/patcher.py
> @@ +6,5 @@
> > +from release.updates.snippets import SCHEMA_2_OPTIONAL_ATTRIBUTES
> > +
> > +log = logging.getLogger()
> > +
> > +def substitutePath(path, platform='UNDEFINED', locale='UNDEFINED', version='UNDEFINED'):
> 
> Why do you need default 'UNDEFINED' parameters here?

This function is used to substitute various paths - which all require different platforms. I can rework this a bit to avoid those, and only substitute when the %foo% version exist. Doing that would mean we raise when missing a variable which may be better....what do you think?

The rest of your comments make sense to me, I'll address them.

Comment 7

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

Comment on attachment 646696 [details] [diff] [review]
necessary patcher config adjustments

>Index: mozEsr10-thunderbird-branch-patcher2.cfg
>===================================================================
>             <partial>
>-                betatest-url   http://stage.mozilla.org/pub/mozilla.org/thunderbird/nightly/10.0.6esr-candidates/build1/update/%platform%/%locale%/thunderbird-10.0.5esr-10.0.6esr.partial.mar
>-                esrtest-url   http://stage.mozilla.org/pub/mozilla.org/thunderbird/nightly/10.0.6esr-candidates/build1/update/%platform%/%locale%/thunderbird-10.0.5esr-10.0.6esr.partial.mar
>-                path   thunderbird/nightly/10.0.6esr-candidates/build1/update/%platform%/%locale%/thunderbird-10.0.5esr-10.0.6esr.partial.mar
>-                url   http://download.mozilla.org/?product=thunderbird-10.0.6esr-partial-10.0.5esr&os=%bouncer-platform%&lang=%locale%
>+                betatest-url   http://stage.mozilla.org/pub/mozilla.org/thunderbird/nightly/15.0b1-candidates/build1/update/%platform%/%locale%/thunderbird-14.0b5-15.0b1.partial.mar
>+                esrtest-url   http://stage.mozilla.org/pub/mozilla.org/thunderbird/nightly/15.0b1-candidates/build1/update/%platform%/%locale%/thunderbird-14.0b5-15.0b1.partial.mar
>+                path   thunderbird/nightly/15.0b1-candidates/build1/update/%platform%/%locale%/thunderbird-14.0b5-15.0b1.partial.mar
>+                url   http://download.mozilla.org/?product=thunderbird-15.0b1-partial-14.0b5&os=%bouncer-platform%&lang=%locale%
>             </partial>

r+ if you leave this out, and the new config bumper is going to wholesale replace <partials> blocks.

Comment 8

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

Comment on attachment 646693 [details] [diff] [review]
patcher config library + snippet creation script

First off, I agree this is very nicely written code! Reducing 3050 lines of perl down to 460 lines of python (950 if you count the config parser) is a huge win. Mostly I'm suggesting some improvements.

General question - will we tag the tools repo with an UPDATE_PACKAGING_Rn tag (and use that in the buildbot/scriptFactory) to allow for needing to change this code for different branches ? Perhaps we deal what if the need arises.

>diff --git a/lib/python/build/checksums.py b/lib/python/build/checksums.py
>+def parseChecksumsFile(contents):

We could do some sanity checks on the file content, eg
 * size > 0
 * hash_ is well formed in length and chars used

>+        # Same goes for the hash.
>+        elif fileInfo[file_]['hashes'].get(type_, hash_) != hash_:

The fallback to hash_ here is sneaky!

>diff --git a/lib/python/mozilla_buildtools/test/test_build_checksums.py b/lib/python/mozilla_buildtools/test/test_build_checksums.py

No test for the differing hash case ?

diff --git a/lib/python/mozilla_buildtools/test/test_release_updates_patcher.py b/lib/python/mozilla_buildtools/test/test_release_updates_patcher.py
>+    def testParseCurrentUpdate(self):
>+        dom = self._parse(goodCurrentUpdateSection)
>+        expected = {
>+            'channel': ['release'],

I had a style question - in some cases you have expected objects close to config files, and in others (like here) they're separated. Any reason for that ? If not it makes sense to put them together when the length is over some threshold.

>+    def testGetUrlComplete(self):
>+        pc = samplePatcherConfigObj
>+        with mock.patch.dict('release.platforms.ftp_bouncer_platform_map', {'a': 'aa'}):
>+            got = pc.getUrl('12.0', 'a', 'a', 'complete', 'release')
>+            self.assertEquals(got, 'http://aa.a.complete')
>+    def testGetUrlCompleteOverride(self):
>+    def testGetUrlPartial(self):
>+    def testGetPathComplete(self):
>+    def testGetPathPartial(self):

In tests like this it would be good to use 'p', 'pp', and 'l' to confirm that the ordering is correct.

>+class TestSubstitutePath(unittest.TestCase):
>+    def testNoSubstitutes(self):
>+    def testSubstituteLocale(self):
>+    def testSubstituteBouncerPlatform(self):

Missing a test for version substitution.

>diff --git a/lib/python/release/platforms.py b/lib/python/release/platforms.py
> update_platform_map = {
...
>+    'macosx': ['Darwin_x86-gcc3-u-ppc-i386', 'Darwin_ppc-gcc3-u-ppc-i386'],
>+    'macosx64': ['Darwin_x86-gcc3-u-i386-x86_64', 'Darwin_x86_64-gcc3-u-i386-x86_64'],

I think we can drop PPC support.
 
>+# These FTP -> other mappings are provided so that things interpreting patcher
>+# configs can figure update/bouncer platforms without using the Buildbot
>+# platform as an intermediary.

Will just need to make sure we keep these in sync carefully.

>diff --git a/lib/python/release/updates/patcher.py b/lib/python/release/updates/patcher.py
>+def substitutePath(path, platform='UNDEFINED', locale='UNDEFINED', version='UNDEFINED'):
>+    """Returns a platform/locale specific path based on a generic one."""
>+    bouncerPlatform = ftp2bouncer(platform)
>+    path = path.replace('%platform%', platform)
>+    path = path.replace('%locale%', locale)
>+    path = path.replace('%version%', version)
>+    return path.replace('%bouncer-platform%', bouncerPlatform)

This looked a little odd to me too. All your callers provide all three variables, but if you want to be general you could do this sort of thing:
>+def substitutePath(path, platform=None, locale=None, version=None):
>+    """Returns a platform/locale specific path based on a generic one."""
>+    bouncerPlatform = ftp2bouncer(platform)
>+    if %platform% in path and not platform:
>+        raise SubstitutionError("No value given to substitute for %platform% in %s" % path)
>+    else:
>+        path = path.replace('%platform%', platform)


>+    def getUrl(self, version, platform, locale, type_, channel):
>+        """Figures out what the URL to a specific update. ...

Comment needs a tweak to parse.

>+        # Parse the config, with the heavy lifiting in helper methods.

Typo: lifiting

>+    def parseCurrentUpdate(self, currentUpdate):
...
>+            elif node.name in ('complete', 'partial', 'partials'):

I'd suggest leaving out 'partial' if it's not used in all this pythony goodness.

>+    def parseRelease(self, release):
>+        r = {}
>+        for node in release:
>+            if node.name in release:
>+                raise PatcherConfigError("Found multiple entries for '%s' in release section" % node.name)

Looks like this should be 
>+            if node.name in r:
Missing a test that would catch this ?

>diff --git a/lib/python/release/updates/snippets.py b/lib/python/release/updates/snippets.py
>+SCHEMA_2_OPTIONAL_ATTRIBUTES = (
>+    'showPrompt', 'showNeverForVersion', 'showSurvey', 'actions', 'licenseUrl',
>+    'billboardURL', 'openURL', 'notificationURL', 'alertURL',
>+)

detailsUrl is optional too, but we always specify it. I could either way on including it here.

>diff --git a/lib/python/vendor/apache_conf_parser.py b/lib/python/vendor/apache_conf_parser.py

Skipped reading this. Please pull in your license commit from github.

>diff --git a/scripts/updates/create-snippets.py b/scripts/updates/create-snippets.py
...
>+    for fromVersion, platform, locale, updateTypes, channels in pc.getUpdatePaths():

Swapping channels with updateTypes would give something that mimics the snippets dirs, if you're feeling aesthetic.

>+        # We use both an on-disk and in-memory cache of checksums to speed up
>+        # the snippet creation process. In order, we try to:

I wondered if this caching code should be put into a separate function to help with readability in the loop. Also I agree with rail that some sort of error handling/retry would be good on the http requests (the validation on checksum I mentioned at the top is related too).

Comment 9

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #7)
> Comment on attachment 646696 [details] [diff] [review]
> necessary patcher config adjustments
> 
> >Index: mozEsr10-thunderbird-branch-patcher2.cfg
> >===================================================================
> >             <partial>
> >-                betatest-url   http://stage.mozilla.org/pub/mozilla.org/thunderbird/nightly/10.0.6esr-candidates/build1/update/%platform%/%locale%/thunderbird-10.0.5esr-10.0.6esr.partial.mar
> >-                esrtest-url   http://stage.mozilla.org/pub/mozilla.org/thunderbird/nightly/10.0.6esr-candidates/build1/update/%platform%/%locale%/thunderbird-10.0.5esr-10.0.6esr.partial.mar
> >-                path   thunderbird/nightly/10.0.6esr-candidates/build1/update/%platform%/%locale%/thunderbird-10.0.5esr-10.0.6esr.partial.mar
> >-                url   http://download.mozilla.org/?product=thunderbird-10.0.6esr-partial-10.0.5esr&os=%bouncer-platform%&lang=%locale%
> >+                betatest-url   http://stage.mozilla.org/pub/mozilla.org/thunderbird/nightly/15.0b1-candidates/build1/update/%platform%/%locale%/thunderbird-14.0b5-15.0b1.partial.mar
> >+                esrtest-url   http://stage.mozilla.org/pub/mozilla.org/thunderbird/nightly/15.0b1-candidates/build1/update/%platform%/%locale%/thunderbird-14.0b5-15.0b1.partial.mar
> >+                path   thunderbird/nightly/15.0b1-candidates/build1/update/%platform%/%locale%/thunderbird-14.0b5-15.0b1.partial.mar
> >+                url   http://download.mozilla.org/?product=thunderbird-15.0b1-partial-14.0b5&os=%bouncer-platform%&lang=%locale%
> >             </partial>
> 
> r+ if you leave this out, and the new config bumper is going to wholesale
> replace <partials> blocks.

OK, I'll drop that hunk. The current patch for the bumper (https://bug775535.bugzilla.mozilla.org/attachment.cgi?id=645435) fully overrides the previous contents. It bases them on the <partial> block right now though...which we may have to change.

Comment 10

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #8)
> First off, I agree this is very nicely written code! Reducing 3050 lines of
> perl down to 460 lines of python (950 if you count the config parser) is a
> huge win. Mostly I'm suggesting some improvements.

Thanks!

> General question - will we tag the tools repo with an UPDATE_PACKAGING_Rn
> tag (and use that in the buildbot/scriptFactory) to allow for needing to
> change this code for different branches ? Perhaps we deal what if the need
> arises.

I hadn't thought about it, to be honest. Without doing anything, we'll end up using whatever the _RELEASE tag is at, so that could potentially be something we deal with by hand when setting up the release (or maybe as part of bug 748763), since that tagging is done pre-automation. I can dig into this more deeply if you think it needs to be addressed as part of the overall multiple partials project, but otherwise I'll defer it until later.

> >+        # Same goes for the hash.
> >+        elif fileInfo[file_]['hashes'].get(type_, hash_) != hash_:
> 
> The fallback to hash_ here is sneaky!

Hmmm, I see what you mean. I can make it more explicit as this if it'd be clearer:
elif type_ in fileInfo[file_]['hashes'] and fileInfo[file_]['hashes'][type_] != hash_:
  ...


> diff --git
> a/lib/python/mozilla_buildtools/test/test_release_updates_patcher.py
> b/lib/python/mozilla_buildtools/test/test_release_updates_patcher.py
> >+    def testParseCurrentUpdate(self):
> >+        dom = self._parse(goodCurrentUpdateSection)
> >+        expected = {
> >+            'channel': ['release'],
> 
> I had a style question - in some cases you have expected objects close to
> config files, and in others (like here) they're separated. Any reason for
> that ? If not it makes sense to put them together when the length is over
> some threshold.

It sounds like you're talking about readXml tests vs. tests for methods that read subsections. If that's the case, I didn't see any reason to have tests like this one look at an entire config object.

Or are you talking about relying on the global variables vs. expected objects being in the test methods?

> >+# These FTP -> other mappings are provided so that things interpreting patcher
> >+# configs can figure update/bouncer platforms without using the Buildbot
> >+# platform as an intermediary.
> 
> Will just need to make sure we keep these in sync carefully.

I'll see if I can add a test for that......

In the glorious Balrog future we can start using Buildbot platforms in whatever config we have feeding those client scripts, so we can eventually get rid of this.


> >+    def parseCurrentUpdate(self, currentUpdate):
> ...
> >+            elif node.name in ('complete', 'partial', 'partials'):
> 
> I'd suggest leaving out 'partial' if it's not used in all this pythony
> goodness.

Hmmmm, yes, that's a good point.

> >diff --git a/lib/python/release/updates/snippets.py b/lib/python/release/updates/snippets.py
> >+SCHEMA_2_OPTIONAL_ATTRIBUTES = (
> >+    'showPrompt', 'showNeverForVersion', 'showSurvey', 'actions', 'licenseUrl',
> >+    'billboardURL', 'openURL', 'notificationURL', 'alertURL',
> >+)
> 
> detailsUrl is optional too, but we always specify it. I could either way on
> including it here.

What's the expected behaviour when detailsUrl is missing? What gets set in the snippet?

> >diff --git a/scripts/updates/create-snippets.py b/scripts/updates/create-snippets.py
> ...
> >+    for fromVersion, platform, locale, updateTypes, channels in pc.getUpdatePaths():
> 
> Swapping channels with updateTypes would give something that mimics the
> snippets dirs, if you're feeling aesthetic.

=)

Comment 11

[[e-mail] (:bhearsum)]

Attachment: address review comments

OK, this patch should address all of the review comments. The actual changes compared to the last patch can be found in:
https://github.com/bhearsum/tools/commit/87c72f756592c1c08df04a16b01fffadd2b2f813
and
https://github.com/bhearsum/tools/commit/255c893edd34e560499f61538ff991a5a65cbd18

...if that makes it any easier for you to review.

However...

> > diff --git
> > a/lib/python/mozilla_buildtools/test/test_release_updates_patcher.py
> > b/lib/python/mozilla_buildtools/test/test_release_updates_patcher.py
> > >+    def testParseCurrentUpdate(self):
> > >+        dom = self._parse(goodCurrentUpdateSection)
> > >+        expected = {
> > >+            'channel': ['release'],
> > 
> > I had a style question - in some cases you have expected objects close to
> > config files, and in others (like here) they're separated. Any reason for
> > that ? If not it makes sense to put them together when the length is over
> > some threshold.
> 
> It sounds like you're talking about readXml tests vs. tests for methods that
> read subsections. If that's the case, I didn't see any reason to have tests
> like this one look at an entire config object.
> 
> Or are you talking about relying on the global variables vs. expected
> objects being in the test methods?

I'm 99% sure you meant the latter, so I went ahead and moved everything into the methods. They were originally global because I thought they might get used more than once.

> > >+# These FTP -> other mappings are provided so that things interpreting patcher
> > >+# configs can figure update/bouncer platforms without using the Buildbot
> > >+# platform as an intermediary.
> > 
> > Will just need to make sure we keep these in sync carefully.
> 
> I'll see if I can add a test for that......
> 
> In the glorious Balrog future we can start using Buildbot platforms in
> whatever config we have feeding those client scripts, so we can eventually
> get rid of this.

I couldn't figure out how to test anything here. I think ftp -> update is OK as it is, because that just references the buildbot -> update. I couldn't figure out how to test ftp -> bouncer, so we'll just have to be prudent there.

> > >diff --git a/lib/python/release/updates/snippets.py b/lib/python/release/updates/snippets.py
> > >+SCHEMA_2_OPTIONAL_ATTRIBUTES = (
> > >+    'showPrompt', 'showNeverForVersion', 'showSurvey', 'actions', 'licenseUrl',
> > >+    'billboardURL', 'openURL', 'notificationURL', 'alertURL',
> > >+)
> > 
> > detailsUrl is optional too, but we always specify it. I could either way on
> > including it here.
> 
> What's the expected behaviour when detailsUrl is missing? What gets set in
> the snippet?

I didn't address this yet, because I'm not sure what to do.

Comment 12

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

(In reply to Ben Hearsum [:bhearsum] from comment #11)
> I'm 99% sure you meant the latter, so I went ahead and moved everything into
> the methods. They were originally global because I thought they might get
> used more than once.

I meant all global or all local, so that the two were close to each other in the file. If they're not shared then local sounds good.

> > > detailsUrl is optional too, but we always specify it. I could either way on
> > > including it here.
> > 
> > What's the expected behaviour when detailsUrl is missing? What gets set in
> > the snippet?

The line would just get left out of the snippet, AUS2 doesn't set it in the XML, and the app falls back to default pref.
 
> I didn't address this yet, because I'm not sure what to do.

Lets leave it as it is. No point spending time on making something optional if we always use it, and are planning to obsolete with Balrog.

Comment 13

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

Comment on attachment 651008 [details] [diff] [review]
address review comments

Only minor nits for fix on checkin.

>diff --git a/.hgtags b/.hgtags

There's a conflict in this to squash.

>diff --git a/lib/python/release/updates/patcher.py b/lib/python/release/updates/patcher.py
>+def substitutePath(path, platform=None, locale=None, version=None):
...
>+    for sub,replacement in subs.items():
>+        print sub, replacement, path

Please squash the print.

>+    def getUrl(self, version, platform, locale, type_, channel):
>+        """Returns the the final URL to a specific update. Completes come from
>+           the <complete>. Partials come from their version-specific block

Uh uh, what what the the ... ... ? ? :-) :-)

>+        # Make sure we have a current-update
>+        if not self['current-update']:
>+            raise PatcherConfigError("Required section current-update is empty")

This should happen before checking the required nodes exist.

Comment 14

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #13)
> >+    def getUrl(self, version, platform, locale, type_, channel):
> >+        """Returns the the final URL to a specific update. Completes come from
> >+           the <complete>. Partials come from their version-specific block
> 
> Uh uh, what what the the ... ... ? ? :-) :-)

Sorry, can you expand on this? I could say "URLs for completes come from the <complete> block. URLs for partials come from their version-specific block...", but I'm not sure if that will address this.

Comment 15

[[e-mail] (:bhearsum)]

Comment on attachment 646696 [details] [diff] [review]
necessary patcher config adjustments

With patcher-configs moving to build/tools, I moved this patch to the build/tools patch in bug 773290.

Comment 16

[Phil Ringnalda (:philor)]

(In reply to Ben Hearsum [:bhearsum] from comment #14)
> (In reply to Nick Thomas [:nthomas] from comment #13)
> > >+    def getUrl(self, version, platform, locale, type_, channel):
> > >+        """Returns the the final URL to a specific update. Completes come from
> > >+           the <complete>. Partials come from their version-specific block
> > 
> > Uh uh, what what the the ... ... ? ? :-) :-)
> 
> Sorry, can you expand on this? I could say "URLs for completes come from the
> <complete> block. URLs for partials come from their version-specific
> block...", but I'm not sure if that will address this.

I think (since it's the same sort of thing I would do) that he was just amusing himself while pointing out that you said "Returns the the".

Comment 17

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

Yes, just a philor-esque joke about the repeated 'the'.

Comment 18

[Rail Aliiev [:rail]]

Comment on attachment 651008 [details] [diff] [review]
address review comments

       _~
    _~ )_)_~
    )_))_))_)
    _!__!__!_
    \______t/
  ~~~~~~~~~~~~~ it!

Comment 19

[[e-mail] (:bhearsum)]

Attachment: address final review comments

This just addresses the final review comments. The metadiff is:
 diff --git a/lib/python/build/checksums.py b/lib/python/build/checksums.py
 new file mode 100644
 index 0000000..01b1c5a
@@ -906,7 +888,7 @@
          if len(entry)>1:
 diff --git a/lib/python/release/updates/patcher.py b/lib/python/release/updates/patcher.py
 new file mode 100644
-index 0000000..5fdedd8
+index 0000000..441ccf9
 --- /dev/null
 +++ b/lib/python/release/updates/patcher.py
 @@ -0,0 +1,272 @@
@@ -928,7 +910,6 @@
 +        'bouncer-platform': ftp2bouncer(platform)
 +    }
 +    for sub,replacement in subs.items():
-+        print sub, replacement, path
 +        if '%%%s%%' % sub in path:
 +            if replacement == None:
 +                raise TypeError("No substitution provided for '%s'" % sub)
@@ -974,8 +955,8 @@
 +        return substitutePath(path, platform, locale, version)
 +
 +    def getUrl(self, version, platform, locale, type_, channel):
-+        """Returns the the final URL to a specific update. Completes come from
-+           the <complete>. Partials come from their version-specific block
++        """Returns the final URL to a specific update. Completes come from
++           the <complete> block. Partials come from their version-specific block
 +           inside of <partials>. In both cases if a [channel]-url exists, it
 +           will be returned. If it doesn't exist the generic url will be
 +           returned. If neither exists a PatcherConfigError will be raised.
@@ -1084,6 +1065,10 @@
 +                    self.addRelease(releaseNode.name, self.parseRelease(releaseNode.body.nodes))
 +
 +        # Now, a bunch of verifications
++        # Make sure we have a current-update
++        if not self['current-update']:
++            raise PatcherConfigError("Required section current-update is empty")
++        # Make sure required nodes exist in <current-update> and <release> nodes.
 +        for node in ('channel', 'testchannel', 'complete', 'details', 'from', 'to'):
 +            if node not in self['current-update']:
 +                raise PatcherConfigError("Required node '%s' not found in current-update" % node)
@@ -1091,9 +1076,6 @@
 +            for node in ('locales', 'version', 'platforms'):
 +                if node not in releaseNode:
 +                    raise PatcherConfigError("Required node '%s' not found in release node '%s'" % (node, version))
-+        # Make sure we have a current-update
-+        if not self['current-update']:
-+            raise PatcherConfigError("Required section current-update is empty")
 +        # Make sure that versions mentioned have a release block.
 +        if self['current-update']['to'] not in self['release']:
 +            raise PatcherConfigError("No release found for version '%s'" % self['current-update']['to'])

Carrying forward r+.

Comment 20

[[e-mail] (:bhearsum)]

Worked fine in Firefox 15.0b6/Thunderbird 15.0b5.