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
>
