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. > 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? 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? 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 >
