Closed Bug 379380 Opened 15 years ago Closed 15 years ago

TinderConfig step should handle no whitespace between variables

Categories

(Release Engineering :: General, defect)

defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: nthomas, Assigned: rhelmer)

References

Details

Attachments

(1 file, 2 obsolete files)

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%.
Rob points out that 
 # CONFIG: %foo% %bar%
works and I'm really talking about
 # CONFIG: %foo%/$bar%

In fact any non-whitespace between variables.
Nick's patch looks pretty good to me, and passes all the tests I've tried. What do you think?
Assignee: build → rhelmer
Status: NEW → ASSIGNED
Attachment #263649 - Flags: review?(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.
Attachment #263649 - Flags: review?(preed) → review-
Severity: normal → critical
Attached patch address preed's comment #3 (obsolete) — Splinter Review
Attachment #263649 - Attachment is obsolete: true
Attachment #267620 - Flags: review?(preed)
already marked as critical. Need for "release automation", any update?
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.
Attachment #267620 - Flags: review?(preed) → review+
Attached patch patch as landedSplinter Review
use regex not ""
Attachment #267620 - Attachment is obsolete: true
Summary: TinderConfig step - doesn't make multiple substitutions → TinderConfig step should handle no whitespace between variables
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
Status: ASSIGNED → RESOLVED
Closed: 15 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.