Ginnie,
I just took the python files (surprising, I know!)
General NITS:
you need #!/usr/bin/python for every python file. This
is due to some wonkiness with pkgdepend. Keith knows all about this
and can enlighten further if needed.
need to move *all* subprocess calls to solaris_install.Popen(). This
will allow you to remove all piping and redirects. you'll also get
automated logging of the command, stdout and stderr. It's MAGIC!
python modules need to be all lower case. Camelcase variable names
are reserved for class names. ict.Common needs to be renamed to
ict.common, globally.
---
__init__.py
---
31: remove trailing space before "]"
---
common.py
---
39: shouldn't need this. os.chdir() is what you want to use
58: shouldn't need this. os.unlink() is what you want to use
---
apply_sysconfig.py
---
86-90: you can remove the check for a None self.target if you use the
not_found_is_err=True in the get_descendants() call
96-97: align, or use a 4-space indent if it line-wraps
---
boot_archive.py
---
80-84: you can remove the check for a None self.target if you use the
not_found_is_err=True in the get_descendants() call
---
cleanup_live_cd.py
---
37: since it's a built-in module, move it above the from <module>
import lines above. (pep8-ism)
import os
import subprocess
from stat import X, Y, Z
from solaris.install.....
70: does 'bootcd_microroot ' really mean 'bootcd_microroot' (no
trailing space)?
125: align the variables of name and class_type
141-150: why not just use os.unlink(os.path.join(a, b))? Why the
overhead with subprocess?
152-160: I'm honestly surprised subprocess didn't explode at this.
You have pipes *and* redirects to /dev/null ... and your cmd variable
is a string and not a list and the check_call shell variable isn't set
to True .... wow.
Can we rewrite this to use os.walk instead of find, transfer.cpio
instead of /usr/bin/cpio (do we really need all of cpio for this?) and
os.unlink instead of rm?
219: s/remove/unlink (to be consistant)
229: where did the 60 seconds come from?
---
create_snapshot.py
---
98: this doesn't need a "%" character? also, align the second line
with the top
111: where did the 5 seconds come from?
---
update_dumpadm.py
---
96-97: align the two rows
99-100: align and no "%" ?
133: remove extra line
---
device_config.py
---
55: self.soft_list is never used
---
generate_sc_profile.py
---
90-99: can you replace this with a 'with' call?
154: align. if it line-wraps, do a 4 space indent instead
160: no "%" needed?
184-199: don't use try/except as a control structure. Also, you're
doing too much within a single try/except block. What if the first
section (a series of read calls) succeeds but the write call fails at
the end?
For the read section, can you check for the file first and if present,
do whatever. If it's not present, raise an exception, or ignore or
whatever you need.
Since you're opening the self.sc_profile_dst as w+, it's guarenteed to
overwrite whatever's there, so you shouldn't need a try/except around
it at all.
---
initialize_smf.py
---
104-115:
it might make it more clear (and not have gobs of whitespace) to do it
one entry at a time:
self.sys_profile_dict = dict()
self.sys_profile_dict[common.GEN_LTD_NET_XML] = \
os.path.join(self.target_dir, common.GENERIC_XML)
<...>
*OR*
you could set up the dictionary in common.py (since that's where both
the key and value are from in the first place) and when you walk the
dictionary items on lines 125-128, stick os.path.join in there.
for key, value in common.sys.profile_dict.items():
new_value = os.path.join(self.target_dir, value)
os.unlink(new_value)
os.symlink(key, new_value)
On 2/8/11 12:38 PM, Ginnie Wray wrote:
Hi -
I've need a code review for the ICT conversions. They are relatively
short, so I thought I would group them and ask a few people each take
a group.
Because this is short, I would appreciate feedback by February 16th.
If that doesn't work, let me know and I can accomodate you.
Webrev is located at:
http://cr.opensolaris.org/~ginnie/icts/
Thanks,
ginnie
Group 1:
usr/src/Makefile.master
usr/src/Targetdirs
usr/src/lib/Makefile
usr/src/lib/Makefile.targ
usr/src/lib/install_ict/Makefile
usr/src/lib/install_ict/__init__.py
usr/src/pkg/manifests/system-library-install.mf
usr/src/lib/install_ict/common.py
usr/src/lib/install_ict/apply_sysconfig.py
usr/src/lib/install_ict/test/test_apply_sys_config.py
usr/src/lib/install_ict/boot_archive.py
usr/src/lib/install_ict/test/test_boot_archive.py
Group 2:
usr/src/lib/install_ict/cleanup_live_cd.py
usr/src/lib/install_ict/test/test_cleanup_live_cd.py
usr/src/lib/install_ict/create_snapshot.py
usr/src/lib/install_ict/test/test_create_snapshot.py
usr/src/lib/install_ict/update_dumpadm.py
usr/src/lib/install_ict/test/test_updatedumpadm.py
Group 3:
usr/src/lib/install_ict/device_config.py
usr/src/lib/install_ict/test/test_device_config.py
usr/src/lib/install_ict/generate_sc_profile.py
usr/src/lib/install_ict/test/test_gen_sc_profile.py
usr/src/lib/install_ict/initialize_smf.py
usr/src/lib/install_ict/test/test_init_smf.py
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
