575400 - tagging scripts should be more robust

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P3)

Description

[Aki Sasaki (not active)]

I couldn't find this bug, but this is something I'd like as I ran into this several times in Fennec 1.1 ( https://wiki.mozilla.org/Releases/Fennec_1.1/BuildNotes )

o hg.m.o timeouts should retry
o There should be an option to create the relbranch if it doesn't exist in build >1...
  o This caused an issue in a previous Fennec release when we used an existing relbranch on m-1.9.2, but needed to create a relbranch on mobile-browser
  o This caused an issue in Fennec 1.1, when a new locale was added in build 3, and therefore didn't have a relbranch already.

It's not difficult to manually create the branch, check in, comment out the locales that have already been tagged, reconfig, force the tag build, revert the locales changes, add a dummy tag step, reconfig, and restart the automation.  However, I think making the tag step more robust would be an improvement.

Comment 1

[[e-mail] (:bhearsum)]

Updating summary to match the new world

Comment 2

[[e-mail] (:bhearsum)]

Planning to look at this this quarter.

Comment 4

[[e-mail] (:bhearsum)]

Attachment: enhance tagging scripts to support multiple source repositories

Here's a mostly tested patch which I believe meets all the requirements. It's probably not going to be the final patch, but I'd appreciate some feedback before doing more robust testing. Here's a summary of the changes:
- Replace sourceRepo* with new sourceRepositories dict; l10n remains separate because it doesn't have the same attributes as these.
- Completely isolates build number from relbranch from bump files. There is not more implied logic between the three.
- Creates relbranches whenever needed.
- Bumps files in the relbranch/default whenever "bumpFiles" is set for a repository.
- Relbranches per source repository and set of l10n repositories.

I'm pretty happy with the patch overall, it gets rid of some ickyness like isRelbranchGenerated, and the generalization of the source repositories is a good thing IMHO. I'm not super happy about the way bumpFiles works now -- you have to remove them in any situation you don't want bumping to happen (for example, build2), and re-add them when you *do* want it to happen again. The fact that we need to bump the milestone.txt files for some mobile releases pushed me in this direction. That situation makes it really difficult to hardcode or otherwise intelligently determine which files to bump from within the script. bug 627308 makes this a lot less painful.

There is one thing that this patch explicitly does not handle: using one "tagging" run for releases of both products. I believe that this should be done if we look at chaining mobile/desktop automation together. We might be doing that in bug 619550, but the more I think about it, the more it seems unlikely to me that we'll be integrating them to that degree soon. Because of that, I've added a check in validate() that ensures we don't try to bump versions in multiple sourceRepositories in one run, because that will end with one repository getting that the wrong version.

Aki, just to avoid confusion: The milestone stuff I was talking about earlier is from bug 607372. Whatever happens with that doesn't affect what we do here AFAIK.

Sample sourceRepositories {} are coming is a separate attachment.

Comment 5

[[e-mail] (:bhearsum)]

Attachment: sample tagging config snippets

I've only included relevant variables in each section.

Comment 6

[Aki Sasaki (not active)]

Comment on attachment 508558 [details] [diff] [review]
enhance tagging scripts to support multiple source repositories

I think this is the right approach.
I also moved to dictionaries of repositories, but lists of dictionaries since I needed to pull in a specific order.

I'm almost leaning towards keeping bumpFiles relatively static, and having a boolean flag saying whether to bump the bumpFiles list, since otherwise we'd need to delete the list every build 2 and recreate it every build 1.  I suppose we can fix that by either commenting lines or having the list defined above the dictionaries.

Comment 7

[[e-mail] (:bhearsum)]

Thanks for the quick feedback Aki!

(In reply to comment #6)
> Comment on attachment 508558 [details] [diff] [review]
> enhance tagging scripts to support multiple source repositories
> 
> I think this is the right approach.
> I also moved to dictionaries of repositories, but lists of dictionaries since I
> needed to pull in a specific order.

I hadn't consider this. I did choose to use static names for the dictionary keys, though: "mozilla" always means "gecko repository" whether it's mozilla-1.9.2, mozilla-central, or whatever. Same for mobile. Doing so should enable us to build both products with the same config later, if we want.

> I'm almost leaning towards keeping bumpFiles relatively static, and having a
> boolean flag saying whether to bump the bumpFiles list, since otherwise we'd
> need to delete the list every build 2 and recreate it every build 1.  I suppose
> we can fix that by either commenting lines or having the list defined above the
> dictionaries.

Yeah, I'm not super pleased with the current way either. Did you see this part of my previous comment, though?:
> The fact that we need to bump the milestone.txt files for some mobile releases
> pushed me in this direction. That situation makes it really difficult to
> hardcode or otherwise intelligently determine which files to bump from within
> the script. bug 627308 makes this a lot less painful.

If you've got an idea about how to make bump files work for this and the "normal" scenarios I'm happy to hear it!

Comment 8

[Rail Aliiev [:rail]]

Comment on attachment 508558 [details] [diff] [review]
enhance tagging scripts to support multiple source repositories

> I'm not super happy about the way bumpFiles works now

100% agree with you.

(In reply to comment #7)
> If you've got an idea about how to make bump files work for this and the
> "normal" scenarios I'm happy to hear it!

What if you don't _bump_ these files at all?
You could _set_ the versions using releaseConfig values.

Instead of 

'bumpFiles': ['browser/config/version.txt', 'config/milestone.txt', 'js/src/config/milestone.txt']

you could use something like this:

'bumpFiles': [
               {'file': 'browser/config/version.txt', 'version': releaseConfig['version']}, 
               {'file': 'config/milestone.txt', 'version': releaseConfig['milestone']}, 
               {'file': 'js/src/config/milestone.txt', 'version': releaseConfig['milestone']}
             ]
and iterate over the list setting the version using explicit versions instead of bumping. In this case for builds > 1 you'll get the same values there as in build1.

*thumbs up*

Comment 9

[[e-mail] (:bhearsum)]

That's an interesting idea, Rail. I'm not 100% sure how I feel about because it feels a bit heavy handed, but it could end up being a fix for bug 630871.

How do you propose making it work for both the default and relbranch?

Anyone else have thoughts on it?

Comment 10

[Rail Aliiev [:rail]]

A summary from IRC:

* We already set versions explicitly instead of bumping them, so ignore my proposal. bump() doesn't alter the same files for builds > 1.

* Probably we'll need a conditional commit on default branch. hg commit fails if there are no changes in the repository, which is true for builds > 1.

Comment 11

[[e-mail] (:bhearsum)]

We talked about this a bunch more on IRC today and decide on the following format:
releaseConfig['productName']         = 'firefox'
releaseConfig['appName']             = 'browser'
releaseConfig['version']             = '4.0b21'
releaseConfig['appVersion']          = releaseConfig['version']
releaseConfig['nextAppVersion']      = '4.0b22pre'
releaseConfig['milestone']           = '2.0b21
releaseConfig['nextMilestone']       = '2.0b22pre'
releaseConfig['buildNumber']         = 1
releaseConfig['baseTag']             = 'FIREFOX_4_0b21'
releaseConfig['sourceRepositories']  = {
    'mozilla': {
        'name': 'mozilla-central',
        'path': 'users/bhearsum_mozilla.com/mozilla-central',
        'revision': 'c134bec8b7f0',
        'relbranch': None,
        'bumpFiles': {
            'browser/config/version.txt': {
                 'version': releaseConfig['appVersion'],
                 'nextVersion': releaseConfig['nextAppVersion']
            }
            'config/milestone.txt': {
                 'version': releaseConfig['milestone'],
                 'nextVersion': releaseConfig['nextMilestone']
            }
            'js/src/config/milestone.txt': {
                 'version': releaseConfig['milestone'],
                 'nextVersion': releaseConfig['nextMilestone']
            }
        }
    },
}

Pros in favour of this are:
- Makes it possible to bump default branch to whatever we want, removing any need to bump manually for a -> b or b -> rc (also fixes bug 630871)
- Gives us an explicit history of what the automation bumped the next version too, in version control & tagged

We thought about using nextVersion() here to reduce the amount of manual tweaking needed in the release config, that was shot down for two reasons:
- We decided that importing things into config files is uncouth
- The upcoming release config bumper (bug 627308) takes away this pain

Comment 12

[[e-mail] (:bhearsum)]

Oh, and thanks to everyone who contributed here! I'm really happy with the outcome, and confident that we'll be able to handle all known cases once this bug is fixed.

Comment 13

[[e-mail] (:bhearsum)]

Attachment: take 1

This isn't ready for review yet but I figured I'd post it in case anyone is curious. This seems to work for sourceRepositories, but manages not to do any tagging on l10n repos.

Comment 14

[[e-mail] (:bhearsum)]

Attachment: versatile tagging, take 2

This patch has been tested pretty well and to the best of my knowledge, addresses all of the things we discussed with regards to relbranches, versions, and bumping files. A few notes:
- bumpFile() works for all version files that we care about, but doesn't append newlines to the end. I chose not to because the milestone.txt/version.txt files do not normally have one, and I fear breaking consumers of them. confvars.sh *does* at this time, and will lose it after the first bump with this. Being a shell script, I don't think this is an issue.
- nextVersion() is unused at the moment, but I expect bug 627308 or a follow-up to start using it.
- I'm not *super* happy about the third arg to bump(), but I couldn't come up with a better idea that kept bump() working for relbranch + nightly version bumping.

I still need to address the points about retrying things more and not dieing when hg is down or we hit network issues, but in the meantime, feedback on this part would be useful.

Comment 15

[Aki Sasaki (not active)]

Comment on attachment 510353 [details] [diff] [review]
versatile tagging, take 2

> - nextVersion() is unused at the moment, but I expect bug 627308 or a follow-up
> to start using it.

nextVersion() is still right most of the time, and can result in bad things accidentally happening the other percentage of the time.

I'd rather either have an automated solution that is right all of the [foreseeable] time, or manually entering the next version for bug 627308.  So I would lean towards ripping this code out.

(However, if bug 627308 creates config files, and those diffs are then human-parsed for review, that would probably be ok.)

> - bumpFile() works for all version files that we care about, but doesn't append
> newlines to the end.

Hm, is there a good way for us to preserve the existence/lack of newlines per file?  I was able to do so in perl, not sure if there's a comparable fh.readline that preserves line endings that we can regex against.

+# Regex that matches all possible versions and milestones
+ANY_VERSION_REGEX =\
+    ('\d\.\d[\d\.]*'     # A version number
+    '([a-zA-Z]+\d+)?'    # Might be a project branch

Not sure what this project branch regex is supposed to match -- do you have
examples?

Comment 16

[[e-mail] (:bhearsum)]

Thanks for the quick feedback, Aki.

(In reply to comment #15)
> > - nextVersion() is unused at the moment, but I expect bug 627308 or a follow-up
> > to start using it.
> 
> nextVersion() is still right most of the time, and can result in bad things
> accidentally happening the other percentage of the time.
> 
> I'd rather either have an automated solution that is right all of the
> [foreseeable] time, or manually entering the next version for bug 627308.  So I
> would lean towards ripping this code out.
> 
> (However, if bug 627308 creates config files, and those diffs are then
> human-parsed for review, that would probably be ok.)

Those configs will definitely be human reviewed for the forseeable future. The way I look at it is that the tool will default to nextVersion() when bumping versions, but allow explicit overrides.

> > - bumpFile() works for all version files that we care about, but doesn't append
> > newlines to the end.
> 
> Hm, is there a good way for us to preserve the existence/lack of newlines per
> file?  I was able to do so in perl, not sure if there's a comparable
> fh.readline that preserves line endings that we can regex against.

I'm sure I can make it happen if it's important, I just didn't want to spend time on it if it wasn't.

> +# Regex that matches all possible versions and milestones
> +ANY_VERSION_REGEX =\
> +    ('\d\.\d[\d\.]*'     # A version number
> +    '([a-zA-Z]+\d+)?'    # Might be a project branch
> 
> Not sure what this project branch regex is supposed to match -- do you have
> examples?

3.6.4plugin1 -- aka Lorentz :)

Comment 17

[[e-mail] (:bhearsum)]

Attachment: take 4, with retry support

New in this version:
- Preserve trailing newlines when bumping files
- Added util/retry.py w/ tests + switching buildfarm/utils/retry.py to it
- Use retry() for all network based operations

A few notes:
- I chose not to use retry() anywhere directly in util/hg.py because I didn't think that it should be hardcoded.
- I chose not to use retry() for local disk operations because I couldn't think of any scenario in which they would fail in a way that we could recover from: out of disk space, invalid syntax calling hg, etc. (The local operations that happen in apply_and_push and bump_and_tag/tagRepo are wrapped in retry() because apply_and_push as a whole is).
- I tested this by going through all the scenarios I listed in the earlier attachment while running "while sleep 60; do date && killall -r '.*hg.*'; done" on the slave. In every case except one, they successfully tagged all repositories. Going through the logs, I saw that 'push', 'out', and 'pull' were all killed at one point during my tests. The one that didn't recover failed in the middle of "hg tag", after it had modified the working copy, but before committing it. Because of that, "hg strip" failed to tidy things up and that Exception made it all the way up. I don't know of a way we could get into that situation in real life.

I'm pretty sure this is ready to go from the tag-release.py side, I still need to make sure buildfarm/utils/retry.py works as well as before, so feedback? only for now.

Comment 18

[[e-mail] (:bhearsum)]

Oh, and I've left my master running with all of the tagging logs, feel free to have a look if you want. Builds 3 and up are relevant: http://staging-master.build.mozilla.org:8018/builders/release-mozilla-central-tag/builds/3.

Comment 19

[Aki Sasaki (not active)]

I looked at the logs and the comment more than the code this round. I can take a closer look at the code if you want.

Comments: the log is huge, but very very readable; thumbs up!
I know you were unhappy you had a failing situation, but I wanted to see failing situation behavior, so build 5 was good that way.  Summarizing which locale(s) failed at the end is great.  There's probably ways we can make that specific failure more visible, but that can wait for future enhancement.

... I'm wondering how we recover from such a failure irl; log into the slave and run the commands from the log that failed after fixing root cause?

I do see the "error updating" messages.  Later runs have "Killed!" and earlier runs have no output -- was this "Killed!" message added somewhere in the middle, or do we have situations where we don't have output?

Awesome work Ben!

Comment 20

[[e-mail] (:bhearsum)]

(In reply to comment #19)
> I looked at the logs and the comment more than the code this round. I can take
> a closer look at the code if you want.
> 
> Comments: the log is huge, but very very readable; thumbs up!
> I know you were unhappy you had a failing situation, but I wanted to see
> failing situation behavior, so build 5 was good that way.  Summarizing which
> locale(s) failed at the end is great.  There's probably ways we can make that
> specific failure more visible, but that can wait for future enhancement.
> 
> ... I'm wondering how we recover from such a failure irl; log into the slave
> and run the commands from the log that failed after fixing root cause?

It depends. Here's a few scenarios:
If one of the sourceRepositories fails to tag the script bails immediately and doesn't attempt l10n or other repositories. If you've only got one sourceRepository or if you have multiple and none of them tag properly you can just restart the automation from scratch after fixing the root problem. If you have multiple and one or more succeeds while others fail, your best course of action at this time is to run the rest by hand on the slave, commenting out those that already succeeded.
If an l10n or otherRepoToTag fails you're in the same boat as latter scenario above.

I haven't thought too much about how to skip or disable repositories on the fly yet to make any of the "some repositories failed to tag" situations better. These are pretty rare already, and even more so once this bug is fixed.

Sorry for the long windedness above, is that helpful?

> I do see the "error updating" messages.  Later runs have "Killed!" and earlier
> runs have no output -- was this "Killed!" message added somewhere in the
> middle, or do we have situations where we don't have output?

"Killed!" is actually coming from Mercurial, which seems to print that when it receives SIGTERM. Looking over earlier logs, Builds 3 & 4 must've been before I started killing hg in a loop. The "error updating" messages you see in #4 don't seem to be related to hg being killed, but rather something being wrong with the shared repository. I'm not exactly sure why though, given that the repositories in question successfully updated from the shared repository in Build #3. I'll do some more digging and see if I can figure out what happened.

> Awesome work Ben!

Thanks!

Comment 21

[[e-mail] (:bhearsum)]

Attachment: final patch, set sys.path in retry.py, fully tested

I tested the utils/retry.py changes in staging today, had to fix up the sys.path entries to make it work. Other than that, this is the same as the previous patch.

Comment 22

[[e-mail] (:bhearsum)]

Attachment: tools patch, updated to adjust everything else for sourceRepositories

Aki, I haven't changed any of the tag-release.py logic so I won't bother you with this one.

This patch is additive compared to the previous one, it adjusts all of the tools that depended on sourceRepo*. This, combined with the buildbotcustom and buildbot-configs patch that I'm about to post, went through a full staging run of 3.5.18 and of 3.6.15. A couple of minor issues were fixed along the way but otherwise it went well. The staging runs did all of the normal stuff (tag -> update verify) as well as pre_push_checks, push_to_mirrors, and major update + verify.

I don't intend to land this until after b12, because I promised Nick I wouldn't invalidate his staging run.

Comment 23

[[e-mail] (:bhearsum)]

Attachment: buildbotcustom adjustments

Comment 24

[[e-mail] (:bhearsum)]

Attachment: configs updates

This is going to bitrot because of Beta 12, but most of it will stay valid.

Comment 25

[[e-mail] (:bhearsum)]

Before I forget: Catlee mentioned that I erroneously removed the sourceRepo from clone_repositories in the release.py patch. Need to fix that.

Comment 26

[Chris AtLee [:catlee]]

Comment on attachment 512546 [details] [diff] [review]
tools patch, updated to adjust everything else for sourceRepositories

>-def compareVersion(fileHandle, versionNumber):
>+def compareVersion(contents, versionNumber):
>     """Given an open readable file-handle look for the occurrence
>        of the version # in the file"""
>-    log.info("looking for %s in %s" % (versionNumber, fileHandle.geturl()))
>-    ret = re.search(re.compile(re.escape(versionNumber), re.DOTALL), fileHandle.read())
>-    if ret:
>-        log.info("Found a match!")
>+    ret = re.search(re.compile(re.escape(versionNumber), re.DOTALL), contents)
>     return ret

The name of this function, 'compareVersion' isn't really applicable to what it does..possible to rename? findVersion maybe?

>-def verify_build(branch, revision, hghost, version, milestone):
>+def verify_build(sourceRepo, hghost):
>     """Pull down the version.txt, milestone.txt and js/src/config/milestone.txt
>        and make sure it matches the release configs"""

Update docstring now that we're not hardcoding these files?

>     except KeyboardInterrupt:
>         sys.exit(1)
>+    except Exception, e:
>+        log.info("Unable to successfully run %s after %d attempts" % \
>+          (args, options.retries))
>+        # If we caught a RunWithTimeoutException we can exit with the same
>+        # rc as the command. If something else was hit, just exit with 1
>+        rc = getattr(e, 'rc', 1)
>+        sys.exit(1)

I think this is busted. If we don't get the output we expect (via the stdout/stderr_regexp), but the command succeeds, then the rc of the exception will be 0.

>diff --git a/lib/python/build/versions.py b/lib/python/build/versions.py
>index 5d43880..4b27025 100644
>--- a/lib/python/build/versions.py
>+++ b/lib/python/build/versions.py
>@@ -1,9 +1,56 @@
> import re
> 
>+class BuildVersionsException(Exception):
>+    pass
>+
>+# Versions that match this should not be bumped
>+DO_NOT_BUMP_REGEX = '^\d\.\d(pre)?$'
>+
>+# Regex that matches all possible versions and milestones
>+ANY_VERSION_REGEX =\
>+    ('\d\.\d[\d\.]*'     # A version number
>+    '([a-zA-Z]+\d+)?'    # Might be a project branch
>+    '((a|b)\d+)?'        # Might be an alpha or beta
>+    '(pre)?')            # Might be a 'pre' (nightly) version
>+
>+BUMP_FILES = {
>+    '^.*(version.*\.txt|milestone.txt)$': '^%(version)s$',
>+    '^(default-version.txt|confvars.sh)$': '^MOZ_APP_VERSION=%(version)s$'
>+}

does it make sense to put these regexes in the release config where the list of bump files are?

Comment 27

[[e-mail] (:bhearsum)]

Comment on attachment 512547 [details] [diff] [review]
buildbotcustom adjustments

This patch needs a couple of fixes, see earlier comments.

Comment 29

[[e-mail] (:bhearsum)]

(In reply to comment #26)
> >         sys.exit(1)
> >+    except Exception, e:
> >+        log.info("Unable to successfully run %s after %d attempts" % \
> >+          (args, options.retries))
> >+        # If we caught a RunWithTimeoutException we can exit with the same
> >+        # rc as the command. If something else was hit, just exit with 1
> >+        rc = getattr(e, 'rc', 1)
> >+        sys.exit(1)
> 
> I think this is busted. If we don't get the output we expect (via the
> stdout/stderr_regexp), but the command succeeds, then the rc of the exception
> will be 0.

> >+BUMP_FILES = {
> >+    '^.*(version.*\.txt|milestone.txt)$': '^%(version)s$',
> >+    '^(default-version.txt|confvars.sh)$': '^MOZ_APP_VERSION=%(version)s$'
> >+}
> 
> does it make sense to put these regexes in the release config where the list of
> bump files are?

I'm not sure what purpose they would serve there, can you explain?

Comment 30

[[e-mail] (:bhearsum)]

Attachment: updated configs

Comment 31

[[e-mail] (:bhearsum)]

Attachment: latest buildbotcustom

Comment 32

[[e-mail] (:bhearsum)]

Attachment: tools patch, updated and comments addressed

(In reply to comment #26)
> Comment on attachment 512546 [details] [diff] [review]
> tools patch, updated to adjust everything else for sourceRepositories
> 
> >-def compareVersion(fileHandle, versionNumber):
> >+def compareVersion(contents, versionNumber):
> >     """Given an open readable file-handle look for the occurrence
> >        of the version # in the file"""
> >-    log.info("looking for %s in %s" % (versionNumber, fileHandle.geturl()))
> >-    ret = re.search(re.compile(re.escape(versionNumber), re.DOTALL), fileHandle.read())
> >-    if ret:
> >-        log.info("Found a match!")
> >+    ret = re.search(re.compile(re.escape(versionNumber), re.DOTALL), contents)
> >     return ret
> 
> The name of this function, 'compareVersion' isn't really applicable to what it
> does..possible to rename? findVersion maybe?

Done.

> >-def verify_build(branch, revision, hghost, version, milestone):
> >+def verify_build(sourceRepo, hghost):
> >     """Pull down the version.txt, milestone.txt and js/src/config/milestone.txt
> >        and make sure it matches the release configs"""
> 
> Update docstring now that we're not hardcoding these files?

Done.

> >     except KeyboardInterrupt:
> >         sys.exit(1)
> >+    except Exception, e:
> >+        log.info("Unable to successfully run %s after %d attempts" % \
> >+          (args, options.retries))
> >+        # If we caught a RunWithTimeoutException we can exit with the same
> >+        # rc as the command. If something else was hit, just exit with 1
> >+        rc = getattr(e, 'rc', 1)
> >+        sys.exit(1)
> 
> I think this is busted. If we don't get the output we expect (via the
> stdout/stderr_regexp), but the command succeeds, then the rc of the exception
> will be 0.

Fixed. run_with_timeout now sets the rc to "-1" if the command succeeds but the output isn't found, and this block exits with sys.exit(rc).

> >+BUMP_FILES = {
> >+    '^.*(version.*\.txt|milestone.txt)$': '^%(version)s$',
> >+    '^(default-version.txt|confvars.sh)$': '^MOZ_APP_VERSION=%(version)s$'
> >+}
> 
> does it make sense to put these regexes in the release config where the list of
> bump files are?

I _think_ you mean that these could move to the release configs and we could bump all files that match the glob. If that's the case, I don't think we should do it, because it's better to be explicit in the release config. I didn't make any changes related to this in this version of the patch.

Comment 33

[[e-mail] (:bhearsum)]

Comment on attachment 515120 [details] [diff] [review]
updated configs

Catlee is gone all this week, moving reviews to Rail.

Comment 34

[Rail Aliiev [:rail]]

Comment on attachment 515120 [details] [diff] [review]
updated configs

A nit. Remove trailing spaces after releaseConfig['l10nChunks']          = 6

Comment 35

[Rail Aliiev [:rail]]

Comment on attachment 515121 [details] [diff] [review]
latest buildbotcustom

Almost there! :)
You should remove the following lines:

-        releaseName = releasePrefix()
and
-        ftpURL = genericFtpUrl()

these variables are used in release email templates and passed there via locals().

r+ with those lines reverted.

Comment 36

[Rail Aliiev [:rail]]

Comment on attachment 515123 [details] [diff] [review]
tools patch, updated and comments addressed

+        if contents != newContents:
+            open(fileToBump, "w").write(newContents)

Please close the file handle here. It's safe to read w/o closing, but for 'w' mode it would be safer to close the handle explicitly.

+                if e.returncode != 1 or "nothing changed" not in e.output:
+                    raise

Using e.output is not safe here. get_output should add this attribute even if Popen raises an exception.

Comment 37

[[e-mail] (:bhearsum)]

Attachment: address review comments, tools patch

Going to test this with my 4.0rc1 staging run.

Comment 38

[[e-mail] (:bhearsum)]

Attachment: address review comments, buildbotcustom patch

Comment 39

[[e-mail] (:bhearsum)]

Attachment: tools patch w/ typo fixed

Comment 40

[[e-mail] (:bhearsum)]

No issues with these latest patches up to l10n repacks in my staging run.

Comment 41

[[e-mail] (:bhearsum)]

Comment on attachment 516042 [details] [diff] [review]
tools patch w/ typo fixed

Landed on default

Comment 42

[[e-mail] (:bhearsum)]

Comment on attachment 515987 [details] [diff] [review]
address review comments, buildbotcustom patch

Landed on default: changeset:   1563:fb81d35b7c14

Comment 43

[[e-mail] (:bhearsum)]

Comment on attachment 515120 [details] [diff] [review]
updated configs

Landed on default: changeset:   4091:6b897b72a2f1

Comment 44

[[e-mail] (:bhearsum)]

I updated the Documentation for this: https://wiki.mozilla.org/index.php?title=Release%3ARelease_Automation_on_Mercurial%3ADocumentation&action=historysubmit&diff=288031&oldid=286234

Comment 45

[Rail Aliiev [:rail]]

Attachment: Bustage fix

During 3.6.15 release cycle partner-repacks failed because the repo was untagged.

Comment 46

[Rail Aliiev [:rail]]

Comment on attachment 516561 [details] [diff] [review]
Bustage fix

http://hg.mozilla.org/build/buildbot-configs/rev/b556955c8450

Comment 47

[[e-mail] (:bhearsum)]

Last patch has been merged to production -> FIXED.

Comment 48

[Justin Wood (:Callek)]

Ben,

Probably not worth touching at the moment, but I would have expected that |sourceRepoKey="mozilla"| as defined in various places, could have just been keyed off releaseConfig['sourceRepoKey'] instead of having to be passed in at the various points, still defaulting to "mozilla" everywhere.

To me that would have simplified these codepaths a whole lot.

Comment 49

[[e-mail] (:bhearsum)]

Perhaps. However, unless other apps are able to fully make use of generateReleaseBranchObjects, it doesn't make a difference, does it?