----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/150/#review359 -----------------------------------------------------------
"(which really has the dual meaning "something that might be a problem" and "something that should be seen at the default logging level")" . As I mention in the comments, I think only the first kind of message should be shown at WARNING level because I don't like chat. Others may disagree. autobuild/autobuild_main.py <http://codereview.secondlife.com/r/150/#comment255> I'd would be a little cleaner to use: os.environ.get(AUTOBUILD_LOGLEVEL) which returns None if the key does not exist in the map. I prefer None to '' for an unset value myself. Or, you can use: os.environ.get(AUTOBUILD_LOGLEVEL, '') if you prefer an empty string. autobuild/autobuild_tool_install.py <http://codereview.secondlife.com/r/150/#comment256> I disagree with upping this to warning. IMHO, the default mode for autobuild shouldn't be chatty. If you want it to talk to you, set your warning level to INFO. I prefer that it just run quietly with no messages if everything is going fine. If we have other messages that are WARNING level but not true warnings, I'd prefer they be made info instead. - Alain On Feb. 13, 2011, 5:21 a.m., Oz Linden wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://codereview.secondlife.com/r/150/ > ----------------------------------------------------------- > > (Updated Feb. 13, 2011, 5:21 a.m.) > > > Review request for Viewer. > > > Summary > ------- > > I combined open-31 into this because that change is so small and simple that > it didn't seem worth a separate commit (besides, while testing the rest of > this, I really wanted the short form of --verbose). > > open-31 changes the short form of --version to -V, so that the short form of > the more commonly used --verbose can be the lower case -v, and adds -n as a > short form for --dry-run (as it is used in many versions of make, and in > other commands like scp and rsync). > > open-2 corrects the problem that (at least in the viewer build) there is a > long delay during which there are no progress messages. The delay is the > configure step while packages are checked, including possibly downloading, > extracting, and installing them. Increasing the logging level with the > --verbose or --debug switches did not increase the amount of logging during > this phase. > > The fundamental problem proved to be that the package checks were done in > recursively invoked instances of autobuild (I am guessing that the process > hierarchy is autobuild->cmake->autobuild, but it could have just been > autobuild calling itself directly; this fix should work regardless of the > depth of the process tree). The verbosity control options were not passed > through to the child processes, and since there could be some other process > in between (as cmake above), it seemed more robust (and relies less on > correct configuration) to pass the verbosity control to the children through > the environment. > > The default verbosity is now read from the AUTOBUILD_LOGGING environment > variable, whose contents can be --debug, --verbose, --quiet, or the one > letter forms of any of those options (if the environment variable is not set, > then the default value is to log warnings and worse, as before). Any of the > options used on the command line override any value from the environment. > Regardless of how the verbosity level is established, the environment > variable AUTOBUILD_LOGGING is set for all child processes, so that if any of > those (directly or indirectly) are another invocation of autobuild, then that > recursive child will use the same verbosity level as the parent (unless, of > course the recursive invocation uses an explicit option). It is true that > this also allows the user to set a default verbosity in the shell > environment, and that works, but it wasn't really the motivation for the > change and I did not add anything to the documentation about it. > > With that change made, the options correctly controlled whether or not > logging is visible during the package checking phase. However, the resulting > messages had an inconsistent level of detail - some operations (such as > uninstall) were very verbose, while others (some of which might take > significant time) were logged only at high verbosity levels. This led to the > addition of a few short log messages at the default 'warning' level (which > really has the dual meaning "something that might be a problem" and > "something that should be seen at the default logging level") in order to > make sure that some message is printed before any potentially long operation > (downloads, extracts, installs, and uninstalls). Some other very detailed > messages were changed from info to debug levels, as the information they > display is really only useful when debugging either a new autobuild > configuration or autobuild itself. > > I think that the combination of these changes makes the default verbosity > reasonably informative (no long silences) without being overwhelming. > > (there is a failure displaying the diff for autobuild.configfile.py because > it is a one word change of the logging level for a log message added by one > of my other changes that has not yet been applied to the trunk) > > > This addresses bugs open-2 and open-31. > > > Diffs > ----- > > autobuild/autobuild_main.py 9ee2db08d677 > autobuild/autobuild_tool_install.py 9ee2db08d677 > autobuild/autobuild_tool_source_environment.py 9ee2db08d677 > autobuild/common.py 9ee2db08d677 > autobuild/configfile.py 9ee2db08d677 > > Diff: http://codereview.secondlife.com/r/150/diff > > > Testing > ------- > > Manually verified using configuring and building in viewer-autobuild > > > Thanks, > > Oz > >
_______________________________________________ Policies and (un)subscribe information available here: http://wiki.secondlife.com/wiki/OpenSource-Dev Please read the policies before posting to keep unmoderated posting privileges
