Bootstrap - polish milestone bumping logic

RESOLVED FIXED

Status

RESOLVED FIXED
12 years ago
5 years ago

People

(Reporter: preed, Assigned: rhelmer)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

12 years ago
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).
(Assignee)

Updated

12 years ago
Assignee: build → rhelmer
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

12 years ago
Created attachment 259760 [details] [diff] [review]
if milestone is set in config, use it; otherwise skip that file
Attachment #259760 - Flags: review?(preed)
(Assignee)

Comment 2

12 years ago
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/
Attachment #259760 - Attachment is obsolete: true
Attachment #259760 - Flags: review?(preed)
(Assignee)

Comment 3

12 years ago
Created attachment 260363 [details] [diff] [review]
only bump milestone if specified

This works on the staging environment. The bump logic is generalized and (hopefully) easier to understand now.
Attachment #260363 - Flags: review?(preed)
(Reporter)

Comment 4

12 years ago
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?
Attachment #260363 - Flags: review?(preed) → review-
(Assignee)

Comment 5

12 years ago
(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).
(Reporter)

Updated

12 years ago
Blocks: 375788
(Assignee)

Comment 6

12 years ago
Created attachment 260658 [details] [diff] [review]
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.
Attachment #260363 - Attachment is obsolete: true
Attachment #260658 - Flags: review?(preed)
(Reporter)

Comment 7

12 years ago
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.
Attachment #260658 - Flags: review?(preed) → review+
(Assignee)

Comment 8

12 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 12 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.