All submissions to this site are governed by the Second Life Viewer Contribution Agreement. By submitting patches and other information using this site, you acknowledge that you have read, understood, and agreed to those terms.

Review Board 1.6.11

Welcome to the Second Life Viewer Code Review tool.
See the documentation on our wiki for how to use this site.

OPEN-7: allow specification of the config-file with env var AUTOBUILD_CONFIG_FILE

Review Request #140 - Created Feb. 6, 2011 and submitted

Oz Linden Reviewers
viewer
open-7
None autobuild 
This change allows the environment variable AUTOBUILD_CONFIG_FILE to specify a default config-file, so that the precedence becomes:

   1. the --config-file command line option
   2. the environment variable AUTOBUILD_CONFIG_FILE
   3. "autobuild.xml"

It also adds an info-level (--verbose) display of the config file name that is being loaded, and a warning level display if the config file does not exist.
Manually tested with and without the command line option, and with and without the environment variable set: confirmed that the correct file name is used per the precedence above.

Confirmed that the new logging works correctly.
Alain Linden
Posted (Feb. 8, 2011, 9:48 a.m.)
Should AUTOBUILD_CONFIG_FILE be renamed to AUTOBUILD_CONFIG_FILENAME to clarify this is a name and not a full file path?  Not sure what would happen if someone set AUTOBUILD_CONFIG_FILE=foo/bar/baz/MyAutobuildConfig.xml, but probably not what the user expects.
  1. The option name was chosen to match the name of the long form option that it supplies a default value for.

I just tested what would happen, and it's identical to what happens if you use the command
  autobuild configure --config-file foo/bar/baz/MyAutobuildConfig.xml
which is to say that the autobuild command reads the configuration file just fine, but cmake fails because the relative paths are incorrect.

If this is a problem, I think that it's a separate issue; the environment variable is bug-for-bug compatible with the command line switch.
autobuild/configfile.py (Diff revision 1) 
 
 
 
This gets called once on package load.  I guess that's fine as long as you just use autobuild through the autobuild script, but you might get unexpected results if you are calling autobuild functions in your own interactive python session. Admittedly that's a corner case... 
  1. According to its documentation, the os.environ.get is always going to return the same value that it did on the first call regardless.
autobuild/configfile.py (Diff revision 1) 
 
 
 
This isn't a warning.  It is perfectly OK to run autobuild with no script file, like, for example when you call autobuild edit to begin configuring an autobuild package.
  1. That depends on what you think that 'warning' means... in this case, I think it means that if I specified a file that I think exists but it does not, then without this warning that fact is not at all obvious - I get an error about some package not being defined, not a message that gives me any clue that the configuration file is not read.  

I don't think that anyone who is trying to create a new file would be bothered by a message that says that it does not already exist at the beginning, but in fact I tested it and that's a different code path - you don't get the warning when creating the file.
Ship it!
Jenn
Posted (Feb. 11, 2011, 5:52 p.m.)