[opensource-dev] Review Request: OPEN-7: allow specification of the config-file with env var AUTOBUILD_CONFIG_FILE

[Oz Linden]

> On Feb. 8, 2011, 9:48 a.m., Alain Linden wrote:
> > 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.

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.


> On Feb. 8, 2011, 9:48 a.m., Alain Linden wrote:
> > autobuild/configfile.py, line 44
> > <http://codereview.secondlife.com/r/140/diff/1/?file=802#file802line44>
> >
> >     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...

According to its documentation, the os.environ.get is always going to return the same value that it did on the first call regardless.


> On Feb. 8, 2011, 9:48 a.m., Alain Linden wrote:
> > autobuild/configfile.py, line 214
> > <http://codereview.secondlife.com/r/140/diff/1/?file=802#file802line214>
> >
> >     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.

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.


- Oz


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/140/#review343
-----------------------------------------------------------


On Feb. 6, 2011, 5:39 p.m., Oz Linden wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/140/
> -----------------------------------------------------------
> 
> (Updated Feb. 6, 2011, 5:39 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> -------
> 
> 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.
> 
> 
> This addresses bug open-7.
> 
> 
> Diffs
> -----
> 
>   autobuild/autobuild_tool_build.py 9ee2db08d677 
>   autobuild/autobuild_tool_configure.py 9ee2db08d677 
>   autobuild/autobuild_tool_edit.py 9ee2db08d677 
>   autobuild/autobuild_tool_install.py 9ee2db08d677 
>   autobuild/autobuild_tool_installables.py 9ee2db08d677 
>   autobuild/autobuild_tool_manifest.py 9ee2db08d677 
>   autobuild/autobuild_tool_package.py 9ee2db08d677 
>   autobuild/autobuild_tool_print.py 9ee2db08d677 
>   autobuild/autobuild_tool_uninstall.py 9ee2db08d677 
>   autobuild/configfile.py 9ee2db08d677 
> 
> Diff: http://codereview.secondlife.com/r/140/diff
> 
> 
> Testing
> -------
> 
> 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.
> 
> 
> Thanks,
> 
> Oz
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.secondlife.com/pipermail/opensource-dev/attachments/20110208/9edb7383/attachment.htm