Niall, I looked at everything. I primarily focused on boot.py, though.
Anything I didn't comment on looks ok to me.
NITS:
- You've got some Sun Microsystems copyrights in some of your headers.
Questions:
- Are you removing the grub_setup.py checkpoint? If so, are you planning on
removing the Grub specific DOC definitions from distro_spec.py and the
test for
grub_setup.py?
- do you have coverage numbers for your unittests?
create_iso.py
-------------
109-120: If dc_pers_dict must exist, you can change the call to
get_children()
to use not_found_is_err=True. This would allow you to have something that
looks like:
dc_pers_dict = self.doc.persistent.get_children(name=DC_PERS_LABEL,
class_type=DataObjectDict, not_found_is_err=True)[0].data_dict
if self.arch == "i386":
self.bios_eltorito = self.dc_pers_dict["bios-eltorito-img"]
# UEFI When GRUB2/UEFI is available, uncomment
# self.uefi_eltorito = self.dc_pers_dict["uefi-eltorito-img"]
Which is much more compact.
boot_spec.py
------------
54: Easier to change the outer quotes to a single tick and not have to
bother
with escaping the double quotes
175-197: There's no need to predeclare the variables as None. element.get
follows standard convention for dict.get() in that if there's no key by the
name you're requesting, it'll set the variable to None.
Also, please change the conditionals to "if <variable> is not None:"
test_boot.py
------------
30-48: NIT - please follow pep8 convention and alphabatize the imports
115-116: not really sure what this comment is saying. Can you clarify?
test_boot_spec.py:
-----------------
29-36: NIT - please follow pep8 convention and alphabatize the imports
boot.py:
-------
28-38 NIT - please follow pep8 convention and alphabatize the imports
84: NIT - I prefer to see an empty dictionary instantiated with dict()
rather
than {}
90: where did the value of 5 come from?
129: can you move this to __init__ ?
143: no need to escape the single ticks since it's wrapped in double quotes
152: can this move to __init__? You already have it there, but it's
initalized as None. Could it initialize as list() instead?
207: trailing ']' ?
211: align with line above
218, 221, 246, 248, : use "is None" rather than "== None"
229: use os.path.isabs()
245-258: can you use shutil.copy2() instead? It will handle copying all of
the file's metadata (uid, gid, permissions, etc.) along with copying the
file.
It's equivalent to cp -p
286: use dict() instead of {}
288: use list() instead of []
306: no need for "is True" just do if autogen:
337: no need for this since you're not returning anything
385-387: can we even get to this point since we trap on arch back in
execute()?
394: instead of doing the line-wrap at the "=", do it at the arguments to
mkdtemp()
temp_dir = tempfile.mkdtemp(dir="/tmp",
prefix="sparc_boot_menu_")
396: for really long line continuations, a 4 space indent is much easier to
read:
boot_menu_path = os.path.join(temp_dir,
boot_menu.path.strip(os.path.sep))
403-406: don't use try/except as a control structure
if not os.path.isdir(boot_menu_path):
os.makedirs(boot_menu_path)
411: you shouldn't do this here. Exiting the context manager will
close the
file for you.
412-417: unindent one level.
432-439: see above
437-439: see above about using a 4 space indent instead
445-449: same as 394/396
455-460: same as 403-406
481: use list()
507-510: same as 385-387
521: use list()
648-651: this is getting ugly. Can you back this out into a normal
for-loop walker?
651: no need for "== True"
674: can this move to __init__ ?
700-703: NIT: I ended up using 'slc' to designate disk slices. Can you
change slyce to slc to keep things in sync? Not a big deal to me either
way.
360, 718, 894, 1030: You read the first line /etc/release a lot. Could you
store that string in __init__? Also, there's an easier way to get the first
line:
import linecache
self.etc_release = linecache.getline("/etc/release", 1)
linecache will also never ever raise an exception. If the file or line is
missing it'll return an empty string
765: no need for "== True"
777: no need to wrap temp_dir in ()
798: *waves arms frantically* :D
819, 820: use dict()
839-843: with all the usage of /etc/release, shouldn't this move up into
__init__ ?
871-874: use not_found_is_err=True as above
908-910: as above
990-1001: It hurts me deep down that you used enumerate instead of
zip() :D
-Drew
On 4/20/11 7:51 PM, Niall Power wrote:
Hi all,
I'm looking for a few brave souls to review my implementation of a boot
checkpoint based on pybootmgmgt. If you'd like to volunteer but get around to
it later, please let me know when
you could be available to review.
The changes aren't huge mostly tweaks here and there, with the bulk of the code
contained in
usr/src/lib/install_boot/boot.py (about 1100 LOC) and boot_spec.py (about 250
LOC) so I'd be
very grateful for any comments received by April 27/28th. I'll even buy you a
pint :-)
The changes can be logically grouped together as follows if you want to look at
a subset of the changes:
1) Boot schema definitions and Data Object Cache support
usr/src/lib/install_manifest/dtd/Makefile
usr/src/lib/install_manifest/dtd/boot.dtd
usr/src/lib/install_manifest/dtd/dc.dtd
usr/src/lib/install_boot/boot_spec.py
usr/src/lib/install_boot/test/test_boot_spec.py (Tests)
2) Boot checkpoint implementation
usr/src/lib/install_boot/__init__.py
usr/src/lib/install_boot/boot.py
usr/src/lib/install_boot/test/test_boot.py (Tests)
3) Distro Constructor changes to use new boot-setup checkpoint instead of
grub-setup for X86
usr/src/cmd/distro_const/__init__.py
usr/src/cmd/distro_const/checkpoints/create_iso.py
usr/src/cmd/distro_const/manifest/dc_ai_x86.xml
usr/src/cmd/distro_const/manifest/dc_livecd.xml
usr/src/cmd/distro_const/manifest/dc_text_x86.xml
4) Packaging and makefile changes to slim_source repo
usr/src/Makefile.master
usr/src/Targetdirs
usr/src/lib/Makefile
usr/src/lib/Makefile.targ
usr/src/lib/install_boot/Makefile
usr/src/lib/install_manifest/dtd/Makefile
usr/src/pkg/manifests/system-library-install.mf
Webrev location:
http://cr.opensolaris.org/~niall/cud-boot/
Thanks,
Niall
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
