TinderConfig step should handle no whitespace between variables

RESOLVED FIXED

Status

Release Engineering
General
--
critical
RESOLVED FIXED
11 years ago
4 years ago

People

(Reporter: nthomas, Assigned: rhelmer)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

Comment 1

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

In fact any non-whitespace between variables.
(Assignee)

Comment 2

11 years ago
Created attachment 263649 [details] [diff] [review]
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?
Assignee: build → rhelmer
Status: NEW → ASSIGNED
Attachment #263649 - Flags: review?(preed)

Comment 3

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

Updated

11 years ago
Severity: normal → critical
(Assignee)

Comment 4

11 years ago
Created attachment 267620 [details] [diff] [review]
address preed's comment #3
Attachment #263649 - Attachment is obsolete: true
Attachment #267620 - Flags: review?(preed)
already marked as critical. Need for "release automation", any update?

Comment 6

11 years ago
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+
(Assignee)

Comment 7

11 years ago
Created attachment 268314 [details] [diff] [review]
patch as landed

use regex not ""
Attachment #267620 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Summary: TinderConfig step - doesn't make multiple substitutions → TinderConfig step should handle no whitespace between variables
(Assignee)

Comment 8

11 years ago
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
Last Resolved: 11 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.