387426 - Bootstrap Tag::Bump substep fails to update all relevant tags in client.mk

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P1)

Description

[J. Paul Reed [:preed]]

During the review for the final respin support patch (bug 372755), cf pointed out that the Bump substep doesn't correctly bump the tag for all of the tags in client.mk:

http://lxr.mozilla.org/mozilla/source/tools/release/Bootstrap/Step/Tag/Bump.pm#96

This code bumps MOZ_CO_TAG, but doesn't bump NSPR_CO_TAG, or the other _CO_TAGs; it used to. This regressed in version 1.6, with the fix for bug 372759:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/tools/release/Bootstrap/Step/Tag&command=DIFF&root=/cvsroot&file=Bump.pm&rev1=1.5&rev2=1.6#8

Comment 1

[Robert Helmer [:rhelmer]]

I agree with comment #0. 
For MOZILLA_1_8_BRANCH these tags look like this:
MOZ_CO_TAG           = MOZILLA_1_8_BRANCH
NSPR_CO_TAG          = NSPR_4_6_7_RTM
NSS_CO_TAG           = NSS_3_11_5_WITH_CKBI_1_64_RTM
LDAPCSDK_CO_TAG      = MOZILLA_1_8_BRANCH
LOCALES_CO_TAG       = MOZILLA_1_8_BRANCH

So, both before and after this regression, NSPR_CO_TAG and NSS_CO_TAG would not have been bumped. The right code will be tagged though so I'm not sure that it matters.

I'll do a patch for LOCALES_CO_TAG and LDAPCSDK_CO_TAG as those are easy, but should we do anything about NSPR_CO_TAG and NSS_CO_TAG?

To clarify, the regression is because we got more careful and don't just do "s/$branchTag/$releaseTag/g" on the whole client.mk, we look explicitly for MOZ_CO_TAG, and do the search/replace only on that line. 

We could do this for NSPR_CO_TAG and NSS_CO_TAG but we'd have to just clobber the line instead of the search/replace.

Comment 2

[J. Paul Reed [:preed]]

(In reply to comment #1)

> I'll do a patch for LOCALES_CO_TAG and LDAPCSDK_CO_TAG as those are easy, but
> should we do anything about NSPR_CO_TAG and NSS_CO_TAG?

Yes, we should move both of those to the appropriate product _RELEASE tag.

Comment 3

[Robert Helmer [:rhelmer]]

Attachment: (untested) bump more than one value per file

The previous implementation could only bump one value per file; this uses arrays for search/replace instead of strings.

Note - untested, just want to get general feedback on this approach, before I spend a bunch of time testing :)

Comment 4

[J. Paul Reed [:preed]]

Comment on attachment 271787 [details] [diff] [review]
(untested) bump more than one value per file


>-        my $search = undef;
>-        my $replace = undef;
>+        my @search = undef;
>+        my @replace = undef;

This approach is OK, but if you're gonna do this, can you use a hash table.

The reason is that if you don't, you end up having to assert below (you'll see) that scalar(@search) > 0, scalar(@replace) > 0, and scalar(@search) == scalar(@replace).

> 
>         if ($fileName eq 'client.mk') {
>-            $search = '^MOZ_CO_TAG\s+=\s+' . $branchTag . '$';
>-            $replace = 'MOZ_CO_TAG           = ' . $releaseTag;
>+            @search = ('^MOZ_CO_TAG\s+=\s+' . $branchTag . '$',
>+                       '^LDAPCSDK_CO_TAG\s+=\s+' . $branchTag . '$',
>+                       '^NSPR_CO_TAG\s+=\s+*',
>+                       '^NSS_CO_TAG\s+=\s+*',
>+                       '^LOCALES_CO_TAG\s+=\s+' . $branchTag . '$');
>+            @replace = ('MOZ_CO_TAG           = ' . $releaseTag,
>+                        'NSPR_CO_TAG          = ' . $releaseTag,
>+                        'NSS_CO_TAG           = ' . $releaseTag,
>+                        'LDAPCSDK_CO_TAG           = ' . $releaseTag,
>+                        'LOCALES_CO_TAG           = ' . $releaseTag);
>         } elsif ($fileName eq $moduleVer) {
>             $preVersion = $version . 'pre';
>-            $search = '^WIN32_MODULE_PRODUCTVERSION_STRING=' . $preVersion . '$';
>-            $replace = 'WIN32_MODULE_PRODUCTVERSION_STRING=' . $version;
>+            @search = ('^WIN32_MODULE_PRODUCTVERSION_STRING=' . $preVersion . '$');
>+            @replace = ('WIN32_MODULE_PRODUCTVERSION_STRING=' . $version);
>         } elsif ($fileName eq $versionTxt) {
>             $preVersion = $version . 'pre';
>-            $search = '^' . $preVersion . '$';
>-            $replace = $version;
>+            @search = ('^' . $preVersion . '$');
>+            @replace = ($version);
>         } elsif ($fileName eq $milestoneTxt) {
>             $preVersion = $milestone . 'pre';
>-            $search = '^' . $preVersion . '$';
>-            $replace = $milestone;
>+            @search = ('^' . $preVersion . '$');
>+            @replace = ($milestone);
>         } else {
>             die("ASSERT: do not know how to bump file $fileName");
>         }
>@@ -114,11 +122,15 @@
>         open(INFILE,  "< $file") or die("Could not open $file: $!");
>         open(OUTFILE, "> $file.tmp") or die("Could not open $file.tmp: $!");

I'd assert() somewhere in here that scalar(keys(%hash)) (see above) > 0.

>         while(<INFILE>) {
>-            if($_ =~ /$search/) {
>-                $this->Log(msg => "$search found");
>-                $found = 1;
>-                $_ =~ s/$search/$replace/;
>-                $this->Log(msg => "$search replaced with $replace");
>+            for (my $i=0; $i < $#search; $i++) {

Please use "for (my $i = 0; $i < scalar(@search); $i++)."

It's more readable, and the above code has the oft-made off-by-one-error (it's $i <= if you're going to use $#search, which is why I never use that construct).

>+                my $s = $search[$i];
>+                my $r = $replace[$i];
>+                if($_ =~ /$s/) {
>+                    $this->Log(msg => "$s found");
>+                    $found = 1;
>+                    $_ =~ s/$s/$r/;
>+                    $this->Log(msg => "$s replaced with $r");
>+                }
>             }
> 
>             print OUTFILE $_;
>@@ -126,7 +138,7 @@
>         close INFILE or die("Could not close $file: $!");
>         close OUTFILE or die("Coule not close $file.tmp: $!");
>         if (not $found) {
>-            die("No " . $search . " found in file $file: $!");
>+            die("None of " . @search . " found in file $file: $!");

Should be die("None of " . join(' ', @search) . ...) (actually, join(' ', keys(%hash)), now ;-)

Make those changes and r=preed.

Comment 5

[Robert Helmer [:rhelmer]]

(In reply to comment #4)
> (From update of attachment 271787 [details] [diff] [review])
> 
> >-        my $search = undef;
> >-        my $replace = undef;
> >+        my @search = undef;
> >+        my @replace = undef;
> 
> This approach is OK, but if you're gonna do this, can you use a hash table.
> 
> The reason is that if you don't, you end up having to assert below (you'll see)
> that scalar(@search) > 0, scalar(@replace) > 0, and scalar(@search) ==
> scalar(@replace).

Yes I would rather use a hash, but what should the key be (key="search", value="replace"?) Or did you mean use e.g. %search and %replace and e.g. unique id for the keys?

I think that key="search" and value="replace" would be better, then the "for (...)" would be a "foreach my $s (keys(%hash))".. maybe this is what you meant, but your comment about the "for" confused me if so :)

Comment 6

[J. Paul Reed [:preed]]

(In reply to comment #5)
> I think that key="search" and value="replace" would be better, then the "for
> (...)" would be a "foreach my $s (keys(%hash))".. maybe this is what you meant,
> but your comment about the "for" confused me if so :)

Yeah, sorry about that; if you used a %searchReplace{$search} = $replace model, then you could use a foreach if order doesn't matter.

I don't think it does the way we're using it (and I would comment that when you set keys/values,i.e., above this block:

>+            @search = ('^MOZ_CO_TAG\s+=\s+' . $branchTag . '$',
>+                       '^LDAPCSDK_CO_TAG\s+=\s+' . $branchTag . '$',
>+                       '^NSPR_CO_TAG\s+=\s+*',
>+                       '^NSS_CO_TAG\s+=\s+*',
>+                       '^LOCALES_CO_TAG\s+=\s+' . $branchTag . '$');
>+            @replace = ('MOZ_CO_TAG           = ' . $releaseTag,
>+                        'NSPR_CO_TAG          = ' . $releaseTag,
>+                        'NSS_CO_TAG           = ' . $releaseTag,
>+                        'LDAPCSDK_CO_TAG           = ' . $releaseTag,
>+                        'LOCALES_CO_TAG           = ' . $releaseTag);

say "Order or searching for these values is not preserved, so make sure order doesn't matter" (or something to that effect).

The above code block would become (regexes are just examples):

%searchHash = ('^NSPR_CO_TAG\s*blah' => 'NSPR_CO_TAG=whee!',
               '^FOO_CO_TAG\s*blorp' => 'FOO_CO_TAG=bl'
               ...);

Make more sense?

(Sorry, I reviewed the code before having eaten dinner. :-)

Comment 7

[Robert Helmer [:rhelmer]]

Attachment: use hash not arrays, plus other suggestions

Output from the test:
log: Starting time is 23:03:44 07/10/07
log: Logging output to /builds/release/logs/tag-bump_checkout.log
log: Timeout: 3600
log: Ending time is 23:03:49 07/10/07
log: ^MOZ_CO_TAG\s+=\s+MOZILLA_1_8_BRANCH$ found
log: ^MOZ_CO_TAG\s+=\s+MOZILLA_1_8_BRANCH$ replaced with MOZ_CO_TAG           = FIREFOX_2_0_0_4_RELEASE
log: ^NSPR_CO_TAG\s+=\s+\w* found
log: ^NSPR_CO_TAG\s+=\s+\w* replaced with NSPR_CO_TAG          = FIREFOX_2_0_0_4_RELEASE
log: ^NSS_CO_TAG\s+=\s+\w* found
log: ^NSS_CO_TAG\s+=\s+\w* replaced with NSS_CO_TAG           = FIREFOX_2_0_0_4_RELEASE
log: ^LDAPCSDK_CO_TAG\s+=\s+MOZILLA_1_8_BRANCH$ found
log: ^LDAPCSDK_CO_TAG\s+=\s+MOZILLA_1_8_BRANCH$ replaced with LDAPCSDK_CO_TAG           = FIREFOX_2_0_0_4_RELEASE
log: ^LOCALES_CO_TAG\s+=\s+MOZILLA_1_8_BRANCH$ found
log: ^LOCALES_CO_TAG\s+=\s+MOZILLA_1_8_BRANCH$ replaced with LDAPCSDK_CO_TAG           = FIREFOX_2_0_0_4_RELEASE
log: ^WIN32_MODULE_PRODUCTVERSION_STRING=2.0.0.4pre$ found
log: ^WIN32_MODULE_PRODUCTVERSION_STRING=2.0.0.4pre$ replaced with WIN32_MODULE_PRODUCTVERSION_STRING=2.0.0.4
log: ^2.0.0.4pre$ found
log: ^2.0.0.4pre$ replaced with 2.0.0.4
log: ^1.8.1.4pre$ found
log: ^1.8.1.4pre$ replaced with 1.8.1.4

Comment 8

[Nick Thomas [:nthomas] (UTC+12)]

Heckling from the sidelines ....

+             '^LOCALES_CO_TAG\s+=\s+' . $branchTag . '$' =>
+              'LDAPCSDK_CO_TAG           = ' . $releaseTag,
+             '^LDAPCSDK_CO_TAG\s+=\s+' . $branchTag . '$' =>
+              'LDAPCSDK_CO_TAG           = ' . $releaseTag);

Should be LOCALES_CO_TAG in the first value, and it would be nice to align the = with the previous three pairs.

Comment 9

[Robert Helmer [:rhelmer]]

Attachment: fix typo caught by cf

Thanks for catching that Nick :) I aligned on " = " to make it easier to read, as you suggested.

Comment 10

[J. Paul Reed [:preed]]

Comment on attachment 271849 [details] [diff] [review]
fix typo caught by cf

Looks good; one minor (mostly readability) nit:

>+
>+        if (! scalar(keys(%searchReplace)) > 0) {
>+            die("ASSERT: no search/replace to perform");
>+        }

I'd write this if (scalar(keys(%searchReplace)) <= 0) { ... }

Looks good though!

Comment 11

[Robert Helmer [:rhelmer]]

Attachment: patch as landed

Comment 12

[Robert Helmer [:rhelmer]]

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