[caiman-discuss] Code Review for DC packaging and finalizer script changes

[Jack Schwartz]

Hi Karen.

Here are my comments:

usr/src/Makefile.master: OK

usr/src/Targetdirs: OK

usr/src/cmd/Makefile:

34: install-tools isn't required here.

usr/src/cmd/Makefile.targ: OK

usr/src/cmd/distro_const/Makefile: OK

usr/src/cmd/distro_const/distro_const.py:

69: I suggest that instead of hardwiring this pathname, put it into 
DC_defs.py
(Its defs are being brought in anyway...).  Then this will keep all defs 
in one
place.

usr/src/cmd/distro_const/slim_cd/Makefile: OK

usr/src/cmd/distro_const/iso.sort:

Is this brought over from other places?  Why is this included here?  
It's not a rename.  Is it future planning?

usr/src/cmd/distro_const/slim_cd/post_bootroot_pkg_image_mod:

144: Seems kind of heaviweight to copy all of /platform and then delete
everything besides files called unix (of which there are two).  Can we just
copy only those two files in the first place, instead?

154-162:  Might be more straightforward to viewers if changed to the 
following:
    MKISOFS_ZLIB_OPTS="-o solaris.zlib -quiet -N -l -R -U -allow-multidot" \
        "-no-iso-translate -cache-inodes -d -D -V \"compress\" usr"
    if [[ "X${DIST_ISO_SORT}" != "X" && -s "${DIST_ISO_SORT}" ]]; then
        MKISOFS_ZLIB_OPTS=$MKISOFS_ZLIB_OPTS+" -sort $DIST_ISO_SORT"
    fi

177, 220: Missing ; after LD_LIBRARY_PATH setting and before "time"

194: Add a comment that /bin/cd is being called to check that you an cd to a
dir.

228: Is this check needed?  This was checked at 194.

usr/src/cmd/distro_const/slim_cd/pre_bootroot_pkg_image_mod: OK
    (Not an expert here though.)

usr/src/cmd/distro_const/slim_cd/slim_cd.xml:

10: This looks like it is left over from testing.  I would put
pkg.opensolaris.org:80 as the main URL for now, and get rid of the 
additional
authority.

41, 42: These can be removed for now, and let defaults get set for them.

usr/src/cmd/distro_const/utils/Makefile: OK

usr/src/cmd/distro_const/utils/build_dist.bash: OK

usr/src/cmd/distro_const/utils/build_dist.lib:

31-33: If cleanup() is a no-op, please delete it.

Would be cleaner to have an ENV variable for 
/usr/share/distro_const/slim_cd,
that would shorten 145, 158, 303 amd show these files referenced are in one
place.

usr/src/cmd/distro_const/utils/create_iso:

50: If OUTPUT_PATH is not returned by ManifestRead, err out.  There 
should be a
default for it.  You don't want to continue without it either.

usr/src/cmd/distro_const/utils/create_usb: OK

usr/src/cmd/distro_const/utils/mkrepo:

comment on line 30 needs to be updated.

usr/src/cmd/install-tools/Makefile: OK

usr/src/cmd/install-tools/proc_slist: OK

... although there is some inconsistency between not renaming 
ManifestRead.py
and ManifestServ.py to their non ".py" equivalents, while renaming
proc_slist.pl to proc_slist.

usr/src/pkgdefs/Makefile: OK

usr/src/pkgdefs/SUNWdistro-const/Makefile: OK

usr/src/pkgdefs/SUNWdistro-const/depend: OK

usr/src/pkgdefs/SUNWdistro-const/pkginfo.tmpl: OK.

usr/src/pkgdefs/SUNWdistro-const/prototype_com:

These are data files and should not be root bin.  root other?
97 f none usr/share/distro_const/DC-manifest.defval.xml 0444 root bin
98 f none usr/share/distro_const/DC-manifest.rng 0444 root bin
99 f none usr/share/distro_const/slim_cd/generic_live.xml 0444 root bin
100 f none usr/share/distro_const/slim_cd/iso.sort 0444 root bin
101 f none usr/share/distro_const/slim_cd/slim_cd.xml 0444 root bin
108 f none usr/share/distro_const/slim_cd/var_microroot_files 0444 root bin
109 f none usr/share/distro_const/slim_cd/usr_microroot_files 0444 root bin

usr/src/pkgdefs/SUNWdistro-const/prototype_i386: OK

usr/src/pkgdefs/SUNWdistro-const/prototype_sparc: OK

usr/src/pkgdefs/SUNWinstall/prototype_com: OK

    thanks,
    Jack

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
>