[opensource-dev] Review Request: OPEN-45: Correctly include the pass-through options when displaying the configure and build commands

Boroondas Gupte sllists at boroon.dasgupta.ch
Thu Mar 17 02:03:23 PDT 2011


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

Ship it!


This change looks correct and well done. (Didn't test it yet, though.) Thanks!

I especially like the aspect that logging is moved from the callers to the callee, which makes the affected code cleaner.

I have some ideas on how to make this even better:
* Make it easier to see where the command starts, e.g. by adding a semicolon after the word "command" or by adding some color.
* In the dry-run case, should the log message indicate that nothing actually happens? (e.g. have it say "would execute" instead of "executing" if dry_run is true)

Both these suggestions would change expected behavior, so they are out of the scope of this issue. I'll file separate jira issues for them.

- Boroondas


On March 16, 2011, 8:19 p.m., Oz Linden wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/213/
> -----------------------------------------------------------
> 
> (Updated March 16, 2011, 8:19 p.m.)
> 
> 
> Review request for Viewer and Alain Linden.
> 
> 
> Summary
> -------
> 
> Extended the __call__ method of Executable to pass in the command type as a string and the dry_run flag so that the logging of the command being run could use the infrastructure there to exactly assemble the options and arguments for the logging of the command to be run.  The callers then don't need to do the logging separately or do anything with dry_run other than pass it down to the executable.
> 
> 
> This addresses bug open-45.
>     http://jira.secondlife.com/browse/open-45
> 
> 
> Diffs
> -----
> 
>   autobuild/autobuild_tool_build.py abc1014d5ad6 
>   autobuild/autobuild_tool_configure.py abc1014d5ad6 
>   autobuild/executable.py abc1014d5ad6 
> 
> Diff: http://codereview.secondlife.com/r/213/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Oz
> 
>

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


More information about the opensource-dev mailing list