Hi Jack,
Thank you for your code review. My responses are below.
Jack Schwartz wrote:
> 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.
It is required. Otherwise, all those "usbcopy/usbgen...." programs
doesn't get copied into the proto area correctly.
>
> 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.
>
Changed as suggested.
> 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?
Yes, it is brought over from the old DC gate. I forgot to bring this
over when I was doing the gate merge.
It is needed for optimizing the layout of the live cd. Without it, the
mkisofs command would still work, and
the resulting image will still work. But it is not as optimized. So,
this is needed.
>
> 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?
Don't understand this comment. All of /platform are there in the pkg
image area
are there as a result of IPS installing packages. /platform is copied
to the bootroot.
Everything besides unix are deleted from the **pkg image area**, because
it is
already there in the bootroot. This logic is from the old DC. Not new
here.
>
> 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
Not sure whether having the -sort option at the end works.
This again is code from the old DC, but now that you mention it, I think
it is cleaner
to change those lines to this.
if [[ "X${DIST_ISO_SORT}" != "X" && -s "${DIST_ISO_SORT}" ]]; then
SORT_OPTION="-sort ${DIST_ISO_SORT}"
fi
/bin/mkisofs -o solaris.zlib $SORT_OPTION -quiet -N -l -R \
-U -allow-multidot -no-iso-translate -cache-inodes \
-d -D -V "compress" usr
>
> 177, 220: Missing ; after LD_LIBRARY_PATH setting and before "time"
It works without the ";". This is old code, it works the way it is.
I will experiment with adding the ; and will add that if it works so
that it is
not too confusing for others.
>
> 194: Add a comment that /bin/cd is being called to check that you an
> cd to a
> dir.
>
Added. But this check is for making sure that I go back to the "top" of
the pkg image area.
> 228: Is this check needed? This was checked at 194.
After generating each archive, I always go back to the "top" of the pkg
image area.
This check is being able to successfully go back to the "top" of the pkg
image area.
So, I would say that it might not be needed, but it is a "safety" call,
in case people
"cd" to elsewhere while they are creating the archives.
>
> usr/src/cmd/distro_const/slim_cd/pre_bootroot_pkg_image_mod: OK
> (Not an expert here though.)
>
No problem. It is existing old DC code that is working. :-)
> 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.
>
Fixed as suggested.
> 41, 42: These can be removed for now, and let defaults get set for them.
I don't think we should remove them. We can set them to different values if
you don't like the values that's here now. This is because everybody's
machine setup
is different, and the default for these 2 values is most likely wrong
for people's machine.
Having the lines here will make it easy for people to customize it to
their machine.
>
> 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.
As I was replying to Jean in her code review comments, cleanup() is
indeed a no-op, but removing it involve removing all the places that
call it,
and since build_dist.lib will be thrown a way in the next day or two,
i don't feel like it is worth the trouble.
>
> 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.
Yes, that's done already. In line 45-48. Line 50 is to create the
directory if it doesn't exist.
>
> usr/src/cmd/distro_const/utils/create_usb: OK
>
> usr/src/cmd/distro_const/utils/mkrepo:
>
> comment on line 30 needs to be updated.
Done.
>
> 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.
I want to keep Manifest*.py with the .py subfixes so that they can be
compiled
to check for syntax errors. Then, we just "cp" them to be without the .py.
proc_slist.pl is different. It is just a perl script, and all the
script names generally
have the convention to not have the .xx part. That's why I rename it
that way.
>
> 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
>
All those are changed to root:sys.
Thanks again for your code review.
--Karen
> 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
>>
>
