> On Feb. 14, 2011, 12:57 p.m., Alain Linden wrote: > > "(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. > >
If we had a level that meant "this should normally be shown but is not any kind of error indicator", I'd have used that. It didn't seem worth adding a custom level just to get that, and existing usage meant that to get it you have to use warning. > On Feb. 14, 2011, 12:57 p.m., Alain Linden wrote: > > autobuild/autobuild_main.py, line 103 > > <http://codereview.secondlife.com/r/150/diff/3/?file=880#file880line103> > > > > 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. > > I found that to get the right effect, I needed to set the environment variable using the direct [] form, so I thought that using that form would be better when reading too. According to the documentation, they are functionally equivalent. > On Feb. 14, 2011, 12:57 p.m., Alain Linden wrote: > > autobuild/autobuild_tool_install.py, line 265 > > <http://codereview.secondlife.com/r/150/diff/3/?file=881#file881line265> > > > > 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. The whole point of this is that on many systems, new configurations just sit for as much as a few minutes with no indication that anything is going on at all. This issue got opened because multiple users complained about it. The info level doesn't show unless you set the logging to --verbose, and is much chattier. I could just as easily suggest that if you want it to be quiet, tell it to be --quiet. These things are a balance, and a minute and half of silence at the default level is a bit to far down the silent side. Besides, in that particular case, if I thought I was updating because I gave a command to update, it might be important to remind me that nothing is being changed. - Oz ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/150/#review359 ----------------------------------------------------------- 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
