372757 - Bootstrap config file format changes

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[J. Paul Reed [:preed]]

In bootstrap.cfg, we need to be able to specify platform-specific build variables for various config options.

The problem lies in that we use the same bootstrap.cfg for an entire release, but the following variables are platform and/or machine specific:

buildDir
l10n_buildDir
logDir
updateDir
verifyDir
tagDir
buildPlatform

Maybe these all turn into:

mac_buildDir
win32_buildDir
linuxi686_buildDir

etc.?

Comment 1

[Robert Helmer [:rhelmer]]

I think that the only part that is really platform-specific is the tinderbox build directory:

buildPlatform   = Linux_2.4.21-37.EL_Depend

This is so that the Build/Repack steps know where to remove the "last-built" file from. 

We could build that at runtime instead, or explicitly tell Tinderbox to clobber (which I think would be more clear anyway).

Besides that, the only platform issue that I can think of is that on Windows Tinderbox is in "/cygdrive/c/builds/tinderbox". We *could* detect cygwin and auto-prepend that, although I don't really like magical behavior like that.

So, I agree that an easy way to do this right now is to detect the platform, then have platform-specific variables for the following:

buildDir
l10n_buildDir
buildPlatform

Do you think the rest are necessary? It's only an issue on cygwin, and the difference is that anything bootstrap creates will go into "c:\cygwin\" instead of "c:\". It seems unlikely that we'll (for instance) tag on windows, and if we do, the difference is that the tagdir would be in e.g. "c:\cygwin\builds\tags" instead of "c:\builds\tags".

My only real concern is that it'll make the config file larger and harder to read. 

One alternative would be to detect the OS and look for an OS-specific override in the Config object (e.g. if you ask for buildDir and are detected as windows, Config looks first for win_buildDir and then for buildDir). This would give the ability to arbitrarily override anything based on OS.

Comment 2

[J. Paul Reed [:preed]]

(In reply to comment #1)
> I think that the only part that is really platform-specific is the tinderbox
> build directory:
> 
> buildPlatform   = Linux_2.4.21-37.EL_Depend
> 
> This is so that the Build/Repack steps know where to remove the "last-built"
> file from. 
> 
> We could build that at runtime instead, or explicitly tell Tinderbox to clobber
> (which I think would be more clear anyway).

We could, except remember that we found out that running Tinderbox mode in clobber can cause different results than depend, and in some cases, broken results.

This is a problem that should be fixed, and it's annoying, but... I don't think we're gonna fix it now. :-/

> Besides that, the only platform issue that I can think of is that on Windows
> Tinderbox is in "/cygdrive/c/builds/tinderbox". We *could* detect cygwin and
> auto-prepend that, although I don't really like magical behavior like that.

The other issue is that it isn't always /cygdrive/c; sometimes it's /cygdrive/d/ or even /cygdrive/e

> One alternative would be to detect the OS and look for an OS-specific override
> in the Config object (e.g. if you ask for buildDir and are detected as windows,
> Config looks first for win_buildDir and then for buildDir). This would give the
> ability to arbitrarily override anything based on OS.

That would be awesome!

If you could specify a win32_confVar that, if it didnt' exist, it would just return confVar, that would be teh winnar.

Comment 3

[Robert Helmer [:rhelmer]]

Attachment: collect and use system info from uname

Here's a first stab at this (only tested on Linux so far). Adds a SystemInfo method to Config, which can return info retrieved from uname.

This info is used to allow overrides on any variable in bootstrap.cfg by prepending $osname_, and also to include hostname in the announce and error email features.

Let me know what you think about the general design, I'll test on all three platforms in the meantime. 

Note - osname is a fuzzy match on sysname; if it contains (case-insensitive) cygwin or darwin or linux, osname is set to win32 or macosx or linux respectively. This is mostly done because cygwin uname returns the NT version number in the sysname, which we don't care about; it also makes it a little friendlier for the operator.

Comment 4

[Robert Helmer [:rhelmer]]

Attachment: just config.pm, tested on linux/mac/win32

Just the Config.pm changes, since the email changes are in bug 372762 and the config refresh is in bug 374555. This has been tested on all three platforms.

Comment 5

[J. Paul Reed [:preed]]

Comment on attachment 259048 [details] [diff] [review]
just config.pm, tested on linux/mac/win32

I don't think a configuration module should be printing out messages about whether something is a system variable or not.

I actually think system variables should be completely separate from configuration variables. It's fine that they're in the same class, but if I check if a variable exists with Exists(), it should return true or false based on whether the *configuration* variable actually was defined.

I think it's ok to return undef for a system configuration variable that may not be defined (you're basically wrapping uname() here; you could copy its behavior.)

Comment 6

[Robert Helmer [:rhelmer]]

(In reply to comment #5)
> (From update of attachment 259048 [details] [diff] [review])
> I don't think a configuration module should be printing out messages about
> whether something is a system variable or not.
> 
> I actually think system variables should be completely separate from
> configuration variables. It's fine that they're in the same class, but if I
> check if a variable exists with Exists(), it should return true or false based
> on whether the *configuration* variable actually was defined.

Ok, patching coming up.


> I think it's ok to return undef for a system configuration variable that may
> not be defined (you're basically wrapping uname() here; you could copy its
> behavior.)
 

Not sure what you're asking for here.. uname should return:
$sysname, $hostname, $release, $version, $machine

None of these should return undef, and if the user asks for something else then we know this function can't return it. 

Comment 7

[Robert Helmer [:rhelmer]]

Attachment: more comments, don't fall back in Exists case

Comment 8

[J. Paul Reed [:preed]]

Comment on attachment 259452 [details] [diff] [review]
more comments, don't fall back in Exists case


>-    open (CONFIG, "< bootstrap.cfg") 
>-      || die("Can't open config file bootstrap.cfg");
>+    open (CONFIG, "< bootstrap.cfg.example") 
>+      || die("Can't open config file bootstrap.cfg.example");

Live system should never use the .example config.

>     while (<CONFIG>) {
>         chomp; # no newline
>@@ -44,31 +45,113 @@
>     close CONFIG;
> }
> 
>+##
>+# Get checks to see if a variable exists and returns it.
>+# Returns scalar
>+#
>+# This method supports system-specific overrides, or "sysvar"s.
>+#  For example, if a caller is on win32 and does 
>+#  Get(sysvar => "buildDir") and only buildDir exists, the value of 
>+#  win32_buildDir will be returned. If not, the value of buildDir will
>+#  be returned (silently falls back).
>+##

This comment is somewhat confusing. I think what you meant was "if the caller is on win32 and does Get(sysvar => "buildDir") and a win32_buildDir variable exists, that will be returned. Otherwise, if only buildDir exists, the value of buildDir will be returned. Otherwise, the die() assertionw ill be hit.

It may be useful to put up a little ascii chart of all the cases and what gets returned, as well as the rationale for doing this.

>-    die "ASSERT: Bootstep::Config::Get(): null var requested" if (not 
>-     defined($args{'var'}));
>+    if ((not defined($args{'var'})) and (not defined($args{'sysvar'}))) {
>+      die "ASSERT: Bootstep::Config::Get(): null var requested";
>+    } elsif ((defined($args{'var'})) and (defined($args{'sysvar'}))) {
>+      die "ASSERT: Bootstep::Config::Get(): both var and sysvar requested";
>+    }

With my recent run in with "and" and "not," I'd prefer to stick to !, &&, and ||.

> 
>-    if ($this->Exists(var => $var)) {
>+    if (defined($args{'sysvar'})) {
>+        # look for a system specific var first
>+        my $osname = $this->SystemInfo(var => 'osname');
>+        my $sysvarOverride = $osname . '_' . $sysvar;
>+
>+        if ($this->Exists(var => $sysvarOverride)) {
>+            return $config{$sysvarOverride};
>+        }
>+    } elsif ($this->Exists(var => $var)) {
>         return $config{$var};
>-    } else {
>-        die("No such config variable: $var\n");
>     }
> }

I don't think this implements the fallback quite right; if we had a sysvar requested, we check to see if the sysvar exists; if it doesn't, we don't fallback to anything, and return... well... nothing (techincally, I think we return 0, because that's the result of the last call, which is $this->Exists()).

Also, it looks like the die() logic is missing (in the case that I request a totally bogus variable).


>+        if (exists($config{$sysvarOverride})) {
>+            return 1;
>+        }

The way this is written, you could just return exists(...), but this logic suffers from the same issues that Get() suffers from, so it may not be the right logic.

>+sub SystemInfo {
>+    my $this = shift;
>+    my %args = @_;
> 
>     my $var = $args{'var'};
>-    return exists($config{$var});
>+
>+    my ($sysname, $hostname, $release, $version, $machine ) = uname;
>+
>+    if ($var eq 'sysname') {
>+        return $sysname;
>+    } elsif ($var eq 'hostname') {
>+        return $hostname;
>+    } elsif ($var eq 'release') {
>+        return $release;
>+    } elsif ($var eq 'version') {
>+        return $version;
>+    } elsif ($var eq 'machine') {
>+        return $machine;
>+    } elsif ($var eq 'osname') {
>+        if ($sysname =~ /cygwin/i) {
>+            return 'win32';
>+        } elsif ($sysname =~ /darwin/i) {
>+            return 'macosx';
>+        } elsif ($sysname =~ /linux/i) {
>+            return 'linux';
>+        } else {
>+            die("Unrecognized OS: $sysname");
>+        }
>+    } else {
>+        die("No system info named $var");
>+    }

This looks OK; you may want to cache the result of the uname() call, but it probably doesn't matter.

Comment 9

[Robert Helmer [:rhelmer]]

(In reply to comment #8)
> (From update of attachment 259452 [details] [diff] [review])
> 
> >-    open (CONFIG, "< bootstrap.cfg") 
> >-      || die("Can't open config file bootstrap.cfg");
> >+    open (CONFIG, "< bootstrap.cfg.example") 
> >+      || die("Can't open config file bootstrap.cfg.example");
> 
> Live system should never use the .example config.


Sorry about that; didn't notice that I left it there. I've make this filename configurable now, since we need it for the unit test (and it should be configurable anyway).


> >+##
> >+# Get checks to see if a variable exists and returns it.
> >+# Returns scalar
> >+#
> >+# This method supports system-specific overrides, or "sysvar"s.
> >+#  For example, if a caller is on win32 and does 
> >+#  Get(sysvar => "buildDir") and only buildDir exists, the value of 
> >+#  win32_buildDir will be returned. If not, the value of buildDir will
> >+#  be returned (silently falls back).
> >+##
> 
> This comment is somewhat confusing. I think what you meant was "if the caller
> is on win32 and does Get(sysvar => "buildDir") and a win32_buildDir variable
> exists, that will be returned. Otherwise, if only buildDir exists, the value of
> buildDir will be returned. Otherwise, the die() assertionw ill be hit.
> 
> It may be useful to put up a little ascii chart of all the cases and what gets
> returned, as well as the rationale for doing this.


Yeah missing the "win32_" there makes all the difference. I am not thrilled about ascii art charts in comments, I've just corrected it for now. 


> >-    die "ASSERT: Bootstep::Config::Get(): null var requested" if (not 
> >-     defined($args{'var'}));
> >+    if ((not defined($args{'var'})) and (not defined($args{'sysvar'}))) {
> >+      die "ASSERT: Bootstep::Config::Get(): null var requested";
> >+    } elsif ((defined($args{'var'})) and (defined($args{'sysvar'}))) {
> >+      die "ASSERT: Bootstep::Config::Get(): both var and sysvar requested";
> >+    }
> 
> With my recent run in with "and" and "not," I'd prefer to stick to !, &&, and
> ||.


Pretty sure the precedence is right here and it's certainly more readable; I went ahead and switched to ! && and || though.
 

> >-    if ($this->Exists(var => $var)) {
> >+    if (defined($args{'sysvar'})) {
> >+        # look for a system specific var first
> >+        my $osname = $this->SystemInfo(var => 'osname');
> >+        my $sysvarOverride = $osname . '_' . $sysvar;
> >+
> >+        if ($this->Exists(var => $sysvarOverride)) {
> >+            return $config{$sysvarOverride};
> >+        }
> >+    } elsif ($this->Exists(var => $var)) {
> >         return $config{$var};
> >-    } else {
> >-        die("No such config variable: $var\n");
> >     }
> > }
> 
> I don't think this implements the fallback quite right; if we had a sysvar
> requested, we check to see if the sysvar exists; if it doesn't, we don't
> fallback to anything, and return... well... nothing (techincally, I think we
> return 0, because that's the result of the last call, which is
> $this->Exists()).
> 
> Also, it looks like the die() logic is missing (in the case that I request a
> totally bogus variable).


Good catch here, thanks.. not covering all the bases.

Comment 10

[Robert Helmer [:rhelmer]]

Attachment: address concerns from previous patch

Comment 11

[J. Paul Reed [:preed]]

Comment on attachment 259481 [details] [diff] [review]
address concerns from previous patch


> sub new {
>     my $proto = shift;
>+    my %args = @_;
>     my $class = ref($proto) || $proto;
> 
>     return $singleton if defined $singleton;
> 
>     my $this = {};
>     bless($this, $class);
>-    $this->Parse();
>+    $this->Parse(%args);

This is reasonable to do, but be aware that the logic here has a bug:

if I do my $conf = new Config("foo"), and then later in the app, I do my $conf = new Config("bar"), I will get the config for foo, not bar, because the singleton is returned immaterial of which config was parsed. That's a pretty bad bug. :-/

I would prefer the hard-coded config file if that's too difficult to fix in this round.

>+    my $configFile = 'bootstrap.cfg';

Should define this as a CONSTANT at the top of the file.

>+##
>+# Get checks to see if a variable exists and returns it.
>+# Returns scalar
>+#
>+# This method supports system-specific overrides, or "sysvar"s.
>+#  For example, if a caller is on win32 and does 
>+#  Get(sysvar => "buildDir") and win32_buildDir exists, the value of 
>+#  win32_buildDir will be returned. If not, the value of buildDir will
>+#  be returned. Otherwise, the die() assertion will be hit.
>+##
>+
> sub Get {
>     my $this = shift;
> 
>     my %args = @_;
>     my $var = $args{'var'};
>+    # sysvar will attempt to prepend the OS name to the requested var
>+    my $sysvar = $args{'sysvar'};
> 
>-    die "ASSERT: Bootstep::Config::Get(): null var requested" if (not 
>-     defined($args{'var'}));
>+    if ((! defined($args{'var'})) && (! defined($args{'sysvar'}))) {
>+      die "ASSERT: Bootstep::Config::Get(): null var requested";
>+    } elsif ((defined($args{'var'})) && (defined($args{'sysvar'}))) {
>+      die "ASSERT: Bootstep::Config::Get(): both var and sysvar requested";
>+    }
> 
>-    if ($this->Exists(var => $var)) {
>+    if (defined($args{'sysvar'})) {
>+        # look for a system specific var first
>+        my $osname = $this->SystemInfo(var => 'osname');
>+        my $sysvarOverride = $osname . '_' . $sysvar;
>+
>+        if ($this->Exists(var => $sysvarOverride)) {
>+            return $config{$sysvarOverride};
>+        } else {
>+            die("No such config variable: $var");
>+        }

I still don't think this logic is right. If I do Get(sysvar => "foo"), and that variable win32_foo doesn't exist, but foo does, I think this will blow up.

Same logic problem in Exists() too, I think.

Comment 12

[Robert Helmer [:rhelmer]]

Attachment: correct logic, punt on configurable config filename

Having to specify the config filename everywhere that we instantiate it is less useful; we should think more about this.

I created an entry in the makefile "test" target that copies bootstrap.cfg.example to bootstrap.cfg (if bootstrap.cfg does not currently exist) so the tests can run.

Comment 13

[J. Paul Reed [:preed]]

Comment on attachment 259636 [details] [diff] [review]
correct logic, punt on configurable config filename

Looks good.

cf: can you take a quick look too?

Comment 14

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

Comment on attachment 259636 [details] [diff] [review]
correct logic, punt on configurable config filename

>+    if (defined($args{'sysvar'})) {
>+        # look for a system specific var first
>+        my $osname = $this->SystemInfo(var => 'osname');
>+        my $sysvarOverride = $osname . '_' . $sysvar;
>+
>+        if ($this->Exists(var => $sysvarOverride)) {
>+            return $config{$sysvarOverride};
>+        } elsif ($this->Exists(var => $sysvar)) {
>+            return $config{$sysvar};
>+        } else {
>+            die("No such config variable: $var");

r+ if the last line uses $sysvar and a message says something like "No such sysvar".

Comment 15

[Robert Helmer [:rhelmer]]

Attachment: address concerns from previous patch

Good catch Nick.. also I've included Build.pm and Repack.pm changes to actually use the sysvar where appropriate.

Comment 16

[Robert Helmer [:rhelmer]]

Landed:

Checking in Bootstrap/Config.pm;
/cvsroot/mozilla/tools/release/Bootstrap/Config.pm,v  <--  Config.pm
new revision: 1.5; previous revision: 1.4
done
Checking in Bootstrap/Step/Repack.pm;
/cvsroot/mozilla/tools/release/Bootstrap/Step/Repack.pm,v  <--  Repack.pm
new revision: 1.8; previous revision: 1.7
done
Checking in Bootstrap/Step/Build.pm;
/cvsroot/mozilla/tools/release/Bootstrap/Step/Build.pm,v  <--  Build.pm
new revision: 1.6; previous revision: 1.5
done
Checking in Makefile;
/cvsroot/mozilla/tools/release/Makefile,v  <--  Makefile
new revision: 1.4; previous revision: 1.3
done
Checking in t/test.pl;
/cvsroot/mozilla/tools/release/t/test.pl,v  <--  test.pl
new revision: 1.4; previous revision: 1.3
done

Comment 17

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

Attachment: buildDir needs to be a sysvar in Verify too

Comment 18

[Robert Helmer [:rhelmer]]

Attachment: use sysvar in Build->Verify() too

Comment 19

[Robert Helmer [:rhelmer]]

Landed:

Checking in Bootstrap/Step/Build.pm;
/cvsroot/mozilla/tools/release/Bootstrap/Step/Build.pm,v  <--  Build.pm
new revision: 1.7; previous revision: 1.6
done

Comment 20

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

Attachment: Use sysvar for VerifyConfig in Updates.pm

Discovered while verifying Thunderbird 2 updates