There are places in our automation (like tagging the repository during a release), where the following can occur: - build machine clones rev N - developer pushes rev N+1 - build machine tries to push a different rev N+1 and fails because it would create a new head. At this point the build machine crashes and burns, and there is weeping and gnashing of teeth. The right thing to do would be to re-try a few times if this condition is detected. Then there would be much rejoicing!

I will rejoice in the FUTURE!

Catlee has been working on a ScriptFactory, which would allow us to easily move tagging to an external script. I can't find that bug, but I can give this bug a go once that's ready.

from irc: 10:46 <@catlee> so, for the tagging, I figured we'd have a script that we pass the hg repo path, and a revision to checkout 10:46 <@catlee> and I guess the name of the release config file 10:46 <@catlee> so it would clone buildbot-configs, update to the RELEASE tag, and then read the proper release_config file 10:47 <@catlee> so we'd need to tag buildbot-configs before starting the release 10:47 < bhearsum> hmmm, i wasn't originally thinking of going quite that far, i was just going to pass in the required parameters 10:48 <@catlee> ah 10:48 < bhearsum> might be a good opportunity to play around with that sort of thing, though 10:48 <@catlee> I figured that was the way we wanted to go in the end 10:49 < bhearsum> it's not going to let us do reconfig-less releases, but we've got to start somewhere 10:49 < bhearsum> yeah, you're right 10:49 <@catlee> it's one of the trickiest pieces

So, this is untested, definitely has some bugs, but I wanted to throw it up and get some general feedback. The process/steps are pretty similar to what happens in ReleaseTaggingFactory, modulo retrying failed pushes. I'm not sure whether or not this is the right place for it to live. If I follow the nanojit/fuzzing model, this should live elsewhere (build/tools/release, probably), and then be called by bash script? I'm not super pleased with how the config file is read. I don't know of a way to import it without hardcoding the name, though. I thought about execfile(), but that will dump all of the variables into the global namespace, which isn't particularly nice. I thought about reading it through open(), but because there's python objects that could get ugly really quickly.

Comment on attachment 474859 [details] [diff] [review] untested first try at a tagging script >+#!/usr/bin/python >+ >+from os import path >+import sys >+ >+from release.info import readReleaseConfig, getTags, generateRelbranchName >+from release.l10n import getShippedLocales, getL10nRepositories >+from release.platforms import getAllLocales >+ >+VERSION_BUMP_SCRIPT="version-bump.pl" >+EN_US_BUMP_FILES=['config/milestone.txt', 'js/src/config/milestone.txt', >+ 'browser/config/version.txt'] >+DEFAULT_MAX_PUSH_ATTEMPTS=10 >+DEFAULT_L10N_CHANGESETS='l10n-changesets' >+ >+def readConfig(): >+ import release_config >+ return readReleaseConfig(release_config) Nooooo...there's gotta be a better way to do this. Maybe something like c = {} execfile(configfile, c) return readReleaseConfig(c) >+def tag(repo, revision, tags, username): >+ for tag in tags: >+ cmd = ['hg', 'tag', '-u', username, '-r', revision, >+ '-m', getTagCommitMessage(revision, tag)] >+ if 'RELEASE' not in tag: >+ cmd.append('-f') Why don't you want to force release tags? >+def push(repo, username, sshKey, attempts): >+ ssh = 'ssh -l %s %s' % (username, sshKey) Missing a '-i'? Or does sshKey already contain that? >+ runCmd(['hg', 'out', '-e', ssh], cwd=repo) >+ for n in range(1, attempts+1): >+ runCmd(['hg', 'pull'], cwd=repo) >+ runCmd(['hg', 'rebase'], cwd=repo) >+ print "Attempting to push, %d/%d" % (n, attempts) >+ try: >+ runCmd(['hg', 'push', '-e', ssh, '-f', sshRepo], cwd=repo) >+ break >+ except: >+ if n == attempts: >+ print "Unable to push to %s, bailing" % repo >+ raise I don't know how much I trust rebase. What about doing loop: hg out hg pull try: hg rebase except: hg update -C re-tag / etc hg push >+def tagRepo(config, repo, reponame, revision, tags, bumpFiles, pushAttempts): >+ relbranch = config['relbranchOverride'] >+ doClone(repo, reponame) >+ if relbranch: >+ runCmd(['hg', 'up', '-C', relbranch]) >+ else: >+ runCmd(['hg', 'up', '-C', '-r', revision]) >+ runCmd(['hg', 'branch', relbranch]) Logic is busted here...If relbranch is set, we'll try and update to it. If it's not set, we'll try and create a new unnamed branch? >+ if buildNumber == 1 or len(bumpFiles) == 0: >+ print "Not bumping anything. buildNumber is " + buildNumber + \ >+ " bumpFiles is " + bumpFiles >+ else: >+ revision = bump(repo, config['version'], config['appVersion'], >+ config['appName'], config['productName'], >+ config['milestone'], config['buildNumber'], >+ bumpFiles, config['hgUsername']) >+ tag(repo, revision, tags, username) >+ push(repo, username, config['hgSshKey'], pushAttempts) I think the tag/push operations need to be retried, as explained above. >+if __name__ == '__main__': >+ from optparse import OptionParser >+ import os >+ >+ parser = OptionParser(__doc__) >+ parser.set_defaults( >+ attempts=os.environ.get('MAX_PUSH_ATTEMPTS', DEFAULT_MAX_PUSH_ATTEMPTS), >+ l10n_changesets=os.environ.get('L10N_CHANGESETS', DEFAULT_L10N_CHANGESETS) >+ ) >+ parser.add_option("-a", "--push-attempts", dest="attempts", >+ help="Number of attempts before giving up on pushing") >+ parser.add_option("-l", "--l10n-changesets", dest="l10n_changesets", >+ help="File to read l10n tagging information from") >+ >+ options, args = parser.parse_args() >+ >+ if not path.exists("release_config.py"): >+ print "release_config.py must exist!" >+ sys.exit(1) >+ config = readConfig() Pass in a filename for release_config, and we can find another way to parse it.

(In reply to comment #5) > Nooooo...there's gotta be a better way to do this. > > Maybe something like > c = {} > execfile(configfile, c) > return readReleaseConfig(c) Awesome, I didn't know you could do that with execfile(). > >+def tag(repo, revision, tags, username): > >+ for tag in tags: > >+ cmd = ['hg', 'tag', '-u', username, '-r', revision, > >+ '-m', getTagCommitMessage(revision, tag)] > >+ if 'RELEASE' not in tag: > >+ cmd.append('-f') > > Why don't you want to force release tags? Whoops, I got that logic backwards. > >+def push(repo, username, sshKey, attempts): > >+ ssh = 'ssh -l %s %s' % (username, sshKey) > > Missing a '-i'? Or does sshKey already contain that? Yeah, I missed -i. > >+ runCmd(['hg', 'out', '-e', ssh], cwd=repo) > >+ for n in range(1, attempts+1): > >+ runCmd(['hg', 'pull'], cwd=repo) > >+ runCmd(['hg', 'rebase'], cwd=repo) > >+ print "Attempting to push, %d/%d" % (n, attempts) > >+ try: > >+ runCmd(['hg', 'push', '-e', ssh, '-f', sshRepo], cwd=repo) > >+ break > >+ except: > >+ if n == attempts: > >+ print "Unable to push to %s, bailing" % repo > >+ raise > > I don't know how much I trust rebase. What about doing > loop: > hg out > hg pull > try: > hg rebase > except: > hg update -C > re-tag / etc > hg push Why don't you trust rebase? It will fail if we pull a revision that is modifying tags, but I can't see why it would fail otherwise. > >+def tagRepo(config, repo, reponame, revision, tags, bumpFiles, pushAttempts): > >+ relbranch = config['relbranchOverride'] > >+ doClone(repo, reponame) > >+ if relbranch: > >+ runCmd(['hg', 'up', '-C', relbranch]) > >+ else: > >+ runCmd(['hg', 'up', '-C', '-r', revision]) > >+ runCmd(['hg', 'branch', relbranch]) > > Logic is busted here...If relbranch is set, we'll try and update to it. If > it's not set, we'll try and create a new unnamed branch? Hmmmm, I see what you mean. I lost something in my translation of the factory. I'll fix this.

Alright, this version should address all of your comments, Chris, and I've tested one scenario -- I'll post the configs used and the log. The diff is against your nanojit branch. It's not quite ready for review yet (needs a few more scenarios tested), but I wanted to get one more round of feedback before I spend the time doing that. A couple more notes: - I'm really not happy with what the output looks right now, I'll fix that in a later version. - There's a couple of todos in tagRepo that I intend to fix before this land.

Sorry for the churn, I somehow missed my hg.py changes....

The main purpose of this patch is update the push/apply_and_push to require Mercurial 1.6. It turns out that there's no safe way to do tagging without --branch and --new-branch arguments to push. It's also much much much easier to write apply_and_push when rebase and strip are available. Further changes: - Tests for push/apply_and_push - Make command.py logging a lot prettier - Factor out revision/branch/sshkey/username arg generation for pull/out/push I'm really not happy about the way apply_and_push reads. Catlee, any ideas how to make it better? This patch also depends on upgrading the python 2.5 Mercurial -- I have a Puppet patch for doing that, and deploying an hgrc to enable rebase/strip. To come: buildbotcustom, buildbot-configs, puppet-manifests patches.

So, this starts using a ScriptFactory for desktop Firefox builds only. It requires that we kick off the automation with two properties: release_config and l10n_changesets set -- and that buildbot-configs is tagged prior to starting the automation. For production, this means that we can drop the l10n changesets file from the config. We still need it for staging because of repo_setup. Also todo in staging is testing whether or not the properties set by the sendchange propagate past repo_setup.

Comment on attachment 477795 [details] [diff] [review] updated to require Mercurial 1.6 >diff --git a/lib/python/util/commands.py b/lib/python/util/commands.py >--- a/lib/python/util/commands.py >+++ b/lib/python/util/commands.py >@@ -1,48 +1,59 @@ > """Functions for running commands""" > import subprocess, os > > import logging > log = logging.getLogger(__name__) > >+def log_cmd(cmd, **kwargs): >+ if 'cwd' in kwargs and kwargs['cwd']: >+ cwd = kwargs['cwd'] >+ else: >+ cwd = os.getcwd() >+ log.info("command: START") >+ log.info("command: arg0: %s" % cmd[0]) >+ log.info("command: args: %s" % " ".join(cmd[1:])) >+ log.info("command: workdir: %s" % cwd) >+ log.info("command: output:") You don't need to use % interpolation with logging commands if you don't want. e.g. log.info("command: workdir: %s", cwd) Why are you splitting up arg0 from args? >+def make_repo_uri(repoPath, hgHost, protocol='http'): >+ if repoPath.startswith('/'): >+ repoPath = repoPath.rstrip('/') >+ return '%s://%s/%s' %( protocol, hgHost, repoPath) do you mean .endswith('/')? >+def common_args(revision=None, branch=None, username=None, sshKey=None): >+ args = [] >+ if username or sshKey: >+ opt = ['-e', 'ssh'] >+ if username: >+ opt[1] += ' -l %s' % username >+ if sshKey: >+ opt[1] += ' -i %s' % sshKey >+ args.extend(opt) >+ if revision: >+ args.extend(['-r', revision]) >+ if branch: >+ if hg_ver() >= (1, 6, 0): >+ args.extend(['-b', branch]) >+ return args change to ssh_username / ssh_key? >+def out(repo, dest, **kwargs): >+ cmd = ['hg', '-q', 'out', '--template', '{node}\n'] >+ cmd.extend(common_args(**kwargs)) >+ cmd.append(dest) >+ log.info(" ".join(cmd)) >+ revs = get_output(cmd, cwd=repo) >+ return get_output(cmd, cwd=repo).rstrip().split("\n") >+ >+def push(repo, dest, **kwargs): >+ cmd = ['hg', 'push'] >+ cmd.extend(common_args(**kwargs)) >+ if 'branch' in kwargs and kwargs['branch']: >+ cmd.append('--new-branch') >+ cmd.append(dest) >+ log.info(" ".join(cmd)) >+ run_cmd(cmd, cwd=repo) I think 'dest' and 'repo' need to be renamed for these. 'repo' has the opposite meaning here from things like 'pull'. Maybe have arguments called 'src' and 'repo' where 'repo' refers to the remote repository? >+def apply_and_push(localrepo, remote, changer, max_attempts=10, branch=None, >+ username=None, sshKey=None): >+ """This function calls `changer' to make changes to the repo, and tries >+ its hardest to get them to the origin repo.""" Please add a docstring describing what arguments `changer` is expecting. >+ assert callable(changer) >+ branch = get_branch(localrepo) This branch shadows the branch argument to the function. >+ changer(localrepo, 1) >+ for n in range(1, max_attempts+1): >+ tip = get_revision(localrepo) >+ try: >+ new_revs = out(dest=remote, repo=localrepo, revision=tip, >+ branch=branch, username=username, sshKey=sshKey) >+ except subprocess.CalledProcessError, e: >+ if e.returncode == 1: >+ raise HgUtilError("No revs to push") >+ else: >+ raise >+ try: >+ push(dest=remote, repo=localrepo, revision=tip, branch=branch, >+ username=username, sshKey=sshKey) >+ return >+ except subprocess.CalledProcessError, e: >+ log.debug("Hit error when trying to push: %s" % str(e)) >+ if n == max_attempts: >+ log.debug("Tried %d times, giving up" % max_attempts) This should generate an error by raising an exception or something. The rest looks good. How does this cope with conflicts in the case where two people are tagging the same repo at the same time?

Comment on attachment 477796 [details] [diff] [review] add ScriptFactory to default buildbotcustom branch Not thrilled with losing the properties download, etc., but it's short term until we can move release builders into 0.8.0.

(In reply to comment #15) > How does this cope with conflicts in the case where two people are tagging the > same repo at the same time? Whomever gets there first will succeed as normal, of course. The second tagger will: - Fail to push - Pull - Fail to rebase - Update to the new tip of the relbranch - Strip away the first attempt at bumping+tagging - Retry the bumping+tagging using the new tip of the relbranch as the parent - Push successfully This case is tested in testApplyAndPushRebaseFails

...and to answer your other questions > Why are you splitting up arg0 from args? I was mostly copying the output from Bootstrap, which I was a fan of (http://mxr.mozilla.org/mozilla/source/tools/release/Bootstrap/Step.pm#75). I'm not tied to it though, I could put it all on one line if you'd prefer. > >+def make_repo_uri(repoPath, hgHost, protocol='http'): > >+ if repoPath.startswith('/'): > >+ repoPath = repoPath.rstrip('/') > >+ return '%s://%s/%s' %( protocol, hgHost, repoPath) > > do you mean .endswith('/')? I don't think so, I can't think of a scenario where repoPath would end with /. > >+def common_args(revision=None, branch=None, username=None, sshKey=None): > change to ssh_username / ssh_key? Sure > >+def out(repo, dest, **kwargs): > >+ cmd = ['hg', '-q', 'out', '--template', '{node}\n'] > >+ cmd.extend(common_args(**kwargs)) > >+ cmd.append(dest) > >+ log.info(" ".join(cmd)) > >+ revs = get_output(cmd, cwd=repo) > >+ return get_output(cmd, cwd=repo).rstrip().split("\n") > >+ > >+def push(repo, dest, **kwargs): > >+ cmd = ['hg', 'push'] > >+ cmd.extend(common_args(**kwargs)) > >+ if 'branch' in kwargs and kwargs['branch']: > >+ cmd.append('--new-branch') > >+ cmd.append(dest) > >+ log.info(" ".join(cmd)) > >+ run_cmd(cmd, cwd=repo) > > I think 'dest' and 'repo' need to be renamed for these. 'repo' has the > opposite meaning here from things like 'pull'. Maybe have arguments called > 'src' and 'repo' where 'repo' refers to the remote repository? I can see the logic in that -- will do. > >+def apply_and_push(localrepo, remote, changer, max_attempts=10, branch=None, > >+ username=None, sshKey=None): > >+ """This function calls `changer' to make changes to the repo, and tries > >+ its hardest to get them to the origin repo.""" > > Please add a docstring describing what arguments `changer` is expecting. K > >+ assert callable(changer) > >+ branch = get_branch(localrepo) > > This branch shadows the branch argument to the function. Ah, that's bad. I'll fix it. > >+ changer(localrepo, 1) > >+ for n in range(1, max_attempts+1): > >+ tip = get_revision(localrepo) > >+ try: > >+ new_revs = out(dest=remote, repo=localrepo, revision=tip, > >+ branch=branch, username=username, sshKey=sshKey) > >+ except subprocess.CalledProcessError, e: > >+ if e.returncode == 1: > >+ raise HgUtilError("No revs to push") > >+ else: > >+ raise > >+ try: > >+ push(dest=remote, repo=localrepo, revision=tip, branch=branch, > >+ username=username, sshKey=sshKey) > >+ return > >+ except subprocess.CalledProcessError, e: > >+ log.debug("Hit error when trying to push: %s" % str(e)) > >+ if n == max_attempts: > >+ log.debug("Tried %d times, giving up" % max_attempts) > > This should generate an error by raising an exception or something. The end of the function raises HgUtilError. I guess I should move it beside the log message now.

Comment on attachment 477796 [details] [diff] [review] add ScriptFactory to default buildbotcustom branch Ready for final review here

Chris, this patch should have all your comments addressed. Nick, do you think you'll be able to look at this this week?

Comment on attachment 477799 [details] [diff] [review] replace ReleaseTaggingFactory with ScriptFactory This one too.

Comment on attachment 477799 [details] [diff] [review] replace ReleaseTaggingFactory with ScriptFactory Pretty much ignored the staging release configs. >+tag_factory = ScriptFactory( >+ scriptRepo='%s%s' % (branchConfig['hgurl'], >+ branchConfig['build_tools_repo_path']), >+ scriptName='scripts/release/tag-release.py', >+ extra_args=['-c', WithProperties('mozilla2-staging/%(release_config)s'), >+ '-l', WithProperties('mozilla2-staging/%(l10n_changesets)s'), Where do these two properties get set ? Otherwise looks fine.

Comment on attachment 477799 [details] [diff] [review] replace ReleaseTaggingFactory with ScriptFactory Ah, you described that in comment #14.

Comment on attachment 482660 [details] [diff] [review] address catlee's review comments >diff --git a/lib/python/release/info.py b/lib/python/release/info.py >+def generateRelbranchName(milestone, prefix='GECKO'): >+ return '%s%s_%s_RELBRANCH' % ( >+ prefix, milestone.replace('.', ''), >+ datetime.now().strftime('%Y%m%d')) Potentially regresses bug 496549, supposing that ever lands. >diff --git a/lib/python/test.sh b/lib/python/test.sh Need a setup doc/script here to make this portable, or accidental inclusion ? >diff --git a/lib/python/util/commands.py b/lib/python/util/commands.py >+def log_cmd(cmd, **kwargs): >+ if 'cwd' in kwargs and kwargs['cwd']: >+ cwd = kwargs['cwd'] >+ else: >+ cwd = os.getcwd() >+ log.info("command: START") >+ log.info("command: arg0: %s", cmd[0]) >+ log.info("command: args: %s", " ".join(cmd[1:])) I quite like the way buildbot prints this sort of thing as argv: ['rm', '-rf', 'configs'] to avoid any ambiguity around spaces. Worth considering. > def get_output(cmd, include_stderr=False, **kwargs): ... >+ output = proc.stdout.read() >+ return output >+ finally: >+ log.info("command: END\n") I tempted to ask for a 'print output' in the finally, but perhaps I should take a peek at an actual log first. And it may not be appropriate for all calls to get_output. >diff --git a/lib/python/util/hg.py b/lib/python/util/hg.py >+def out(src, remote, **kwargs): >+ cmd = ['hg', '-q', 'out', '--template', '{node}\n'] >+ cmd.extend(common_args(**kwargs)) >+ cmd.append(remote) >+ log.info(" ".join(cmd)) >+ return get_output(cmd, cwd=src).rstrip().split("\n") Looks like get_output will do more logging than this solitary log.info will do, remove it here ? Same for push(). >+def apply_and_push(localrepo, remote, changer, max_attempts=10, >+ ssh_username=None, ssh_key=None): ... >+ try: >+ run_cmd(['hg', 'rebase'], cwd=localrepo) At first I was confused that we go back around the loop after a successful rebase to try pushing again, probably worth a comment about that. >diff --git a/scripts/release/tag-release.py b/scripts/release/tag-release.py >+def tagRepo(config, repo, reponame, revision, tags, bumpFiles, pushAttempts): >+ clone(make_repo_uri(repo, HG), reponame) >+ relbranch = config['relbranchOverride'] >+ >+ def bump_and_tag(repo, attempt, config, relbranch, revision, tags): >+ if attempt > 1: >+ # Because all of the work we do is on a branch we don't need to >+ # retry it. If another change was made on this branch we will still >+ # fail, but changes on any other branch will not affect us. >+ # It would be better to strip and redo everything, or rebase, >+ # but currently we need compatibility with Mercurial 1.1.2 >+ return Given the code in hg.py this doesn't look correct any more. Looks like we're going to be calling bump_and_tag with a pristine checkout, or one that failed to rebase and was stripped. So can't this whole if block go away ? >+ try: >+ update(reponame, revision=relbranch) >+ except: >+ update(reponame, revision=revision) >+ run_cmd(['hg', 'branch', relbranch], cwd=reponame) I'd like to tighten this up a bit. In any situation where we've specified a relbranchOverride in the config it should be terminal if we can't update to that. As it is, if we were build >=2 on a non-chemspill and the relbranchOverride was typo-ed then we end up creating a bogus relbranch. We could also avoid the spurious update failure for a build 1 non-chemspill (relbranch doesn't exist yet). Sorry that shreds your short little code block.

(In reply to comment #24) > Comment on attachment 482660 [details] [diff] [review] > address catlee's review comments > > >diff --git a/lib/python/release/info.py b/lib/python/release/info.py > >+def generateRelbranchName(milestone, prefix='GECKO'): > >+ return '%s%s_%s_RELBRANCH' % ( > >+ prefix, milestone.replace('.', ''), > >+ datetime.now().strftime('%Y%m%d')) > > Potentially regresses bug 496549, supposing that ever lands. Is this just a note, or would you like action here? I'm not really sure how to prevent that. > Need a setup doc/script here to make this portable, or accidental inclusion ? > This is accidental, I'll remove it. > I quite like the way buildbot prints this sort of thing as > argv: ['rm', '-rf', 'configs'] > to avoid any ambiguity around spaces. Worth considering. No arguments there. > > def get_output(cmd, include_stderr=False, **kwargs): > ... > >+ output = proc.stdout.read() > >+ return output > >+ finally: > >+ log.info("command: END\n") > > I tempted to ask for a 'print output' in the finally, but perhaps I should take > a peek at an actual log first. And it may not be appropriate for all calls to > get_output. Yeah, I'm not sure.....Catlee, any thoughts? > >+ log.info(" ".join(cmd)) > >+ return get_output(cmd, cwd=src).rstrip().split("\n") > > Looks like get_output will do more logging than this solitary log.info will do, > remove it here ? Same for push(). Looks like I forgot to remove this after centralizing the logging in the util functions. > >+ try: > >+ run_cmd(['hg', 'rebase'], cwd=localrepo) > > At first I was confused that we go back around the loop after a successful > rebase to try pushing again, probably worth a comment about that. Added. <snip> > >+ # fail, but changes on any other branch will not affect us. > >+ # It would be better to strip and redo everything, or rebase, > >+ # but currently we need compatibility with Mercurial 1.1.2 > >+ return > > Given the code in hg.py this doesn't look correct any more. Looks like we're > going to be calling bump_and_tag with a pristine checkout, or one that failed > to rebase and was stripped. So can't this whole if block go away ? Yeah. I guess I forgot to remove this after switching to Mercurial 1.6.3. > >+ try: > >+ update(reponame, revision=relbranch) > >+ except: > >+ update(reponame, revision=revision) > >+ run_cmd(['hg', 'branch', relbranch], cwd=reponame) > > I'd like to tighten this up a bit. In any situation where we've specified a > relbranchOverride in the config it should be terminal if we can't update to > that. As it is, if we were build >=2 on a non-chemspill and the > relbranchOverride was typo-ed then we end up creating a bogus relbranch. We > could also avoid the spurious update failure for a build 1 non-chemspill > (relbranch doesn't exist yet). Sorry that shreds your short little code block. No apologies necessary, especially after the 3.5.14 fun I'm happy for tagging to be less liberal.

Comment on attachment 482660 [details] [diff] [review] address catlee's review comments Removing remaining review, will repost an updated patch later.

This should address all of the things you mentioned, Nick. To deal with the erroring out you wanted I stopped overriding the releaseConfig['relbranchOverride'] variable, which was a little dirty to do anyways. Turns out that by passing in a flag like I'm doing it's very easy to deal with that case inside of bump_and_tag(). I've got a master running that has some sample runs, for your viewing pleasure: Successful tagging of 4.0b17build1: http://staging-master.build.mozilla.org:8018/builders/tag/builds/49 Successful tagging of 4.0b17build2 (prior to completely fixing the logging): http://staging-master.build.mozilla.org:8018/builders/tag/builds/47 Bailing out because the relbranch didn't exist in 4.0b18build3: http://staging-master.build.mozilla.org:8018/builders/tag/builds/53 You can also see tagging results in my user repos: http://hg.mozilla.org/users/bhearsum_mozilla.com/mozilla-central/ http://hg.mozilla.org/users/bhearsum_mozilla.com/sl/ http://hg.mozilla.org/users/bhearsum_mozilla.com/mk/ 4.0b17build2 is a bit funny, because I did it before build1, and on the b16 relbranch, but I think it's still a valid test.

Comment on attachment 482951 [details] [diff] [review] tagging scripts, nick's comments addressed Thanks for the logs, those are very helpful. Some comments, questions, confusions: * no call to clobberer ? A previous tagging could be in build/, is that by design ? * not pulling tools/ on the release tag ? * I do want a 'print output' in the finally of get_output, annoying that the script gets to see that output but not log it for paranoid types * could be detecting the hg version when hg.py is loaded rather than testing it each time, but ... * is there a log ordering problem ? Shouldn't the first command be an 'hg -q ver' from the clone() of buildbot-configs ? Also don't understand where the 'hg parent' comes from so early in the log, we don't call get_revision until bumping m-c. Disclaimer - it's totally MFBT, brain my be fried.

(In reply to comment #28) > Comment on attachment 482951 [details] [diff] [review] > tagging scripts, nick's comments addressed > > Thanks for the logs, those are very helpful. > > Some comments, questions, confusions: > * no call to clobberer ? A previous tagging could be in build/, is that by > design ? Definitely not. We're going to need to solve this for other script factory release stuff...so I'll give this one some good thought. > * not pulling tools/ on the release tag ? I'll address this. > * I do want a 'print output' in the finally of get_output, annoying that the script gets to see that output but not log it for paranoid types Will fix. > * could be detecting the hg version when hg.py is loaded rather than testing it > each time, but ... I'd prefer not to address this in this bug. > * is there a log ordering problem ? Shouldn't the first command be an 'hg -q > ver' from the clone() of buildbot-configs ? Also don't understand where the 'hg > parent' comes from so early in the log, we don't call get_revision until > bumping m-c. Disclaimer - it's totally MFBT, brain my be fried. I don't _think_ there's a log ordering problem: - 'hg -q ver' is from get_output, and in those logs, that doesn't log. - hg_ver() only logs to DEBUG, and tag-release.py runs at INFO level. - the 'hg parent' comes get_revision(), which is called by update(), which is called by clone().

(In reply to comment #29) > (In reply to comment #28) > > * not pulling tools/ on the release tag ? > > I'll address this. Does that mean anyone doing any release will need to manually tag tools/ in the future?

(In reply to comment #30) > (In reply to comment #29) > > (In reply to comment #28) > > > * not pulling tools/ on the release tag ? > > > > I'll address this. > > Does that mean anyone doing any release will need to manually tag tools/ in the > future? Yeah, when this lands (which will be on the 0.8.0 branch only), you'll need to tag buildbot-configs and tools prior to starting a release.

(In reply to comment #31) > Yeah, when this lands (which will be on the 0.8.0 branch only), you'll need to > tag buildbot-configs and tools prior to starting a release. Hmm, so it will take more manual step, i.e. being less automated than it's now fore me - I always long for reducing manual steps, as it's those I tend to forget even though I have documents that list them. :(

I'm not sure about you, but we've been tagging our build repos with _RELEASE and _BUILD tags for awhile now. Sorry that this adds more for you, though.

Comment on attachment 477796 [details] [diff] [review] add ScriptFactory to default buildbotcustom branch I'm targeting the 0.8.0 automation with this now, so we don't need this ScriptFactory addition.

Fairly simple: - Get ScriptFactory to set the 'master' property (for clobberer) - Set BUILDBOT_CONFIGS and CLOBBERER_URL in the env for all releases, through the builder - Set HG_REPO for tagging factory, through the builder - Set builddir property for tagging factory, through the builder

A few things here: - Addition of jsontool.py to easily parse JSON in .sh scripts - Fix readReleaseConfig to read 0.8.0-style configs - Wrap tag-release.py in tagging.sh, to enable us to run clobberer and run tag-release.py from the correct revision of build/tools. A few things of note: - clobberer works, but I have to exclude 'build', otherwise it clobbers the currently running scripts, which means the hgtool.py call fails - clobberer isn't technically needed here because the clone() function that tag-release.py calls will remove any existing directories before cloning. I think this is OK for tagging, but we may want to revisit that behaviour if/when we port other things to script factories. You can see various runs on this staging master: http://staging-master.build.mozilla.org:8018/builders/release-mozilla-1.9.2-tag?numbuilds=10 The failed ones after #38 are my fault (missing a tag).

Comment on attachment 487725 [details] [diff] [review] buildbotcustom changes for tagging scripts on 0.8.0 >+ tag_factory = ScriptFactory( >+ scriptRepo='%s%s' % (branchConfig['hgurl'], >+ branchConfig['build_tools_repo_path']), >+ scriptName='scripts/release/release-tag.py', >+ extra_args=['-c', >+ WithProperties('mozilla2-staging/%(release_config)s'), >+ '-l', >+ WithProperties('mozilla2-staging/%(l10n_changesets)s'), >+ '-b', '%s%s' % (branchConfig['hgurl'], >+ branchConfig['config_repo_path']), >+ '-t', '%s_RELEASE' % releaseConfig['baseTag']], >+ interpreter='/tools/python/bin/python' Do you mean to be referencing the staging configs here?

(In reply to comment #40) > Comment on attachment 487725 [details] [diff] [review] > buildbotcustom changes for tagging scripts on 0.8.0 > > >+ tag_factory = ScriptFactory( > >+ scriptRepo='%s%s' % (branchConfig['hgurl'], > >+ branchConfig['build_tools_repo_path']), > >+ scriptName='scripts/release/release-tag.py', > >+ extra_args=['-c', > >+ WithProperties('mozilla2-staging/%(release_config)s'), > >+ '-l', > >+ WithProperties('mozilla2-staging/%(l10n_changesets)s'), > >+ '-b', '%s%s' % (branchConfig['hgurl'], > >+ branchConfig['config_repo_path']), > >+ '-t', '%s_RELEASE' % releaseConfig['baseTag']], > >+ interpreter='/tools/python/bin/python' > > Do you mean to be referencing the staging configs here? Whoops, I missed that when moving from 0.7.x -> 0.8.x.

Comment on attachment 487732 [details] [diff] [review] tagging scripts, retested in 0.8.0 land per IRC, going to read the release config file to figure out which l10n file to use.

Comment on attachment 487726 [details] [diff] [review] remove l10nRevisionFile from production release configs per IRC, I think we want to keep these in.

Comment on attachment 487725 [details] [diff] [review] buildbotcustom changes for tagging scripts on 0.8.0 Turns out I attached the wrong diff here, sorry about that.

Catlee, this should address everything we talked about today. Highlights are: - Set 'master' property in ScriptFactory - Add revision support to ScriptFactory, using a doStepIf to protect against the revision=None case. I tested this with the tagging scripts, which have revision set to a _RELEASE tag, and with fuzzing, which has it set to None. Worked fine in both places. - Set BUILDBOT_CONFIGS property and CLOBBERER_URL property in all release builders. Any future work that reads the release config will find BUILDBOT_CONFIGS being set to be useful and all ScriptFactory release builders will need to be able to run clobberer.

Not much new here. I merged my make_repo_uri with syed's make_hg_url, getting rid of mine altogether. The arguments changed a little bit so I had to adjust release_sanity.py for that. I dropped the l10n-changesets arguments in favour of reading it from the release config per your request, catlee. By virtue of the fact that ScriptFactory is doing the updating to the _RELEASE version we're now running tagging.sh from the tagged version, which is great.

Comment on attachment 488216 [details] [diff] [review] build/tools patch, merged against recent landings >diff --git a/buildfarm/utils/jsontool.py b/buildfarm/utils/jsontool.py I know this is a short, simple, file but a one liner comment like 'retrieves a single key from a json encoded file' wouldn't go amiss. >+ parser.add_option("-k", "--key", dest="key") >+ parser.add_option("-d", "--delimeter", dest="delimeter") Spelling nit - use delimiter. Might also be worth a comment that the delimiter is what's used in the single key you want to look up, rather than allowing you to query more than one key in a single shot. >diff --git a/lib/python/release/l10n.py b/lib/python/release/l10n.py >+def getL10nRepositories(file, l10nRepoPath, relbranch): >+ """Reads in a list of locale names and revisions for their associated >+ repository from 'file'. >+ """ >+ if not l10nRepoPath.endswith('/'): >+ l10nRepoPath = l10nRepoPath + '/' Nit: urljoin will take care of this won't it ? >diff --git a/lib/python/util/commands.py b/lib/python/util/commands.py > def get_output(cmd, include_stderr=False, **kwargs): ... >+ output = proc.stdout.read() >+ return output Did you try logging the output here as I suggested on an earlier review ? Could be invaluable if something goes wrong on a release. >diff --git a/scripts/release/tag-release.py b/scripts/release/tag-release.py >+def getBumpCommitMessage(productName, version): >+ return 'Automated checkin: version bump remove "pre" from version number ' \ >+ 'for ' + productName + ' ' + version + ' release. CLOSED TREE' >+ >+def getTagCommitMessage(revision, tag): >+ return "Added tag " + tag + " for changeset " + revision + ". CLOSED TREE" Please don't regress bug 589914 here. >diff --git a/scripts/release/tagging.sh b/scripts/release/tagging.sh >+releaseTag=$($JSONTOOL -k sourcestamp.revision $PROPERTIES_FILE) I'm curious how this gets set now. A different sendchange ? Conditional r+.

(In reply to comment #47) > Comment on attachment 488216 [details] [diff] [review] > build/tools patch, merged against recent landings > > >diff --git a/buildfarm/utils/jsontool.py b/buildfarm/utils/jsontool.py > > I know this is a short, simple, file but a one liner comment like 'retrieves a > single key from a json encoded file' wouldn't go amiss. > >+ parser.add_option("-k", "--key", dest="key") > >+ parser.add_option("-d", "--delimeter", dest="delimeter") > > Spelling nit - use delimiter. Might also be worth a comment that the delimiter > is what's used in the single key you want to look up, rather than allowing you > to query more than one key in a single shot. Will do. > >diff --git a/lib/python/release/l10n.py b/lib/python/release/l10n.py > >+def getL10nRepositories(file, l10nRepoPath, relbranch): > >+ """Reads in a list of locale names and revisions for their associated > >+ repository from 'file'. > >+ """ > >+ if not l10nRepoPath.endswith('/'): > >+ l10nRepoPath = l10nRepoPath + '/' > > Nit: urljoin will take care of this won't it ? I'm not sure, I'll find out and drop this if it does. > >diff --git a/lib/python/util/commands.py b/lib/python/util/commands.py > > def get_output(cmd, include_stderr=False, **kwargs): > ... > >+ output = proc.stdout.read() > >+ return output > > Did you try logging the output here as I suggested on an earlier review ? Could > be invaluable if something goes wrong on a release. Hmmm, I thought I did. Sorry about that, I'll add it. > >diff --git a/scripts/release/tag-release.py b/scripts/release/tag-release.py > >+def getBumpCommitMessage(productName, version): > >+ return 'Automated checkin: version bump remove "pre" from version number ' \ > >+ 'for ' + productName + ' ' + version + ' release. CLOSED TREE' > >+ > >+def getTagCommitMessage(revision, tag): > >+ return "Added tag " + tag + " for changeset " + revision + ". CLOSED TREE" > > Please don't regress bug 589914 here. K. > >diff --git a/scripts/release/tagging.sh b/scripts/release/tagging.sh > >+releaseTag=$($JSONTOOL -k sourcestamp.revision $PROPERTIES_FILE) > > I'm curious how this gets set now. A different sendchange ? Yeah, this is going to be a required property when kicking off a release. Sendchanges will look like this when this and the l10n scripts land: buildbot sendchange --username=bhearsum --master=localhost:9010 --branch=mozilla-central --property "release_config:mozilla/release-firefox-mozilla-central.py" -m " " --revision=FIREFOX_4_0b8_RELEASE gogogo Though, that will be wrapped in release_sanity.py.

(In reply to comment #48) > (In reply to comment #47) > > >+ if not l10nRepoPath.endswith('/'): > > >+ l10nRepoPath = l10nRepoPath + '/' > > > > Nit: urljoin will take care of this won't it ? > > I'm not sure, I'll find out and drop this if it does. Turns out that it doesn't: >>> from urlparse import urljoin >>> urljoin("releases/mozilla-1.9.2", "af") 'releases/af' >>> urljoin("releases/mozilla-1.9.2/", "af") 'releases/mozilla-1.9.2/af' I'll add a comment so we don't forget about this.

Comment on attachment 490935 [details] [diff] [review] nthomas' review comments addressed >diff --git a/scripts/release/tag-release.py b/scripts/release/tag-release.py >+def getBumpCommitMessage(productName, version): >+ return 'Automated checkin: version bump remove "pre" from version number ' \ >+ 'for ' + productName + ' ' + version + \ >+ ' release. CLOSED TREE a=release' >+ >+def getTagCommitMessage(revision, tag): >+ return "Added tag " + tag + " for changeset " + revision + ". CLOSED TREE" Need to add the 'a=release' to the tag messages too. The hook is looking at the last changeset IIRC. r+ with that. Hope the shared checkout testing works OK.

Comment on attachment 488215 [details] [diff] [review] add revision support to ScriptFactory, set necessary properties changeset: 1193:2028a7a63b74

Comment on attachment 490935 [details] [diff] [review] nthomas' review comments addressed changeset: 1267:74ceff4cffa0 changeset: 1268:6264faf6e0ac changeset: 1269:b8ca26bcd873

We're done here. Follow-up issues will be tracked in separate bugs.