372759 - Bootstrap - polish milestone bumping logic

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[J. Paul Reed [:preed]]

Since we now bump versions on the _MINIBRANCH (bug 369825), we need a way to specify either pulling the milestone version from another _RELEASE tag (in the case of Thunderbird 1.5.0.n builds needing the same version as Firefox 1.5.0.n builds), or a way to not bump it at all (in the case of Thunderbird 2.0bN builds needing whatever's currently on the branch).

Comment 1

[Robert Helmer [:rhelmer]]

Attachment: if milestone is set in config, use it; otherwise skip that file

Comment 2

[Robert Helmer [:rhelmer]]

Comment on attachment 259760 [details] [diff] [review]
if milestone is set in config, use it; otherwise skip that file

client.mk needs to be moved into this loop; otherwise it'll try to s/$versionPre/$version/ when it should s/$branchTag/$version/

Comment 3

[Robert Helmer [:rhelmer]]

Attachment: only bump milestone if specified

This works on the staging environment. The bump logic is generalized and (hopefully) easier to understand now.

Comment 4

[J. Paul Reed [:preed]]

Comment on attachment 260363 [details] [diff] [review]
only bump milestone if specified


>+        my $bumpVersion = undef;
>+        my $preVersion = undef;
>+
>+        if ($fileName eq $milestoneTxt) {
>+            $bumpVersion = $milestone;
>+            $preVersion = $bumpVersion . 'pre';
>+        } elsif ($fileName eq 'client.mk') {
>+            $bumpVersion = $version;
>+            $preVersion = $branchTag;

Shouldn't $bumpVersion be $productTag in the client.mk case? Wouldn't this just check in a file with a tag of "2.0.0.3" or whatever?

>+        } else {
>+            $bumpVersion = $version;
>+            $preVersion = $bumpVersion . 'pre';
>+        }

For this, I think:

if ($milestoneTxt) {
   ...
} elsif ($clientMk) {
   ...
} elsif ($bar) {
   ...
} else {
  die "ASSERT";
}

Is more appropriate; it would be way too easy to add something to @bumpFiles, but never create the proper logic for it.

The test of the substitution is could be tightened up, too; we know, for instance, that if the file is named something specific, what exactly to bump. We could constrain the regexes to be safer.

Did this patch get run through the test gauntlet?

Comment 5

[Robert Helmer [:rhelmer]]

(In reply to comment #4)
> (From update of attachment 260363 [details] [diff] [review])
> 
> >+        my $bumpVersion = undef;
> >+        my $preVersion = undef;
> >+
> >+        if ($fileName eq $milestoneTxt) {
> >+            $bumpVersion = $milestone;
> >+            $preVersion = $bumpVersion . 'pre';
> >+        } elsif ($fileName eq 'client.mk') {
> >+            $bumpVersion = $version;
> >+            $preVersion = $branchTag;
> 
> Shouldn't $bumpVersion be $productTag in the client.mk case? Wouldn't this just
> check in a file with a tag of "2.0.0.3" or whatever?


Ack! Excellent catch, that's true.


> >+        } else {
> >+            $bumpVersion = $version;
> >+            $preVersion = $bumpVersion . 'pre';
> >+        }
> 
> For this, I think:
> 
> if ($milestoneTxt) {
>    ...
> } elsif ($clientMk) {
>    ...
> } elsif ($bar) {
>    ...
> } else {
>   die "ASSERT";
> }
> 
> Is more appropriate; it would be way too easy to add something to @bumpFiles,
> but never create the proper logic for it.


Ok, will do.


> The test of the substitution is could be tightened up, too; we know, for
> instance, that if the file is named something specific, what exactly to bump.
> We could constrain the regexes to be safer.


True, I'll see what I can do.


> Did this patch get run through the test gauntlet?


It did, but just the Tag step on the staging server; I didn't try a build off of it (waiting on bug 371325 to be able to do it for real).

Comment 6

[Robert Helmer [:rhelmer]]

Attachment: address preed's comments

Ok, same as last time, plus your suggestions. I tested this and went over the output more carefully, looks like everything is bumped properly.

Comment 7

[J. Paul Reed [:preed]]

Comment on attachment 260658 [details] [diff] [review]
address preed's comments


>+    foreach my $fileName (@bumpFiles) {
>         my $found = 0;
>+
>+        my $file = catfile($parentDir, $fileName);
>+
>+        my $bumpVersion = undef;
>+        my $preVersion = undef;
>+        my $search = undef;
>+        my $replace = undef;
>+
>+        if ($fileName eq 'client.mk') {
>+            $search = '^MOZ_CO_TAG\s+=\s+' . $branchTag . '$';
>+            $replace = 'MOZ_CO_TAG           = ' . $releaseTag;
>+        } elsif ($fileName eq $moduleVer) {
>+            $preVersion = $version . 'pre';
>+            $search = '^WIN32_MODULE_PRODUCTVERSION_STRING=' . $preVersion . '$';
>+            $replace = 'WIN32_MODULE_PRODUCTVERSION_STRING=' . $version;
>+        } elsif ($fileName eq $versionTxt) {
>+            $preVersion = $version . 'pre';
>+            $search = '^' . $preVersion . '$';
>+            $replace = $version;
>+        } elsif ($fileName eq $milestoneTxt) {
>+            $preVersion = $milestone . 'pre';
>+            $search = '^' . $preVersion . '$';
>+            $replace = $milestone;
>+        } else {
>+            die("ASSERT: do not know how to bump file $fileName");
>+        }

This won't accidentally clobber the \n on the end of each of these lines, will it?

I don't think matching against end of line will clobber it in a search/replace, but you'll want to check the automated checkins that were done here to make sure.

Other than that, r=preed.

Comment 8

[Robert Helmer [:rhelmer]]

Made sure that the newline is left alone, and landed:

Checking in Bootstrap/Step/Tag/Bump.pm;
/cvsroot/mozilla/tools/release/Bootstrap/Step/Tag/Bump.pm,v  <--  Bump.pm
new revision: 1.6; previous revision: 1.5
done