Hi Ginnie.
Here's group 1. The code looks pretty clean and clear. Nice job.
src/Makefile.master:
--------------------
OK, but...
- Question to the wider group: any reason why the
ROOTPYTHONVENDORINSTALL... and ROOTPYTHONVENDORSOLINSTALL... items are
not grouped together and in alphabetical order?
src/Targetdirs:
---------------
Please put the ict line behind the js2ai line. The manifest line is
also out of order. Can you position it please even though you didn't
originally place it?
lib/Makefile: OK
-------------
lib/Makefile.targ: OK.
------------------
Not your problem, but the same comment about the ...SOLINSTALLMANIFEST
statements being out of alphabetical order.
lib/install_ict/Makefile:
-------------------------
SUBDIRS is not specified. To build the install_ict/test subdirectory,
specify it in the Makefile as:
SUBDIRS= test
As is, the $(SUBDIRS) line has nothing to do.
lib/install_ict/__init__.py: Please consider putting the contents of
common.py into this file. My understanding of __init__.py is that it is
a good place for keeping package-wide (i.e. common) utility definitions
or declarations.
lib/install_ict/apply_sysconfig.py:
-----------------------------------
Nit: indentation on 97 and 99.
Nit: 110, 123: instead of hardwiring 3 for sys, use grp.getgrnam("sys")
to get it instead. I'd say the same for root too, but everyone knows
root is zero :)
119: Will the new install-gate Popen work here?
See /usr/src/lib/install_common/__init__.py
Handling the error coming back from the subprocess call would help the
user more easily decipher any errors. For example, by default, the
following prints if the default CalledProcessError exception is raised
for "ls <garbage-file>"
>>> subprocess.check_call(["/usr/bin/ls", "asdfasdf"])
asdfasdf: No such file or directory
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/usr/lib/python2.6/subprocess.py", line 488, in check_call
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['/usr/bin/ls', 'asdfasdf']'
returned
non-zero exit status 2
That's a lot for the user to wade through...
Nit: Would it fail more cleanly if the file copy/chown/chmod was done
first before the SVCCFG? This way SMF isn't set up for a missing file,
in case the copy bombs.
Just curious: line 74 says if dry_run is true, "the log message
describes the checkpoint tasks." How much detail, and where is this
implemented?
lib/install_ict/test/test_apply_sys_config.py:
----------------------------------------------
Nit: 41-42: can these be combined onto one line and still fit?
92: What purpose does this line serve?
94: Would it be more helpful to call assertTrue(False) with the
message/info from the received exception, or perhaps just let the raised
exception percolate up?
lib/install_ict/boot_archive.py:
--------------------------------
Same question asfor apply_sysconfig.py about dry_run tasks being
logged. Where is this implemented?
94: Same issue as apply_sysconfig.py about handling
subprocess.check_call() errors.
104: Does update_archive really take only a second to run? I thought it
took longer. You may want to time it and give a more accurate
approximation. (How's that for an oxymoron? :) )
lib/install_ict/test_boot_archive.py:
-------------------------------------
Nit: 41-42: can these be combined onto one line and still fit?
I see setUp() and tearDown() here is almost the same as in
test_apply_sys_config.py. Does it make sense to stick the common code
in a superclass which both of these classes (and perhaps other test
classes which may do the same setup as well -- I haven't looked at the
others yet) would subclass?
92, 94: Same comments as test_apply_sys_config.py about calls to assertTrue.
lib/install_ict/common.py:
--------------------------
Content looks OK, but may be cleaner to add to __init__.py instead.
src/pkg/manifests/system-library-install.mf: OK
--------------------------------------------
Thanks,
Jack
On 02/ 8/11 11:38 AM, 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
