Hi Drew -
Thanks for the feedback. My comments are below.
ginnie
On 02/ 9/11 02:19 PM, Drew Fisher wrote:
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.
I'll add that in.
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!
Jack also commented on this. I'll do so.
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.
I think we cleared this up, and determined Common was actually a class.
That said, Jack suggested that the variables be moved into the
__init__.py file.
---
__init__.py
---
31: remove trailing space before "]"
ok. Wonder why pep8 didn't catch that.
---
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
ok.
---
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
ok.
96-97: align, or use a 4-space indent if it line-wraps
ok
---
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
ok. I'll do that for all of them that do that.
---
cleanup_live_cd.py
---
37: since it's a built-in module, move it above the from <module>
import lines above. (pep8-ism)
ok.
import os
import subprocess
from stat import X, Y, Z
from solaris.install.....
70: does 'bootcd_microroot ' really mean 'bootcd_microroot' (no
trailing space)?
I'll remove the space
125: align the variables of name and class_type
will do.
141-150: why not just use os.unlink(os.path.join(a, b))? Why the
overhead with subprocess?
Good point.
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?
yes. I think that can be done.
219: s/remove/unlink (to be consistant)
ok.
229: where did the 60 seconds come from?
It was a guess. I'll do as you suggested when we talked and add a
comment that the return val needs to be determined.
---
create_snapshot.py
---
98: this doesn't need a "%" character? also, align the second line
with the top
ok. I will fix this.
111: where did the 5 seconds come from?
Same as the above time.
---
update_dumpadm.py
---
96-97: align the two rows
will do
99-100: align and no "%" ?
ok
133: remove extra line
ok
---
device_config.py
---
55: self.soft_list is never used
I'll remove.
---
generate_sc_profile.py
---
90-99: can you replace this with a 'with' call?
I'll see what I can do.
154: align. if it line-wraps, do a 4 space indent instead
ok
160: no "%" needed?
ok.
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.
ok. I'll revise it.
---
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)
ok. I'll revisit this as well.
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
