Hi Jean.
Thanks for your efforts. Here are my comments:
DC-manifest.defval.xml (DC_bugs3)
---------------
OK
DC-manifest.rng:
---------------
223: What is a suitable default for the build area? We can add a
comment here,
and then add stuff to the defval manifest and defaults module to set it up.
How about /export/<distro_ID>?
437: This line looks the same as the original. Did you tab over, or use
spaces? Tabs are better.
DC_checkpoint.py:
----------------
203-204: nit: arguments are listed in a different order.
305: Does the invalid name get printed out, so the user knows which name to
change? (Name and number would probably be best.)
364: This:
for step in cp.step_list[:laststep+1]:
can be reduced to this:
for step in cp.step_list:
419, 431: Nit: I'd like to see the word "step" as well as the "====" here.
This may prove helpful in parsing the output.
441 - 451: This is a mismerge with my changes. Please use what is in
slim_source.
692: Why barf here? Just print script number instead, or just blank out the
checkpoint_name and let the checkpoint_message or script name print by
itself.
DC_defs.py:
----------
Please add a comment at the beginning of the wad of manifest field defs, and
another at the beginning of other defs (86+), to distinguish one group
from the
other. Feel free to add more comments as you see fit.
DC_ti.py:
--------
122-132: Looks to me that BOOTROOT, TMP and PKG_IMAGE areas are being
created
under mntpt, not in the build_data dataset (which the comments at 74 and 118
say).
162: No need for the else here, since the "if" clause returned
unconditionally.
This buys you one nesting level.
167: Instead of:
if cp.get_zfs_found():
do lots of nested stuff
do this:
if (not cp_get_zfs_found()):
return
do lots of nested stuff but at one less nesting level.
172: How come this isn't strip()ped? (use strip(), not rstrip())
190-191: Maybe I'm being dense, but I don't understand difference
between mntpt
and build_area. Please clarify. Better yet, add a comment.
204: I don't understand this either. Is mntpt an empty string, None, ???
I think this is when zfs is found but the provided mount point is not
zfs but
I'm not sure why... Please add a comment.
Please add a comment at all of your "elses". This will help make the
code more
readable / understandable. I'll also be able to review it better...
267: Nit: I'd prefer to see execfiles of definitions files toward the
top of a
file instead of at the bottom, since they are kinda-sorta like include
files.
DC_tm.py:
--------
OK.
DefaultsModule.py:
-----------------
Is logdir (line 67) still used? If not please remove.
ai_bootroot_configure:
---------------------
From your and Karen's responses to my bootroot autosize code review, I
anticipate that we will be keeping separate args for bootroot and temp dirs,
not a single arg for the build area. If that's the case, then there's
no need
to make changes to this file.
After talking with you and synching up, we've agreed that we will be keeping
separate args for these things, and so I will only give cursory review
to the
files which are being changed, but which I will be changing back with my
putback. I will mark these files / changes as "cursory/temp".
All changes in ai_bootroot_configure are cursory/temp.
ai_x86_image.xml:
----------------
OK.
dc_utils.py:
-----------
54: On the surface, I don't agree with this change. An empty string is
still a
string. How come this change is necessary?
distro_const.py:
---------------
OK.
Line 324 is cursory/temp/OK.
finalizer_checkpoint.py: (DC_bugs3)
------------------------
Cursory/temp/OK
finalizer_rollback.py: (DC_bugs3)
---------------------
Cursory/temp/OK
slim_cd.xml:
-----------
OK
slimcd_bootroot_configure:
-------------------------
Cursory/temp/OK
slimcd_post_bootroot_pkg_image_mod:
----------------------------------
Cursory/temp/OK
slimcd_pre_bootroot_pkg_image_mod:
---------------------------------
OK
bootroot_archive.py:
-------------------
Cursory/temp/OK
bootroot_configure:
------------------
Cursory/temp/OK
bootroot_initialize.py:
----------------------
OK
create_iso:
----------
Cursory/temp/OK
create_usb:
----------
47: I know we've been doing this for a while, but...
I caution against using /tmp as a work area, especially knowing
usbgen will use it to store both an iso and the usb image. This puts a
requirement that the system has lots of memory. If the system doesn't
have the
RAM it will need, a swapfile will have to be set up, and users may not know
about that. They'll only see an error :(
I encourage the use of a temp area under the build area instead, since
not all
systems have gigabytes of RAM, disk space is cheap, the temp area will be in
the same general vicinity as everything else used to build the distro
for quick
inspection/debugging (if we don't clean it up), and we can tidy
everything up
nicely (if we do clean it up).
I suppose for now we can leave this, but I'll file a low priority bug to
keep
this on the radar for future discussion.
post_bootroot_pkg_img_mod:
-------------------------
84: please use tabs
Rest is Cursory/temp/OK
pre_bootroot_pkg_img_mod:
-------------------------
Cursory/temp/OK
Thanks,
Jack
Jean McCormack wrote:
> I'd like Jack, when he gets back, and Karen to look at this:
>
> Webrev:
> http://cr.opensolaris.org/~jeanm/DC_bugs/
>
>
> bugs fixed:
> http://defect.opensolaris.org/bz/show_bug.cgi?id=3357
> http://defect.opensolaris.org/bz/show_bug.cgi?id=3359
> http://defect.opensolaris.org/bz/show_bug.cgi?id=3467
> http://defect.opensolaris.org/bz/show_bug.cgi?id=3555
> http://defect.opensolaris.org/bz/show_bug.cgi?id=3556
> http://defect.opensolaris.org/bz/show_bug.cgi?id=3557
> http://defect.opensolaris.org/bz/show_bug.cgi?id=3560
> http://defect.opensolaris.org/bz/show_bug.cgi?id=3647
>
> Jean
>
>
>
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>
