Hi Dave, Thank you for reviewing the changes. Please see my responses inline.
On 02/05/10 14:51, Dave Miner wrote: > On 02/ 3/10 07:19 PM, Karen Tung wrote: >> Hi, >> >> I would like to get at least 2 reviewers to look at changes for >> the following 2010.03 stopper. >> >> 5209 <http://defect.opensolaris.org/bz/show_bug.cgi?id=5209> >> Enhancements to the mkrepo script >> http://defect.opensolaris.org/bz/show_bug.cgi?id=5209 >> >> webrev: >> >> http://cr.opensolaris.org/~ktung/5209/ >> > > Mostly looks good, comments below. > > ai_sparc_image.xml > > 153: "in the order listed" > > (likewise for 170 in ai_x86_image.xml, 172 in > all_lang_slim_cd_x86.xml, 171 in slim_cd_x86.xml) > Changed as suggested. > slimcd_live.xml > > 26: s/Slim// Changed as suggested. > > 34, 125, 170: a comment about the timeout change reason would be nice I found a comment in the existing mkrepo script for the timeout values. I put that here. > > really, we need more comments in all of the install-specific profiles, > anyway I didn't add any additional comments. Should I file a RFE so more comments can be added to the files later? > > boot_archive_configure > > generally, the changes here are stylistically inconsistent in > referring to variables; I'm also not sure they're consistent with the > preferred coding style for ksh scripts, though I believe we have a > general bug about that in the finalizer scripts. It would be nice to > not make it worse, though. I updated the code I added to make it more consistent with whatever it is already there. > > 238: you've got a potential corner case here in that we require using > the build area's DTD for the manipulations here, but the code allows > for reference to the build system's profiles. I'd suggest that the > DTD reference should be rooted to be consistent with each profile > that's applied. Good catch. Fixed as suggested. > > 252: line seems unnecessary Line removed. > > 263: I would expect that this kind of failure results in a failure of > the build. Why should we just continue? You are right. We should immediately fail here. Fixed. > > generic_live.xml > > 26: perhaps should say "Base service profile customized..." Changed as suggested. > > 69, 83: why not put these under the group starting at 168, since that > comment applies to these two? Moved the 2 sections. > > mkrepo > > I hope we're certain that this will be removed post-EMI integration, > otherwise this file really could use a comment noting that it is an > adaptation of manifest-import and changes to manifest-import should be > evaluated for their applicability here. I have filed bug 14433 for removing mkrepo when early manifest import is available. > > media-fs-root, 181: A comment on why this is the right point to do > this would be a good idea. Smae goes for net-fs-root line 331. > Comment added. I have re-built all the images and tested them after making the above changes in the code. Please see the updated webrev here: http://cr.opensolaris.org/~ktung/5209-update/ Thanks again for the code review. --Karen
