Hi Jean, Thank you for the code review. Please see my responses inline.
Jean McCormack wrote: > cmd/Makefile.targ > > nit: remove line 337 > Done. > post_bootroot_pkg_image_mod: > We had decided awhile ago to get rid of the references to proto area. > In that vein, it would > be nice to see DIST_PROTO be replaced with PKG_IMG_PATH or something > like that. > Good idea. Changed to PKG_IMAGE_PATH on all the finalizer scripts. > line 115-117: Should we also be checking that it's a valid compression > type? What if I > put in something that we don't support? I thought that's should be done by the manifest validation. Jack: is that right? If not, I can add it. > > line 209: Is that a good assumption? i.e. the 100k one? What happens > if var/pkg isn't > 100k? > (Note: this might not be something you actually added. Sorry if that's > the case) > This is added a while back. We are trying to not do floating point math here. 100K should be a safe assumption. I just tried an image-create to initialize a IPS directory, but I didn't install any packages in there. Then, I did a "du -sk", the size is already 3612K. > pre_bootroot_pkg_image_mod: > I'd like to see DIST_PROTO renamed. > Done. > > slim_cd/slim_cd.xml: > Why did you remove <logfile_dir>? > Don't know why.. I have added it back. > build_dist.lib: > line 31: If this does nothing, shouldn't it just be removed? > I left it in because if I remove it, I need to change all the places that calls it. I was going to just leave it empty, but that generated syntax error. Everything in build_dist.lib and build_dist.bash should be removed in the next couple of days anyway, when the bootroot work is done. So, I didn't want to spend anytime fixing it up. > create_iso: > Please rename DIST_PROTO Yep, done. > > mkrepo: > line 30: It appears you modified what the 2nd parameter is. Please > have this line reflect the change. > Fixed. > SUNWdistro-const/prototype_com: > Have you checked with Jack to make sure you don't create the same bug > he did with /usr/share? > I checked, and made sure that things have the right owner/group. root:bin for executable/binary/library files. root:sys for data. > Question: We have finalizer_checkpoint.py and finalizer_rollback.py in > /usr/lib/pyth*/vend*/osol*/di* > but the pre and post bootroot finalizer scripts in > /usr/share/dist*/slim_cd. > Why are they in different places? It would be nice to have all > finalizer scripts in the same place. > > pre and post bootroot finalizer scripts are specific to the slim live cd, so, I put then in their special directory inside the distro_const directory. Now I think about it, I should move "finalizer_checkpoint.py" and "finalizer_rollback.py" to /usr/share/distro_const too instead of leaving them in the python modules directory. The python module directory should only have modules, and those 2 are actually executable files. I do agree that we should have all the finalizer scripts in the same place, so, I think I will move the two finalizer*.py scripts into /usr/share/distro_const. Thanks again for the code review. --Karen > Jean > > Karen Tung wrote: >> Hi, >> >> Please review the changes for the following: >> >> 3151 Distro Constructor package >> 3152 Convert part of old DC into finalizer scripts for new DC >> >> http://defect.opensolaris.org/bz/show_bug.cgi?id=3151 >> http://defect.opensolaris.org/bz/show_bug.cgi?id=3152 >> >> webrev: >> http://cr.opensolaris.org/~ktung/dc_pkg/ >> >> Please note that the 4 finalizer scripts: >> - pre_bootroot_pkg_image_mod >> - post_bootroot_pkg_image_mod >> - create_iso >> - create_usb >> are ported directly from the prototype DC gate. I have added >> some error checking to make them more robust, and to >> retrieve needed values from the manifest. There are >> no other new functionalities in the code. >> >> I have tested this by installing the new SUNWdistro-const package and >> updated >> SUNWinstall package into my test machine. Then, I verified that I >> can successfully >> build an image using everything that's delivered by the >> SUNWdistro-const package. >> >> I would appreciate your code review comments by COB Tuesday 9/2. If >> you want >> to review and need more time, please send me email and let me know >> and I will >> wait for you. >> >> Thanks, >> >> --Karen >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >> >
