Hi Karen. Lookin' good! Couple of things, responses in line.
On 10/22/08 15:57, Karen Tung wrote: > Hi Jack, > > Thank you for the code review. Please see my responses inline. > > Jack Schwartz wrote: >> Hi Karen. >> >> Here are my comments: >> >> DC_tm.py, distro_const.py: >> -------------------------- >> >> I think it would be clearer to the user if you had the "Initializing >> the IPS package image area" message before the "Setting main >> repository" and "... authority" messages. It will also be clearer if >> repo and mirrors were grouped using indentation under authorities. >> Something like this is what I have in mind: >> >> Initializing the IPS package image area: rpool/dc/build_data/pkg_image >> >> Setting main authority: opensolaris.org >> repo: http://pkg.opensolaris.org >> mirror: pkg.mirror1.os.org >> mirror: pkg.mirror2.os.org >> .... >> mirror: pkg.mirror10.os.org >> >> Setting alternate authority: osalt.org >> repo: http://pkg.osalt.org >> mirror: ... >> ... >> mirror: ... >> >> > Good idea. Changed as suggested. >> install_utils.py: >> ----------------- >> >> I think the real fix for 3871 is line 431. I'm not sure if 417, >> changing stderr_log_level to ERROR is correct. Wouldn't that change >> mean that errors are printed less than stdout (which is set to >> DEBUG)? My understanding is that errors print more often than output: >> > Yes, errors will be printed to all files, output is just printed to > the detail log file. >> - have the simple-output file require a higher threshold to print, >> than the detailed-output file, >> - have stdout correlate to a threshold which is met for the >> detailed-output file but not for the simple-output file, >> - have stderr correlate to a threshold which is met for both >> detailed- and simple-output files. >> > I don't quiet understand what you are saying, but here's my > understanding on how > python logging works. > > Since we didn't define any extra logging levels, we will be using the > 5 built-in levels, > which are: > > CRITICAL: 50 > ERROR: 40 > WARNING: 30 > INFO: 20 > DEBUG:10 > > When you set an output to a certain level, every message above that > level will get logged. > For the DC, the console and simple log is set to log everything above > INFO. The detail log > is set to log everything above DEBUG. > For the DC, all stdout of the finalizer scripts are logged at the > level debug, and all error are supposed to be logged > for level error. So, all stdout only go to the detail log, and all > errors will go to console, simple and detail log. > > When I did the initial putback, because of this bug, I thought that > some of the command is sending their > stdout to stderr too. So, when I set the log level for the stderr to > ERROR, I got a lot of extra > output in the simple log. To get around that problem, I just set > stderr to the level DEBUG so there is not so much noise > in the simple log. Now, I found out that I have mistakenly assign > stderr to the stdout file descriptor, > everything is outputting the way it is supposed to after I made the > change. So, I am re-adjusting stderr to have the log level of > ERROR. That's why I made that change in line 417. OK, so the higher the log level for the file descriptor, the more frequent the messages: stdout has DEBUG and stderr has ERROR. There is also a level associated with the logs: I see from dc_utils that: simple_log has INFO level, and detail_log has DEBUG level. So stdout has a level higher than the simple log threshold but equal to the detail log threshold, so it goes only to the simple log. stderr has a level higher than both log thresholds. OK, makes sense. Thanks. > >> 435: I don't think you need out_fd and err_fd in the exceptions list >> (3rd arg to select). Have you tried without this? >> > Seemed to work ok without the those 2 file descriptors in the 3rd > argument. > Removing it. > >> babel_cd.xml and remaining files: >> --------------------------------- >> >> Just curious: is babel_cd a defacto-standard name? It doesn't seem >> like a descriptive name to me. Something like all_lang_slim_cd.xml >> is more descriptive. I assume it is called babel because the >> slim_install package is replaced by the babel_install package. >> > I don't know what babel_cd stands for actually. I guess it is from > the package name > like you mentioned. I do agree that all_lang_slim_cd.xml is a more > descriptive name. > I can change it to that. >> Hopefully I didn't miss a discussion on this, but.... >> >> It may be easier to maintain a single slim_cd.xml file, with a >> comment inside to replace the slim_install package with babel_install >> and a suggestion to use lzma compression. That way there will be only >> one file to modify should changes occur down the line. Also, this >> package switch-out can be easily documented by Barbara, and since >> users will be looking at the manifest anyhow, they will see the >> comment there. It will probably be easier (or less intimidating) for >> the user to peruse a single file than to figure out the differences >> of two files. >> >> So, my vote is to get rid of babel_cd.xml. >> > Well, the request to add this file come up when I was giving > instruction to RE > on how to create the all language CD. Dave suggested that it is much > easier for > us to ship a manifest for creating the all language CD instead of > having people > modify the values. > > I disagree on users being more intimidated with using 2 different > files. I actually > think that it is easier for the users. They can just take the file > and use it, instead of > having to read the comment or the documentation and then, hopefully, > make the > right changes. We can document on which file is for what. That way, > people don't need to figure it out. > In the long run, post 2008.11, we could implement the layering feature > of the manifest. > With that, then, we ship one common file, and then, 2 different files > with the differences. > > So, what do you think? I rename the file to all_lang_slim_cd.xml, and > we ship the file? OK. But I would like to see in slim_cd.xml a comment that that file builds a limited language OpenSolaris live CD iso and USB image. This helps make it clearer what the differences between the two files are. > > > The updated webrev with all the changes are available at: > > http://cr.opensolaris.org/~ktung/bugfixes_mod2/ > > Can you check it out to make sure everything is ok? distro_const.py: 320: Nit:It would be clearer to me to see the distribution name before anything else (except the start time). The rest looks fine to me. Thanks, Jack > > Thanks again for the code review. > > --Karen > >> But, if you must have these files, the changes to babel_cd.xml and >> remaining files look OK. >> >> Thanks, >> Jack >> >> >> >> Karen Tung wrote: >> >>> Hi, >>> >>> Please do a code review for changes that fixed the following: >>> >>> 3707 Distro constructor should output config info at start of build >>> 3999 Create a manifest for generating the global CD >>> 3871 stderr output from python finalizer scripts don't show up in DC >>> logfiles >>> >>> webrev: >>> >>> http://cr.opensolaris.org/~ktung/bugfixes/ >>> >>> Thanks, >>> >>> --Karen >>> >>> >>> _______________________________________________ >>> caiman-discuss mailing list >>> caiman-discuss at opensolaris.org >>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>> >> >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >> >
