Closed Bug 372755 Opened 17 years ago Closed 17 years ago

Respin support for Bootstrap

Categories

(Release Engineering :: General, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: preed, Assigned: preed)

References

()

Details

Attachments

(1 file, 1 obsolete file)

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.
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: build → preed
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.
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
(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. 
Attached patch Take I (WIP) (obsolete) — Splinter Review
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)
Attachment #265276 - Flags: review?(rhelmer)
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 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+
needed for "release automation", hence marking as critical.
Severity: normal → critical
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)
Attachment #270382 - Flags: review?(nrthomas)
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)
Sorry for the bugspam; these are now P2 in the New View of the World (tm).
Priority: P1 → P2
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+
(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 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+
(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.
Landed.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.