379380 - TinderConfig step should handle no whitespace between variables

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

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

I hit this problem while bumping tinder-config.pl for l10n builds. We have
 # CONFIG:         "%l10n_buildDir%/%l10n_buildPlatform%/firefox.zip"
which became
 "%l10n_buildDir%/WINNT_5.2_Depend/firefox.zip"

So only one substitution was done, and leading spaces were removed. The basic problem is that initial regexp is greedy, and the code doesn't allow for the possibility of more than one variable to change.

Here's a very rough (and not very elegant) guess at a fix:

Index: Bootstrap/Config.pm
===================================================================
RCS file: /cvsroot/mozilla/tools/release/Bootstrap/Config.pm,v
retrieving revision 1.7
diff -u -r1.7 Config.pm
--- Bootstrap/Config.pm 20 Apr 2007 18:55:24 -0000      1.7
+++ Bootstrap/Config.pm 1 May 2007 19:22:26 -0000
@@ -204,18 +204,16 @@
             print OUTFILE $line;
             $skipNextLine = 1;
             my $interpLine = $line;
-            $interpLine =~ s/^#\s+CONFIG:\s+//;
-            foreach my $variable (grep(/%[\w\-]+%/, split(/\s+/, $line))) {
+            $interpLine =~ s/^#\s+CONFIG:\s?//;
+            foreach my $variable (grep(/%/,split(/(%[\w\-]+?%)/, $l))) {
                 my $key = $variable;
-                if (! ($key =~ s/.*%([\w\-]+)%.*/$1/)) {
-                    die("ASSERT: could not parse $variable");
-                }
+                $key =~ s/^%(.*)%$/$1/;

                 if (! $config->Exists(sysvar => $key)) {
                     die("ASSERT: no replacement found for $key");
                 }
                 my $value = $config->Get(sysvar => $key);
-                $interpLine =~ s/\%$key\%/$value/g;
+                $interpLine =~ s/$variable/$value/g;
             }
             print OUTFILE $interpLine;
         } else {

I'm open to suggestions from perl people on how better to fix this. For extra points, we need only loop over only unique %vars%.

Comment 1

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

Rob points out that 
 # CONFIG: %foo% %bar%
works and I'm really talking about
 # CONFIG: %foo%/$bar%

In fact any non-whitespace between variables.

Comment 2

[Robert Helmer [:rhelmer]]

Attachment: nick's patch to allow non-whitespace-separated variables

Nick's patch looks pretty good to me, and passes all the tests I've tried. What do you think?

Comment 3

[J. Paul Reed [:preed]]

Comment on attachment 263649 [details] [diff] [review]
nick's patch to allow non-whitespace-separated variables


I *think* this is close; it might help to walk through this code, but...

>-            $interpLine =~ s/^#\s+CONFIG:\s+//;
>-            foreach my $variable (grep(/%[\w\-]+%/, split(/\s+/, $line))) {
>+            $interpLine =~ s/^#\s+CONFIG:\s?//;

I think you want: s/^#\s+CONFIG:\s*//; the regex you replaced it with will leave leading spaces if there are more than one space, and I don't *think* you want that.

>+            foreach my $variable (grep(/%/,split(/(%[\w\-]+?%)/, $line))) {
>+                $key =~ s/^%(.*)%$/$1/;

You have an implicit regex for a key ([\w\-]+); if you're using it in multiple places, it may make sense to make it a constant, and use it, in which case, the second line becomes:

$key =~ s/^%$KEY_REGEX%$/$1/;

Also, Do we want to assert that $key isn't "" after we munge it? It might make sense to make sure we don't accidentally break this in the future, if we had to fix it again.

Comment 4

[Robert Helmer [:rhelmer]]

Attachment: address preed's comment #3

Comment 5

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

already marked as critical. Need for "release automation", any update?

Comment 6

[J. Paul Reed [:preed]]

Comment on attachment 267620 [details] [diff] [review]
address preed's comment #3

>Index: Bootstrap/Config.pm

Looks good; I would only change one thing, but this is a suggestion:


>+                if ($key eq "") {
>+                    die("ASSERT: could not get key from $variable");

I would make this if ($key =~ /^\s*$/) {
 assert()
}

reason being that "" is pretty specific, but we basically want to assert on empty keys (zero or more spaces), so using a regex will help there.

It may be logically impossible to get a bunch of spaces in a key *now*, but I want to protect against the possibility in the future.

Comment 7

[Robert Helmer [:rhelmer]]

Attachment: patch as landed

use regex not ""

Comment 8

[Robert Helmer [:rhelmer]]

Thanks preed, landed!

Checking in Bootstrap/Config.pm;
/cvsroot/mozilla/tools/release/Bootstrap/Config.pm,v  <--  Config.pm
new revision: 1.9; previous revision: 1.8
done
Checking in t/tinder-config.pl;
/cvsroot/mozilla/tools/release/t/tinder-config.pl,v  <--  tinder-config.pl
new revision: 1.2; previous revision: 1.1
done