Specifically, Bootstrap::Tag should support pulling from the CVS trunk. Not sure if we need anything else in Bootstrap itself for this.

I tried to run the trunk staging release automation environment and it died in the Tag step like so: log: Running shell command in /builds/tags/FIREFOX_3_0_A9_RC1/cvsroot: log: arg0: cvs log: arg1: -d log: arg2: staging-trunk-automation.build.mozilla.org:/builds/cvsmirror/cvsroot log: arg3: co log: arg4: -r log: arg5: HEAD log: arg6: -D log: arg7: 2007-10-26 03:07 PDT log: arg8: mozilla/client.mk log: Starting time is 10:29:26 10/29/07 log: Logging output to /builds/logs/tag_checkout_client_mk.log log: Timeout: 3600 cvs checkout: warning: new-born mozilla/client.mk has disappeared log: Ending time is 10:29:27 10/29/07 Step Tag died: ^U mozilla/client.mk is not present in file /builds/logs/tag_checkout_client_mk.log at Bootstrap/Step.pm line 149. It appears to be a symptom of trying to use the 'HEAD' tag. If I run that command manually sans '-r HEAD' it checkouts fine.

I'm currently testing this version of Bootstrap on the staging setups for 1.8 and 1.9. No r? yet, but any feedback is welcome.

This patch adds support to Tag.pm (and it's sub-steps) for trunk builds. Logic changes were made to the main Tag.pm file, Bump.pm, and l10n.pm. I also discovered and fixed a couple of error messages that printed out incorrect information. Fixes are included with this patch.

Comment on attachment 287112 [details] [diff] [review] bootstrap trunk support, Tag step As per his request, adding joduinn as r?

Comment on attachment 287112 [details] [diff] [review] bootstrap trunk support, Tag step Summary from IRC; We should also change the /mozilla/tools/release/configs/*.cfg files to add the "useTalkback = 1" line. Otherwise, landing this patch will silently change the 1.8 builds to not include Talkback for both staging and production. Can you add those cfg file changes to this patch and resubmit?

Can we add "-z3" to the cvs args, as a workaround for the win32 slave hang? Should we do this for all platforms, or should we explicitly only do this for win32? For details, see: https://bugzilla.mozilla.org/show_bug.cgi?id=355309#c41

Same as before, appends 'useTalkback = 1' to the end of each configuration file. There's been some discussion about adding a CVS command line utility function, so I didn't address the CVS compression issue yet.

Comment on attachment 287395 [details] [diff] [review] same as before, with updated config files Looks great, thanks for adding the config files too!

Comment on attachment 287395 [details] [diff] [review] same as before, with updated config files >Index: Bootstrap/Step/Tag.pm >=================================================================== >+ # If specified, use Talkback >+ if (1 == $useTalkback) { >+ push(@TAG_SUB_STEPS, 'Talkback'); >+ } Technically, this comment is kinda inaccurate; if specified in the config, this will *tag* talkback; it does affect the build in that the build will fail if Talkback isn't tagged, but in the config, the useTalkback flag doesn't control its usage in the build itself. Also, I would write this as: if "($useTalkback)", since you're not doing a string comparison. Also, unless you want to add the useTalkback variable to all of the configs, you probably want to assume it's 0, in which case the code above should be something like: my $useTalkback = $config->Exists(var => 'useTalkback') ? $config->Get(var => 'useTalkback') : 0; since a Get() on an undefined variable will assert. > if (1 == $rc) { >- $this->Shell(cmd => 'cvs', >- cmdArgs => ['-d', $mozillaCvsroot, >- 'co', >- '-r', $branchTag, >- '-D', $pullDate, >- CvsCatfile('mozilla', 'client.mk'), >- ], >- dir => $cvsrootTagDir, >- logFile => catfile($logDir, 'tag_checkout_client_mk.log'), >- ); >+ # Don't use a tag when pulling from HEAD >+ if ($branchTag eq 'HEAD') { >+ $this->Shell(cmd => 'cvs', >+ cmdArgs => ['-d', $mozillaCvsroot, >+ 'co', >+ '-D', $pullDate, >+ CvsCatfile('mozilla', 'client.mk'), >+ ], >+ dir => $cvsrootTagDir, >+ logFile => catfile($logDir, >+ 'tag_checkout_client_mk.log'), >+ ); >+ } >+ else { >+ $this->Shell(cmd => 'cvs', >+ cmdArgs => ['-d', $mozillaCvsroot, >+ 'co', >+ '-r', $branchTag, >+ '-D', $pullDate, >+ CvsCatfile('mozilla', 'client.mk'), >+ ], >+ dir => $cvsrootTagDir, >+ logFile => catfile($logDir, >+ 'tag_checkout_client_mk.log'), >+ ); >+ } From what I can tell, the only difference between these two branches is the addition of a "-r $branchTag" in cmdArgs. Is that the case? If so, can you add a comment describing that? Also, in cases such as this (where the command argument array differs by one or two arguments, based on some logic), in the past, we've built up the cmdArgs, and then push()ed the extra arguments onto the array as necessary. See Bootstrap::Util::CvsTag for an example. I'm gonna bet that you did this this way because I called you on doing exactly this (building up the arguments outside of the call) in some of the try server code, but in cases like this, where the argument slightly differ, based on program logic, we've made the tradeoff the being able to not-have-to-repeat all of the code to call RunShellCommand, and then seeing the logic so the minor argument differences stand out at you was worth it. > $this->CheckLog(log => catfile($logDir, 'tag_checkout_client_mk.log'), > checkForOnly => '^U mozilla/client.mk'); >Index: Bootstrap/Step/Tag/Bump.pm >=================================================================== >RCS file: /cvsroot/mozilla/tools/release/Bootstrap/Step/Tag/Bump.pm,v >retrieving revision 1.10 >diff -u -r1.10 Bump.pm >--- Bootstrap/Step/Tag/Bump.pm 10 Aug 2007 23:30:01 -0000 1.10 >+++ Bootstrap/Step/Tag/Bump.pm 5 Nov 2007 13:08:44 -0000 >@@ -94,6 +94,10 @@ > # Order or searching for these values is not preserved, so make > # sure that the order replacement happens does not matter. > if ($fileName eq 'client.mk') { >+ # If we're building from HEAD there's no changes for client.mk. >+ if ($branchTag eq 'HEAD') { >+ next; >+ } Is this correct logic? The changes to client.mk point things like NSPR, NSS, and the LDAP SDK to tags. In some cases, those are static tags; in other, they're branch tags. We shouldn't rely on this (undefined) behavior, so we should repoint client.mk to whatever the _RELEASE tag we're setting. > %searchReplace = ( > '^MOZ_CO_TAG\s+=\s+' . $branchTag . '$' => > 'MOZ_CO_TAG = ' . $releaseTag, >@@ -142,7 +146,7 @@ > close INFILE or die("Could not close $file: $!"); > close OUTFILE or die("Coule not close $file.tmp: $!"); > if (not $found) { >- die("None of " . join(keys(%searchReplace)) . >+ die("None of " . join(' ', keys(%searchReplace)) . > " found in file $file: $!"); > } > >@@ -174,6 +178,7 @@ > my $milestone = $config->Exists(var => 'milestone') ? > $config->Get(var => 'milestone') : undef; > my $rc = $config->Get(var => 'rc'); >+ my $branchTag = $config->Get(var => 'branchTag'); > > if ($rc > 1) { > $this->Log(msg => "Skipping Tag::Bump::Verify substep for RC $rc."); >@@ -183,7 +188,12 @@ > my $moduleVer = catfile($appName, 'app', 'module.ver'); > my $versionTxt = catfile($appName, 'config', 'version.txt'); > my $milestoneTxt = catfile('config', 'milestone.txt'); >- my @bumpFiles = ('client.mk', $moduleVer, $versionTxt); >+ my @bumpFiles = ($moduleVer, $versionTxt); >+ >+ # client.mk doesn't get bumped for HEAD runs. >+ if ($branchTag ne 'HEAD') { >+ @bumpFiles = (@bumpFiles, 'client.mk'); >+ } Ditto here. Most of these are nits, so I was going to r+ this, but the client.mk part needs to be fixed, I think.

(In reply to comment #9) > >+ # If specified, use Talkback > >+ if (1 == $useTalkback) { > >+ push(@TAG_SUB_STEPS, 'Talkback'); > >+ } > > Technically, this comment is kinda inaccurate; if specified in the config, this > will *tag* talkback; it does affect the build in that the build will fail if > Talkback isn't tagged, but in the config, the useTalkback flag doesn't control > its usage in the build itself. > Yeah, you're right. > Also, I would write this as: if "($useTalkback)", since you're not doing a > string comparison. > OK > Also, unless you want to add the useTalkback variable to all of the configs, > you probably want to assume it's 0, in which case the code above should be > something like: > > my $useTalkback = $config->Exists(var => 'useTalkback') ? $config->Get(var => > 'useTalkback') : 0; > > since a Get() on an undefined variable will assert. > This patch does add 'useTalkback' to all of the config files, but it doesn't hurt to handle that case better. I'll update the code. > > if (1 == $rc) { > >- $this->Shell(cmd => 'cvs', > >- cmdArgs => ['-d', $mozillaCvsroot, > >- 'co', > >- '-r', $branchTag, > >- '-D', $pullDate, > >- CvsCatfile('mozilla', 'client.mk'), > >- ], > >- dir => $cvsrootTagDir, > >- logFile => catfile($logDir, 'tag_checkout_client_mk.log'), > >- ); > >+ # Don't use a tag when pulling from HEAD > >+ if ($branchTag eq 'HEAD') { > >+ $this->Shell(cmd => 'cvs', > >+ cmdArgs => ['-d', $mozillaCvsroot, > >+ 'co', > >+ '-D', $pullDate, > >+ CvsCatfile('mozilla', 'client.mk'), > >+ ], > >+ dir => $cvsrootTagDir, > >+ logFile => catfile($logDir, > >+ 'tag_checkout_client_mk.log'), > >+ ); > >+ } > >+ else { > >+ $this->Shell(cmd => 'cvs', > >+ cmdArgs => ['-d', $mozillaCvsroot, > >+ 'co', > >+ '-r', $branchTag, > >+ '-D', $pullDate, > >+ CvsCatfile('mozilla', 'client.mk'), > >+ ], > >+ dir => $cvsrootTagDir, > >+ logFile => catfile($logDir, > >+ 'tag_checkout_client_mk.log'), > >+ ); > >+ } > > From what I can tell, the only difference between these two branches is the > addition of a "-r $branchTag" in cmdArgs. > > Is that the case? If so, can you add a comment describing that? > > Also, in cases such as this (where the command argument array differs by one or > two arguments, based on some logic), in the past, we've built up the cmdArgs, > and then push()ed the extra arguments onto the array as necessary. > > See Bootstrap::Util::CvsTag for an example. > > I'm gonna bet that you did this this way because I called you on doing exactly > this (building up the arguments outside of the call) in some of the try server > code, but in cases like this, where the argument slightly differ, based on > program logic, we've made the tradeoff the being able to not-have-to-repeat all > of the code to call RunShellCommand, and then seeing the logic so the minor > argument differences stand out at you was worth it. > > Understood. Joduinn suggested factoring out the 'HEAD' logic to a helper function (Bootstrap::Util::CvsCo, perhaps?). What do you think about this approach? It seems like a fine idea to me. The other reason this was suggested was to deal with the cvs hanging problems on Windows. I'm not sure if you're aware of them, but we've found that a 'cvs co' on the new win32 ref platform will complete it's checkout but never return, unless compression is used (-z3 has been working). If I add Bootstrap::Util::CvsCo I was thinking about adding another variable, CvsCoArgs, that we can specify '-z3' in. This may help deal with future problems, too. Alternatively, '-z3' could be hardcoded into the command line for platform == "win32". The only other solution to this problem I can think of is hardcoding '-z3' into all CVS checkouts; that seems less than ideal though. > > $this->CheckLog(log => catfile($logDir, 'tag_checkout_client_mk.log'), > > checkForOnly => '^U mozilla/client.mk'); > >Index: Bootstrap/Step/Tag/Bump.pm > >=================================================================== > >RCS file: /cvsroot/mozilla/tools/release/Bootstrap/Step/Tag/Bump.pm,v > >retrieving revision 1.10 > >diff -u -r1.10 Bump.pm > >--- Bootstrap/Step/Tag/Bump.pm 10 Aug 2007 23:30:01 -0000 1.10 > >+++ Bootstrap/Step/Tag/Bump.pm 5 Nov 2007 13:08:44 -0000 > >@@ -94,6 +94,10 @@ > > # Order or searching for these values is not preserved, so make > > # sure that the order replacement happens does not matter. > > if ($fileName eq 'client.mk') { > >+ # If we're building from HEAD there's no changes for client.mk. > >+ if ($branchTag eq 'HEAD') { > >+ next; > >+ } > > Is this correct logic? > > The changes to client.mk point things like NSPR, NSS, and the LDAP SDK to > tags. In some cases, those are static tags; in other, they're branch tags. We > shouldn't rely on this (undefined) behavior, so we should repoint client.mk to > whatever the _RELEASE tag we're setting. That makes perfect sense to me now. When I first put this together I was having trouble bumping client.mk. It made sense at the time to skip it, but now that I have a better understanding of what's going on I can see the flaws in that plan. Will fix this.

This patch addresses your previous comments, preed. It also factors out checkouts to Bootstrap::Step::CvsCo(). We chatted about doing this the way CvsTag() works, with the execution happening in Bootstrap::Util::CvsCo(). If that still sounds like the right course of action, that's what I'll do. I wanted to get this up right away though.

This is a merge of a few different branches I had going locally, so I'm going to go through all of the changes in it. * Recognize 'sysname' mingw32 as 'osname' win32 (and make sure re-mounting of cygdrive only happens on sysname cygwin). * Factor out CVS checkouts to Bootstrap::Step::CvsCo. There is still some CVS checkouts done manually in Bootstrap::Util. I didn't want to duplicate the entire CvsCo function in Bootstrap::Util() so I left the Bootstrap::Util() checkouts as they were. This is not hard to change if necessary. * Make Talkback tagging optional, defaulting to "don't tag it". I also fixed the commented there. * Changed the MOZ_CO_TAG regex to be a bit more broad. /^#?MOZ_CO_TAG\s+=.+$/ matches the branches and current trunk. * Fixed a couple debug messages (they were printing the wrong information) This patch is running OK so far, but still needs more testing before I'm really confident with it.

Same as before, fix small bug.

This patch is the same before with a couple of fixes: * Use sysname instead of osname to prevent cygwin specific code from being executed in msys * Add a new config variable, 'useTarGz', in order to support both tar.gz and tar.bz2 extensions. When absent, Bootstrap assumes tar.bz2, which should make this variable unnecessary in the future. I was asked by John awhile ago to include necessary configuration updates in the same patch as the code changes, so this patch has updates to configs/, too.

Comment on attachment 288655 [details] [diff] [review] same as before, with bugfixes for msys/bz2 support Hey Ben, So I started looking at this, and it's a pretty big patch (so it took some time to wrap my head around; still working on that, I think). Anyway, couple of questions: >+ my $echoFiles = exists($args{'echoFiles'}) ? $args{'echoFiles'} : 0; What is $echoFiles, and what's it used for? >+ my @cmdArgs; >+ push(@cmdArgs, '-z3') if ($useCvsCompression); I thought we were wanting this to be the default now? >Index: Bootstrap/Step/Tag/l10n.pm >=================================================================== >RCS file: /cvsroot/mozilla/tools/release/Bootstrap/Step/Tag/l10n.pm,v >retrieving revision 1.8 >diff -u -r1.8 l10n.pm >--- Bootstrap/Step/Tag/l10n.pm 27 Sep 2007 17:17:27 -0000 1.8 >+++ Bootstrap/Step/Tag/l10n.pm 14 Nov 2007 13:10:56 -0000 >@@ -56,28 +56,27 @@ > > # Check out the l10n files from the branch you want to tag. > >- my @l10nCheckoutArgs = (1 == $rc) ? >+ my %l10nCheckoutArgs = (1 == $rc) ? > # For rc1, pull by date on the branch >- ('-r', $branchTag, '-D', $l10n_pullDate) : >+ (branchTag => $branchTag, date => $l10n_pullDate) : > # For rc(N > 1), pull the _RELBRANCH tag and tag that >- ('-r', $geckoTag); >+ (branchTag => $geckoTag); > > for my $locale (sort(keys(%{$localeInfo}))) { > # skip en-US; it's kept in the main repo > next if ($locale eq 'en-US'); > >- $this->Shell( >- cmd => 'cvs', >- cmdArgs => ['-d', $l10nCvsroot, 'co', @l10nCheckoutArgs, >- CvsCatfile('l10n', $locale)], >- dir => $l10nTagDir, >- logFile => catfile($logDir, 'tag-l10n_checkout.log'), >- ); >+ $l10nCheckoutArgs{'cvsroot'} = $l10nCvsroot; >+ $l10nCheckoutArgs{'modules'} = [CvsCatfile('l10n', $locale)]; >+ $l10nCheckoutArgs{'workDir'} = $l10nTagDir; >+ $l10nCheckoutArgs{'logFile'} = catfile($logDir, >+ 'tag-l10n_checkout.log'); >+ $this->CvsCo(%l10nCheckoutArgs); This is mostly a style-istic issue, but I would do the standard calling convention for this (i.e. $this->CvsCo(cvsroot => ..., workDir => ...). My position on this may have been somewhat confusing, so let me try to clarify: I personally prefer the standard convention to be used, *except* when there NOT doing it provides a large benefit, like removing the need for huge if-branches that are all calling the same function, but with different arguments. I believe that's the only place where we've not doing that. My concern here, stylistically, is that you're basically doing a function call, but it doesn't look like it (or, at least, it doesn't stand out as one), because it doesn't use the style of the function calling conventions that Bootstrap uses. Hopefully that makes some sense, but catch my on IRC if it doesn't. One other thing: I noticed you did the useTarGz thing, which is fine, but you might consider changing it to linuxPackageExtension, and making the default .tar.bz2 if that isn't set. It should actually remove some of the tests you have in the code itself (making it a bit clearer), and insulates against future changes to the linux packaging extension (who knows what they may be, but...)

> What is $echoFiles, and what's it used for? > When true, checked out files will be piped to stdout. I should probably rename this to something more obvious / more in line with the CVS manual. > >+ my @cmdArgs; > >+ push(@cmdArgs, '-z3') if ($useCvsCompression); > > I thought we were wanting this to be the default now? > I guess we could, whether it's the default or not doesn't really matter imho. > >Index: Bootstrap/Step/Tag/l10n.pm > >=================================================================== > >RCS file: /cvsroot/mozilla/tools/release/Bootstrap/Step/Tag/l10n.pm,v > >retrieving revision 1.8 > >diff -u -r1.8 l10n.pm > >--- Bootstrap/Step/Tag/l10n.pm 27 Sep 2007 17:17:27 -0000 1.8 > >+++ Bootstrap/Step/Tag/l10n.pm 14 Nov 2007 13:10:56 -0000 > >@@ -56,28 +56,27 @@ > > > > # Check out the l10n files from the branch you want to tag. > > > >- my @l10nCheckoutArgs = (1 == $rc) ? > >+ my %l10nCheckoutArgs = (1 == $rc) ? > > # For rc1, pull by date on the branch > >- ('-r', $branchTag, '-D', $l10n_pullDate) : > >+ (branchTag => $branchTag, date => $l10n_pullDate) : > > # For rc(N > 1), pull the _RELBRANCH tag and tag that > >- ('-r', $geckoTag); > >+ (branchTag => $geckoTag); > > > > for my $locale (sort(keys(%{$localeInfo}))) { > > # skip en-US; it's kept in the main repo > > next if ($locale eq 'en-US'); > > > >- $this->Shell( > >- cmd => 'cvs', > >- cmdArgs => ['-d', $l10nCvsroot, 'co', @l10nCheckoutArgs, > >- CvsCatfile('l10n', $locale)], > >- dir => $l10nTagDir, > >- logFile => catfile($logDir, 'tag-l10n_checkout.log'), > >- ); > >+ $l10nCheckoutArgs{'cvsroot'} = $l10nCvsroot; > >+ $l10nCheckoutArgs{'modules'} = [CvsCatfile('l10n', $locale)]; > >+ $l10nCheckoutArgs{'workDir'} = $l10nTagDir; > >+ $l10nCheckoutArgs{'logFile'} = catfile($logDir, > >+ 'tag-l10n_checkout.log'); > >+ $this->CvsCo(%l10nCheckoutArgs); > > This is mostly a style-istic issue, but I would do the standard calling > convention for this (i.e. $this->CvsCo(cvsroot => ..., > workDir => ...). Yeah, you're right about this. I was using %l10nCheckoutArgs because it was already there and to work with the logic above the for loop. I can make this better, though.

I didn't change anything with regard to the compression, if you think it's important to make it the default I will do that though. Right now, I have useCvsCompression = 1 in my bootstrap.cfg's. The l10n.pm checkout is a bit shorter now, all of the logic is in the function call. I'm not sure how I feel about the ternary assignment in there. It seems fine now, but I'm worried it will be ugly and/or confusing in the future. What do you think?

The bootstrap.cfg has been bumped for 3.0b2, uses local CVS/ftp/stage mirrors, and reports to MozillaTest. The Tag step does not use the Makefile for make/clean stage (because the Makefile in CVS is 1.8 specific). ShellCommand's have been added that do the same thing, minus downloading previous builds/updates (and cleaning them out). This really has to be done manually right now since 3.0b1 isn't released, doesn't have the proper directory structure, etc. The Sign step is has been bumped for 3.0b2. Everything else should be the same. When checking these in I propose we move the existing configs to production-1.8 and staging-1.8 and put these in staging-trunk. (I expect that we'll have production-1.9 and staging-1.9 configs at some point). I think the masters will have to be updated if we do this.

Comment on attachment 289344 [details] [diff] [review] release automation on trunk configs, staging >diff -Naur empty/bootstrap.cfg staging-trunk/bootstrap.cfg >--- empty/bootstrap.cfg Wed Dec 31 19:00:00 1969 >+++ staging-trunk/bootstrap.cfg Mon Nov 19 12:06:51 2007 >+# manually tagged from GECKO181_20070712_RELBRANCH >+RelbranchOverride = GECKO190_20071108_RELBRANCH You probably don't want this set, it will pull this branch instead of pulling HEAD and creating a new one. All looks fine besides. What's up with http://www.flickr.com/photos/pmorgan/32606683/ ? :)

(In reply to comment #19) > (From update of attachment 289344 [details] [diff] [review]) > >diff -Naur empty/bootstrap.cfg staging-trunk/bootstrap.cfg > >--- empty/bootstrap.cfg Wed Dec 31 19:00:00 1969 > >+++ staging-trunk/bootstrap.cfg Mon Nov 19 12:06:51 2007 > >+# manually tagged from GECKO181_20070712_RELBRANCH > >+RelbranchOverride = GECKO190_20071108_RELBRANCH > > You probably don't want this set, it will pull this branch instead of pulling > HEAD and creating a new one. > > All looks fine besides. What's up with > http://www.flickr.com/photos/pmorgan/32606683/ ? :) > Uhhh..good question. I copied that css file from the staging 1.8 setup, heh. I'm going to update that.

After trying to run the previous patch without a RelbranchOverride I encountered problems with 1.9b1 style version numbers. This patch addresses that issue. It supports the 1.9.0.10 => 190 versions as before, but also 1.9b1 => 19B1 as well. I'll be running tests with it on 1.8 (which issues RelbranchOverride) and 1.9 (which no longer uses it) tonight.

Same as before with the following changes: * Comment out the RelbranchOverride in bootstrap.cfg * Fix the background image in mozilla.css * Do some stage cleaning in master.cfg

(In reply to comment #16) > > What is $echoFiles, and what's it used for? > > When true, checked out files will be piped to stdout. I should probably rename > this to something more obvious / more in line with the CVS manual. Right; what's the use case for this?

(In reply to comment #23) > (In reply to comment #16) > > > What is $echoFiles, and what's it used for? > > > > When true, checked out files will be piped to stdout. I should probably rename > > this to something more obvious / more in line with the CVS manual. > > Right; what's the use case for this? > This is used in Bootstrap::Util::GetLocaleManifest(). It's not used anywhere else, so it could be removed without breaking anything. I originally had CvsCo in Bootstrap::Util and converted all of the CVS checkouts (not just the ones in Bootstrap::Step::*, so this was necessary. I'm happy to remove it, or leave it.

New in this patch: * GetBouncerPlatforms() and the sort() of them happens in two steps; this is to accommodate MSYS Perl, which seems to parse "my @bouncerPlatforms = sort(GetBouncerPlatforms());" incorrectly; @bouncerPlatforms ends up being set to nothing. The folks in #perl on Freenode think that the sort() was being parsed with two arguments ('sort SUBNAME LIST'), rather than with the list being returned from GetBouncerPlatforms(). (Sorry for the ridiculous amount of new patches, I swear, it's almost there.)

Comment on attachment 290551 [details] [diff] [review] (forgot to save a file before i diff'ed) These changes look reasonable to me. Allow me to be pedantic and point out that HEAD doesn't mean the same thing as trunk, although CVS sometimes treats it as such, so you need to be careful with this assumption. For example "cvs up -r HEAD" is equivalent to "cvs up -A", but "cvs diff -r HEAD" will compare your current sources to the tip of the branch you are on. The only general comment I have is that it'd be easier to read if the refactoring (Shell to CvsCo) was done as a separate patch from the new features (trunk support). It's also better in terms of revision history and being able to safely back out changes, for instance.

(Just a minor fix to $linuxExtension -- it was using '.tar..$linuxExtension' in a couple of places.) If it's OK, I'm going to carry your r+ forward, Rob.

(In reply to comment #24) > This is used in Bootstrap::Util::GetLocaleManifest(). It's not used anywhere > else, so it could be removed without breaking anything. I originally had CvsCo > in Bootstrap::Util and converted all of the CVS checkouts (not just the ones in > Bootstrap::Step::*, so this was necessary. > > I'm happy to remove it, or leave it. Outputting the file to stdout is a relatively special case used (I think) only in GetLocaleManifest(); I'd take it out entirely, and replace the code in GetLocaleManifest(). Other than that, I think this is Ok. I agree with rhelmer that it's a pretty big patch, and I would've like to have seen it in multiple patches (a refactor patch, first, and a trunk support patch second). But, I'm not pedantic enough to make you redo it. :-)

This version removes the pipeToStdout option to CvsCo(). Only had to remove 3 lines, so I'm just going to carry review forward.

Checking in Bootstrap/Config.pm; /cvsroot/mozilla/tools/release/Bootstrap/Config.pm,v <-- Config.pm new revision: 1.15; previous revision: 1.14 done Checking in Bootstrap/Step.pm; /cvsroot/mozilla/tools/release/Bootstrap/Step.pm,v <-- Step.pm new revision: 1.16; previous revision: 1.15 done Checking in Bootstrap/Util.pm; /cvsroot/mozilla/tools/release/Bootstrap/Util.pm,v <-- Util.pm new revision: 1.11; previous revision: 1.10 done Checking in Bootstrap/Step/Build.pm; /cvsroot/mozilla/tools/release/Bootstrap/Step/Build.pm,v <-- Build.pm new revision: 1.21; previous revision: 1.20 done Checking in Bootstrap/Step/PatcherConfig.pm; /cvsroot/mozilla/tools/release/Bootstrap/Step/PatcherConfig.pm,v <-- PatcherConfig.pm new revision: 1.10; previous revision: 1.9 done Checking in Bootstrap/Step/Repack.pm; /cvsroot/mozilla/tools/release/Bootstrap/Step/Repack.pm,v <-- Repack.pm new revision: 1.20; previous revision: 1.19 done Checking in Bootstrap/Step/Source.pm; /cvsroot/mozilla/tools/release/Bootstrap/Step/Source.pm,v <-- Source.pm new revision: 1.12; previous revision: 1.11 done Checking in Bootstrap/Step/Stage.pm; /cvsroot/mozilla/tools/release/Bootstrap/Step/Stage.pm,v <-- Stage.pm new revision: 1.24; previous revision: 1.23 done Checking in Bootstrap/Step/Tag.pm; /cvsroot/mozilla/tools/release/Bootstrap/Step/Tag.pm,v <-- Tag.pm new revision: 1.17; previous revision: 1.16 done Checking in Bootstrap/Step/TinderConfig.pm; /cvsroot/mozilla/tools/release/Bootstrap/Step/TinderConfig.pm,v <-- TinderConfig.pm new revision: 1.5; previous revision: 1.4 done Checking in Bootstrap/Step/Updates.pm; /cvsroot/mozilla/tools/release/Bootstrap/Step/Updates.pm,v <-- Updates.pm new revision: 1.27; previous revision: 1.26 done Checking in Bootstrap/Step/Tag/Bump.pm; /cvsroot/mozilla/tools/release/Bootstrap/Step/Tag/Bump.pm,v <-- Bump.pm new revision: 1.11; previous revision: 1.10 done Checking in Bootstrap/Step/Tag/Talkback.pm; /cvsroot/mozilla/tools/release/Bootstrap/Step/Tag/Talkback.pm,v <-- Talkback.pm new revision: 1.6; previous revision: 1.5 done Checking in Bootstrap/Step/Tag/l10n.pm; /cvsroot/mozilla/tools/release/Bootstrap/Step/Tag/l10n.pm,v <-- l10n.pm new revision: 1.9; previous revision: 1.8 done Checking in configs/fx-moz18-bootstrap.cfg; /cvsroot/mozilla/tools/release/configs/fx-moz18-bootstrap.cfg,v <-- fx-moz18-bootstrap.cfg new revision: 1.25; previous revision: 1.24 done Checking in configs/fx-moz18-staging-bootstrap.cfg; /cvsroot/mozilla/tools/release/configs/fx-moz18-staging-bootstrap.cfg,v <-- fx-moz18-staging-bootstrap.cfg new revision: 1.8; previous revision: 1.7 done Checking in configs/fx-moz180-bootstrap.cfg; /cvsroot/mozilla/tools/release/configs/fx-moz180-bootstrap.cfg,v <-- fx-moz180-bootstrap.cfg new revision: 1.6; previous revision: 1.5 done Checking in configs/tb-moz18-bootstrap.cfg; /cvsroot/mozilla/tools/release/configs/tb-moz18-bootstrap.cfg,v <-- tb-moz18-bootstrap.cfg new revision: 1.12; previous revision: 1.11 done Checking in configs/tb-moz180-bootstrap.cfg; /cvsroot/mozilla/tools/release/configs/tb-moz180-bootstrap.cfg,v <-- tb-moz180-bootstrap.cfg new revision: 1.7; previous revision: 1.6 done

Updated to their current state. Forgot to check this in before closing this bug :(

RCS file: /cvsroot/mozilla/tools/buildbot-configs/automation/production-1.9/master.cfg,v done Checking in production-1.9/master.cfg; /cvsroot/mozilla/tools/buildbot-configs/automation/production-1.9/master.cfg,v <-- master.cfg initial revision: 1.1 done RCS file: /cvsroot/mozilla/tools/buildbot-configs/automation/staging-1.9/master.cfg,v done Checking in staging-1.9/master.cfg; /cvsroot/mozilla/tools/buildbot-configs/automation/staging-1.9/master.cfg,v <-- master.cfg initial revision: 1.1 done