Closed
Bug 372755
Opened 17 years ago
Closed 17 years ago
Respin support for Bootstrap
Categories
(Release Engineering :: General, defect, P2)
Release Engineering
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: preed, Assigned: preed)
References
()
Details
Attachments
(1 file, 1 obsolete file)
42.54 KB,
patch
|
rhelmer
:
review+
nthomas
:
review+
|
Details | Diff | Splinter Review |
We need to have a way to specify and kick off respins (RC N builds for N > 1) for Firefox/Thunderbird releases. Effectively, this involves specify which files at which revisions are part of a respin, cvs stat'ing, cvs up'ing the relevant files, cvs stat'ing the relevant files, cvs tag'ing the area with the appropriate _RCn tag, cvs tag -F'ing the _RELEASE tag, and then rekicking all the builds with the appropriate config for each of the platforms.
Assignee | ||
Comment 1•17 years ago
|
||
I've not heard any negative feedback about the proposal in my blog/the newsgroups (added to the URL field), so this is what I'll implement.
Assignee | ||
Updated•17 years ago
|
Assignee: build → preed
Assignee | ||
Comment 2•17 years ago
|
||
Here's the process I used for 2.0.0.4, yanked from .bash_history (relevant commands only): For cvsroot: 855 mkdir FIREFOX_2_0_0_4_RELEASE 856 cd FIREFOX_2_0_0_4_RELEASE/ 858 mkdir cvsroot/ 859 cd cvsroot/ 860 cvs co -rMOZILLA_1_8_BRANCH -D"2007-05-01 01:30 PDT" mozilla/client.mk 861 cd mozilla/ 862 gmake -f client.mk checkout MOZ_CO_PROJECT=all MOZ_CO_DATE="2007-05-01 01 :30 PDT" 2>&1 | tee ../make_checkout.log 863 cvs tag FIREFOX_2_0_0_4_RC1 2>&1 | tee ../FIREFOX_2_0_0_4_RC1.log 865 cvs tag -b GECKO181_20070501_RELBRANCH 2>&1 | tee ../GECKO181_20070501_RE LBRANCH.log 866 cvs up -rGECKO181_20070501_RELBRANCH 890 cvs tag FIREFOX_2_0_0_4_RELEASE 2>&1 | tee ../FIREFOX_2_0_0_4_RELEASE.log For mofo: 874 mkdir mofo 875 cd mofo/ 877 cvs -d :ext:cltbld@cvs.mozilla.org:/mofo co -rMOZILLA_1_8_BRANCH -D "200 7-05-01 01:30" talkback/fullsoft 2>&1 | tee checkout.log 878 cd talkback/fullsoft/ 880 cvs tag FIREFOX_2_0_0_4_RELEASE 2>&1 | tee ../../tag.log For l10n: 1001 mkdir l10n 1002 cd l10n/ 1009 (for l in $LOCALES; do cvs -d :ext:cltbld@cvs.mozilla.org:/l10n co -r MOZ ILLA_1_8_BRANCH -D "2007-04-28 00:00 PDT" l10n/$l; done) 2>&1 | tee checkout.log 1011 cd l10n/ 1013 cvs tag FIREFOX_2_0_0_4_RC1 2>&1 | tee ../FIREFOX_2_0_0_4_RC1.log 1016 cvs tag -b GECKO181_20070501_RELBRANCH 2>&1 | tee ../GECKO181_20070501_RE LBRANCH.log 1017 cvs up -rGECKO181_20070501_RELBRANCH 1019 cvs tag FIREFOX_2_0_0_4_RELEASE 2>&1 | tee ../FIREFOX_2_0_0_4_RELEASE.log Some notes: -- cf and I decided on a different _RELBRANCH name, since we needed to include the source branch (1.8.0, 1.8.1) to disambiguate in cases (such as this time) where we start releases on the same day. We also chose GECKO_, since the major point of this is bumping the Gecko version, so that seemed to make sense. -- It probably doesn't matter, but the _RC1 tag is playing the role of the a standard CVS _BASE tag; this makes a diff between _RC1 and _RELEASE when on RC1 actually output something (the version bumps on the branch), so... I'm not 100% sure I like that. It may not matter. -- As cf points out, there is one problem with doing this when release Tb and Fx: both need to munge client.mk with their _RELEASE tags; so what do we do? Branch again? Checkout/checkin again? None of the solutions seem great. We're discussing what to do about that last one.
Assignee | ||
Comment 3•17 years ago
|
||
For RC N > 1 respins, the steps (yanked from bash's history) were: 872 cd ~/tags/FIREFOX_2_0_0_4_RELEASE/ 881 cvs up -jGECKO181_20070501_RELBRANCH -jMOZILLA_1_8_BRANCH client.mk # Normally, this should cause no conflicts; on any _RELBRANCH files (version changes, client.mk changes, etc.) it will... 882 cvs di client.mk # See bonsai for checkin messages 885 cvs ci client.mk # Ensure that we get warnings (and only warnings) about not moving the tag 904 cvs tag FIREFOX_2_0_0_4_RELEASE 905 cvs tag -F FIREFOX_2_0_0_4_RELEASE 906 cvs tag FIREFOX_2_0_0_4_RC2 2>&1 | tee ../FIREFOX_2_0_0_4_RC2.log
Status: NEW → ASSIGNED
Comment 4•17 years ago
|
||
(In reply to comment #2) > For cvsroot: > > 855 mkdir FIREFOX_2_0_0_4_RELEASE > 856 cd FIREFOX_2_0_0_4_RELEASE/ > 858 mkdir cvsroot/ > 859 cd cvsroot/ > 860 cvs co -rMOZILLA_1_8_BRANCH -D"2007-05-01 01:30 PDT" mozilla/client.mk > 861 cd mozilla/ > 862 gmake -f client.mk checkout MOZ_CO_PROJECT=all MOZ_CO_DATE="2007-05-01 > 01 > :30 PDT" 2>&1 | tee ../make_checkout.log > 863 cvs tag FIREFOX_2_0_0_4_RC1 2>&1 | tee ../FIREFOX_2_0_0_4_RC1.log > 865 cvs tag -b GECKO181_20070501_RELBRANCH 2>&1 | tee > ../GECKO181_20070501_RE > LBRANCH.log > 866 cvs up -rGECKO181_20070501_RELBRANCH Somewhere about here I committed the changes to client.mk, config/milestone.txt, browser/app/module.ver, and browser/config/version.txt on GECKO181_20070501_RELBRANCH. > 890 cvs tag FIREFOX_2_0_0_4_RELEASE 2>&1 | tee > ../FIREFOX_2_0_0_4_RELEASE.log Which meant that the RC1 tag ended up on the wrong files, but I fixed this manually after the fact. I hadn't seen ... > -- It probably doesn't matter, but the _RC1 tag is playing the role of the a > standard CVS _BASE tag; this makes a diff between _RC1 and _RELEASE when on RC1 > actually output something (the version bumps on the branch), so... I'm not 100% > sure I like that. It may not matter. I think this might bite other people using the tag, and it'd be preferable to have RC1 tag be the actual code, instead of some special not-quite-but-almost-the-same version. We're fairly likely to forget the difference at some point. > -- As cf points out, there is one problem with doing this when release Tb and > Fx: both need to munge client.mk with their _RELEASE tags; so what do we do? > Branch again? Checkout/checkin again? None of the solutions seem great. > > We're discussing what to do about that last one. We ended up making a _MINIBRANCH for client.mk off GECKO181_20070501_RELBRANCH for Tb.
Assignee | ||
Comment 5•17 years ago
|
||
Take 1 of the Respin support patch; consider this a WIP. I've tested it, but I need to test a couple more RC > 1 cases; but, it's a big patch, so I wanted to get some eyes on it. This patch: -- Adds a Config::Set method, so the automation can set variables for itself (not just out of a config file) -- Adds a default logfile to Step::Shell, in case the caller doesn't specify one -- Clean up args handling of Step::Shell -- Clean up ordering of use statements for all Tagging steps; use strict on all of them, too -- Add logic for this concept of a "Gecko release tag" (it's called geckoTag here; may rename it for clarity) -- As for the other Tag substeps, basically implement the steps outlined in this bug's previous comments.
Attachment #265276 -
Flags: review?(nrthomas)
Assignee | ||
Updated•17 years ago
|
Attachment #265276 -
Flags: review?(rhelmer)
Comment 6•17 years ago
|
||
Comment on attachment 265276 [details] [diff] [review] Take I (WIP) Looks good to me, based on a quick review. (Clearing review request instead of +ing since this isn't the final patch.) Will the cvs up in l10n.pm fail for rc2 and later ? Seems that it's trying to update an empty dir, as releaseTagDir is symlinked to rcTagDir.
Attachment #265276 -
Flags: review?(nrthomas)
Comment 7•17 years ago
|
||
Comment on attachment 265276 [details] [diff] [review] Take I (WIP) >Index: Bootstrap/Step/Tag/Bump.pm >=================================================================== >RCS file: /cvsroot/mozilla/tools/release/Bootstrap/Step/Tag/Bump.pm,v >retrieving revision 1.7 >diff -u -8 -r1.7 Bump.pm >--- Bootstrap/Step/Tag/Bump.pm 30 Apr 2007 20:55:22 -0000 1.7 >+++ Bootstrap/Step/Tag/Bump.pm 18 May 2007 17:09:39 -0000 (snip) >+ ## TODO - we need to handle the case here where we're in security firedrill >+ ## mode, and we need to bump versions on the GECKO_ branch, but they >+ ## won't have "pre" in them. :-o >+ # This is a good point.. should we have a "firedrill" (or maybe more clear name) mode, specified in bootstrap.cfg? >Index: Bootstrap/Step/Tag/Mozilla.pm >=================================================================== >RCS file: /cvsroot/mozilla/tools/release/Bootstrap/Step/Tag/Mozilla.pm,v >retrieving revision 1.2 >diff -u -8 -r1.2 Mozilla.pm >--- Bootstrap/Step/Tag/Mozilla.pm 7 Feb 2007 22:14:23 -0000 1.2 >+++ Bootstrap/Step/Tag/Mozilla.pm 18 May 2007 17:09:39 -0000 (snip) >+ # TODO: should this complain about W's? > foreach my $tag ($releaseTag, $rcTag) { > $this->CheckLog( > log => catfile($logDir, 'tag-mozilla_cvsroot_tag-' . $tag . '.log'), > checkFor => '^T', > ); > } > } Yes, it really should.. If you add this, note that in other places, I've done a nested foreach does each CheckLog separately to look for disallowed characters; if you want to make checkFor take arrays for both "log" and "checkFor" that would be a fine thing to do as well...
Attachment #265276 -
Flags: review?(rhelmer) → review+
Comment 8•17 years ago
|
||
needed for "release automation", hence marking as critical.
Severity: normal → critical
Updated•17 years ago
|
Priority: -- → P1
Assignee | ||
Comment 9•17 years ago
|
||
So, after tons of testing, this patch actually does a real respin and handles a bunch of corner cases. Interdiff will be useful here; changes between take I and take II: Bootstrap/Config.pm -- Change some of the parsing semantics for the config file -- Add some assert()ions to the Set() method Bootstrap/Step/Tag.pm -- Remove all the symlinking garbage; it's not useful and the error checking was annoying to do -- Move the code for the Verify() method into the Execute method, since a) it's different for RC1 vs. RC>1, b) it doesn't really verify all that much anyway, and c) it gets called after all the substeps get called, and a failure in what it's verifying would affect the substeps anyway. -- A lot of whitespace/assert()ion message reformatting -- Reordered/wrote some of the CvsTag assert()tions; they were asserting the wrong things. -- Remove -q in the call to "cvs tag" -- Add a way to override the _RELBRANCH name in the config, if it's needed. -- Add a new GetDiffFileList() method; give it a CVS directory and two tags, and it will return the files containing differences; this maybe should go somewhere else (MozBuild::Util?) if it's useful to others. Bootstrap/Step/Tag/Bump.pm -- Skip the bump step altogether for rc > 1 Bootstrap/Step/Tag/Mozilla.pm -- Remove superfluous calls to Config::Get() (i.e. variables we don't even use) -- Add a "we modified the _RELEASE tag flag," so Verify() knows what to try to verify -- Add a call to GetDiffFileList(), so we can pass a list of files that changed with the force option to CvsTag(). -- Add logic to not modify the _RELEASE tag if we're respinning in a way that doesn't affect the cvsroot module Bootstrap/Step/Tag/Talkback.pm -- Remove superfluous calls to Config::Get() (i.e. variables we don't even use) -- Never tag talkback for rc > 1. Bootstrap/Step/Tag/l10n.pm -- Mostly the same fixes for the Mozilla.pm substep MozBuild::Util: -- The child started/ended time was being given as a string (ret val from localtime()) instead of a seconds-since-epoch This patch actually did a complete run; I can post the full log (it's kinda big) if that'd be helpful to reviewers.
Attachment #265276 -
Attachment is obsolete: true
Attachment #270382 -
Flags: review?(rhelmer)
Assignee | ||
Updated•17 years ago
|
Attachment #270382 -
Flags: review?(nrthomas)
Assignee | ||
Comment 10•17 years ago
|
||
Logs for a run of RC1 and an RC2 run (with some changes to files in cvsroot) are available: RC1: http://people.mozilla.com/~preed/respin-test-rc1.log RC2: http://people.mozilla.com/~preed/respin-test-rc2.log Careful; they're both big (> 5 megs)
Assignee | ||
Comment 11•17 years ago
|
||
Sorry for the bugspam; these are now P2 in the New View of the World (tm).
Priority: P1 → P2
Comment 12•17 years ago
|
||
Comment on attachment 270382 [details] [diff] [review] Take II (actually working patch) Config->Set() makes me think it'd actually be saved to the config; I don't think we'll ever want to actually do that though so it's probably fine. We also already do things like the "sysvar" magic, so it's not like the config file and config object have a 1:1 mapping everywhere else. Anyway looks fine to me, that's the only thing that jumped out at me. Thanks for posting the logs, that helped me to understand the flow.
Attachment #270382 -
Flags: review?(rhelmer) → review+
Assignee | ||
Comment 13•17 years ago
|
||
(In reply to comment #12) > (From update of attachment 270382 [details] [diff] [review]) > Config->Set() makes me think it'd actually be saved to the config; I don't > think we'll ever want to actually do that though so it's probably fine. > > We also already do things like the "sysvar" magic, so it's not like the config > file and config object have a 1:1 mapping everywhere else. We talked about this in IRC, but I wanted to update: So, Set(), to me (as I wrote it) sets a variable/value pair in the config object; it doesn't (to me) imply that it gets written to disk or is stateful (something like a Save() or Write() method would, however). You'll note that I use Set() in the... constructor (or is it Read()) method, to set the instances out of the file itself. As you point out, it's basically a way to (cleanly) do glorified global variables, but it adds some safety (you can't overwrite values unless you really want to, for instance). Here, I *use* Set() to set variables that are completely derivable from other sources, but which other (sub-, actually)steps need access to. So, that's the design thinking there. I'll let cf take a gander, and then check it in. > > Anyway looks fine to me, that's the only thing that jumped out at me. Thanks > for posting the logs, that helped me to understand the flow. >
Comment 14•17 years ago
|
||
Comment on attachment 270382 [details] [diff] [review] Take II (actually working patch) woot! Just a few niggles ... >Index: Bootstrap/Config.pm >=================================================================== ... >+sub Set { >+ my $this = shift; >+ >+ my %args = @_; >+ >+ die "ASSERT: Config::Set(): null var and/or value\n" if >+ (!exists($args{'var'}) || !exists($args{'value'})); >+ >+ die "ASSERT: Config::Set(): Cannot set null value\n" if >+ (!defined($args{'var'}) || >+ (defined($args{'var'}) && $args{'var'} =~ /^\s*$/)); s/value/var/ in ASSERT message ? >Index: Bootstrap/Step/Tag/Bump.pm >=================================================================== I noticed something not related to these changes. Of the checkout tags in client.mk, only MOZ_CO_TAG is updated, while manually we have also done LDAPCSDK_CO_TAG and LOCALES_CO_TAG. Is this a bug ? Don't think this is set elsewhere (mozconfig etc). > sub Verify { ... >+ # TODO: should this complain about W's? >+ foreach my $tag (@checkTags) { > $this->CheckLog( > log => catfile($logDir, 'tag-mozilla_cvsroot_tag-' . $tag . '.log'), > checkFor => '^T', Can you use checkForOnly here, and for other tagging verification ? If I'm reading CheckLog() right, you only need a single ^T line to pass with checkFor. >Index: Bootstrap/Step/Tag/l10n.pm >=================================================================== ... >sub Execute { ... > my $releaseTag = $productTag.'_RELEASE'; > my $rcTag = $productTag.'_RC'.$rc; nit, missed one case of padding around the .
Attachment #270382 -
Flags: review?(nrthomas) → review+
Assignee | ||
Comment 15•17 years ago
|
||
(In reply to comment #14) > s/value/var/ in ASSERT message ? Fixed. > I noticed something not related to these changes. Of the checkout tags in > client.mk, only MOZ_CO_TAG is updated, while manually we have also done > LDAPCSDK_CO_TAG and LOCALES_CO_TAG. Is this a bug ? Don't think this is set > elsewhere (mozconfig etc). Ooh, good catch; this is actually a regression from the fix for bug 372759; I'll get a new bug filed to fix it. > Can you use checkForOnly here, and for other tagging verification ? If I'm > reading CheckLog() right, you only need a single ^T line to pass with checkFor. Yah, I actually need to do a second pass here to clean up the log files/verification for this step. (That's why I put in TODO notes :-) > nit, missed one case of padding around the . Fixed. Thanks Nick.
Assignee | ||
Comment 16•17 years ago
|
||
Landed.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Product: mozilla.org → Release Engineering
You need to log in
before you can comment on or make changes to this bug.
Description
•