Hi Jack, Here are my comments:
General comment: You added 2 new key_value_pairs to the manifest to define the mount point for the bootroot, and the name of the bootroot. I don't think these 2 values belong in the manifest. The mountpoint for the bootroot is just an implementation detail, I don't think the user should care about where the mountpoint is, so, no need for it to be configurable. The name of the bootroot definitely shouldn't be configurable, because it is a "standard" value recognized by the system, if the user change it to anything else, it won't boot. Specific comments: - slim_cd.xml: * line 13: why did you remove the logfile_dir line? * line 130: also change this to /tmp/pre_bootroot_pkg_image.err, like you did with all thye other lines. - usr/src/cmd/distro_const/utils/Makefile: * I know we don't care about having .pyc for those 2 python applications. But it is good to declare a python target so that those 2 python apps can be compiled to check for syntax error. - usr/src/cmd/distro_const/utils/bootroot_archive.py * line 63-65: Should we check to make sure there are indeed 3 arguments passed in before just getting the values? * line 103: shouldn't be any space between the parathesis and "cmd". * line 109-110: can this be done by the os.rename() command? - usr/src/cmd/distro_const/utils/bootroot_configure.bash * why name this file xxxx.bash? Most of the shell scripts don't have the .<shell_name> as the subfix, it looks odd to me. * line 118-120: check to make sure 3 args are passed in? * line 125 and line 194: I don't think we care about the ORIG_DIR, since this script is now independently invoked by the finalier. - usr/src/cmd/distro_const/utils/bootroot_initialize.py * line 62-64: Check to make sure there are indeed 3 args? * line 155-159, since this is not doing anything here, I think it will be less confusing to delete the comments. When we add the functionality later, we can add it in. * line 236, s/CIL/CLI/ * lines 236-247, I think we should just delete this instead of leaving it here. This is because even when we fixed up the TM, we can't just uncomment this to have it work since it doesn't handle the directory exclusion. When we really fix TM, we can write the code correctly then. * line 256-257, use os.chmod()? Actually, better to do os.mkdir() with the permission like line 260 * line 280, do we need to specify a permission for the .livecd file? * line 284, do we need to specify a permission value for .cdrom directory? * line 79 and line 287, no need to go back to "orig_dir". Thanks, --Karen Jack Schwartz wrote: > > -------- Original Message -------- > Subject: Please review: new DC bootroot constructor > Date: Fri, 05 Sep 2008 14:12:08 -0700 > From: Jack Schwartz <jack.a.schwartz at sun.com> > To: caiman-discuss <caiman-discuss at opensolaris.org> > > > > Hi everyone. > > Here is a webrev which installs a new bootroot constructor for DC. It > introduces three finalizer scripts which together handle putting > together the bootroot. It ushers out build_dist.bash and > build_dist.lib. It also includes a change to dc_utils.py to support > manifest keys. This is the final piece needed for our phase 0 release! > > Thanks go to Channing Lovely, who did a significant amount of the > work, particularly on the initialization script and the packaging. > > Please send your comments before Monday noon. > > Webrev is at: > http://cr.opensolaris.org/~schwartz/080905.1/webrev/index.html > > Thanks, > Jack > >
