Hi Joe,
My usual disclaimers apply for these CR's: If a suggested change is too
risky / too big to fit in for the 2.4->2.6 conversion, please just flag
it for changing later (convenient, given you own the bug 6256 for
updating ict.py - I'm letting you off a little easy this time around).
I'll try not to duplicate anything I've mentioned before in reference to
6256, but sometimes my memory fails me...
What's the error about install-finish not conforming to PEP8? Is it
because it doesn't end in .py? Just curious here.
install-finish:
54: Please group this import at the top with sys/getopt/os.
99-114: "if o in '-<letter>':" evaluates true if 'o' is '-'. While that
probably can't happen with getopts, it would be worth strengthening this
as part of 6256.
ict.py:
General: Please group Python standard library imports first, then '3rd
party' imports (such as pkg.cfgfiles), then add any osol_install imports
last.
General: Will you be converting "== None" and "!= None" to "is None" and
"is not None" respectively as part of 6256, or would you be able to do
so here?
General: In a lot of places, you moved from "except OSError, (errno,
strerror):" to "except OSError, (strerror):". If I'm reading that right,
then in the changed version, strerror will actually be either a tuple,
or an int with errno in it, instead of the strerror. Have you tested to
see if that's the case?
101-110: With this many imports, it may be cleaner to "import stat" and
then reference the variables as, e.g., "stat.S_IREAD". This can be
deferred (similar comment with the liblogsvc import below and in
install-finish).
192-193: Duplicated line here.
261-264: For 6256: "signal.signal(...)" returns the original signal
handler, so this can be condensed to "orig_sigpipe =
signal.signal(signal.SIGPIPE, signal.SIG_DFL)"
270-272: I know we talked about this one, but now that I see the
context, this can be condensed even further to simply:
"dfout.append(rline.rstrip('\n'))", since rline won't be None here.
Although, as part of 6256 this whole block will likely be changed to use
subprocess.Popen instead of os.popen, so perhaps not worth the effort/risk.
338-342: The "global" statement shouldn't be needed if you're only
reading the global variable. "global" is only required before modifying
a global variable.
392-394: For 6256, this except clause should probably set DEBUGLVL = -1.
449, 471, 515, 753: Should be "if rline.startswith('#'):" or "if
line.startswith('#'):"
452: At some point, I think this portion of code needs a comment.
544-547: Should be in a "finally" clause.
608: Not related to your changes, but it looks like the zero here should
be in a string, not an integer.
755: "if '=' not in line:"
1163, 1168, 1979, 1981, 2184-2194: Re-align with open parens/braces from
previous line if space allows
1627: Should just be "if browser_install_status:" (or "if
os.path.exists(browsercfg):")
2169-2171: Should not be spaces around the '=' sign when defining or
calling functions with keyword arguments. Also add a note for 6256 that
this function and its parameters could do with a name change.
2318: set_root_password: Add a note for 6256 that this could be modified
to call create_new_user with root as the username.
2391-2394: We don't need to use eval here. getattr or something similar
is more appropriate:
ict_func = getattr(myict, ict_name)
if optparm is not None:
ict_func(optparm)
else:
ict_func()
If this gets deferred to 6256, a fuller solution would be to have
optparm be a list of parameters, instead of a single parameter, and use
the * syntax to pass the list through. (This increases the flexibility,
allowing more than a single additional parameter to get passed in from
the command line)
2402-2407: Please add a reference to http://www.logilab.org/ticket/4037
as an indicator.
Also, you've got a couple typos in the comment:
"This is acceptable when there are no changes to the arguments."
2408-2409: I think this comment would be clearer as:
"Invoke exec_ict() with the command line parameters, ignoring the first
argument, which is always the script name (ict.py)."
(or something similar)
Thanks,
Keith
Joseph J. VLcek wrote:
> Please review the ICT code changes for the Python2.6 conversion, which
> includes PEP8 compliance.
>
> Note: install-finish pylint results: rated at 9.94/10 This is because
> of one error due to the file name not conforming to PEP8. I do not
> want to change this at this time. ict.py pylint results are a 10/10.
>
> The webrev is here: http://cr.opensolaris.org/~joev/PY26/
>
> Bug 6256 has been filed against the ICT code and will include work to
> "Python-ize" it. Attempting to prevent the introduction of bugs, I
> altered the code as little as possible. Any "Python-izing" feedback
> may be incorporated when addressing Bug 6256.
>
> I was able to test far more extensively then I had reported in our
> meeting on Friday. With help from Jean and Keith I was able to
> exercise the pkg Python2.4 code from the updated ICT Python 2.6 code.
> Thanks Jean and Keith!
>
> Testing:
> ------------
>
> An alternate root was populated in an extra disk on an x86 test system.
>
> I then exercised individual ICT's from the command line.
>
> I then installed SUNWinstall from my Python2.6 work gate and ran the
> updated install-finish script.
>
> All tests passed. See below for a details.
>
> Thank you! Joe
>
> ---
> Set Up for an alternate root on keelhaul.east
> ---------------------------------------------
>
>
> pfexec su
> zpool create -f rpool_ICT c1t1d0
> zfs create rpool_ICT/ROOT
> zfs set mountpoint=legacy rpool_ICT/ROOT
> zfs create rpool_ICT/ROOT/opensolaris
> mkdir /a
> mount -F zfs rpool_ICT/ROOT/opensolaris /a
>
> Populate the alternate root
> ---------------------------
>
> ROOT=/a
> pkg image-create --full -a opensolaris=http://ipkg.sfbay/dev/
> ${ROOT}
> pkg -R ${ROOT} install SUNWcsd
> pkg -R ${ROOT} install SUNWcs
> pkg -R ${ROOT} install slim_install
>
> TESTING ict.py
> --------------------------------------------------------
>
> cd /usr/lib/python2.6/vendor-packages
> ln -s /usr/lib/python2.4/vendor-packages/osol_install .
> ln -s /usr/lib/python2.4/vendor-packages/pkg .
> export LS_DBG_LVL=4
> python2.6 ./ict.py ict_test /a
> python2.6 ./ict.py create_smf_repository /a
> python2.6 ./ict.py create_mnttab /a
> python2.6 ./ict.py cleanup_unneeded_files_and_dirs /a
> python2.6 ./ict.py keyboard_layout /a
> python2.6 ./ict.py delete_misc_trees /a
> python2.6 ./ict.py set_prop_from_eeprom /a 3 keyboard-layout
>
> ...
>
>
> TESTING install-finish
> ----------------------
>
> cd <my work gate>/PY26/usr/src/
> time ./nightly -n developer.sh; echo "nightly status --->$?<---"
>
>
> Then on my test system, keelhaul, do:
>
> cd <my work gate>/PY26/packages/i386/nightly-nd
> pkgadd -d . SUNWinstall
>
>
>
> /sbin/install-finish -B '/a' -R
> '$5$Qab1tmRb$r359i65Bj0tRvBeudY6VjwrDpXRGoTm.4v0Ir6cdht7' -n 'Joe
> VLcek' -l 'guest' -p
> '$5$0H2JAt1i$G4BeRrCwIWzrPMBfcsNj4MKEtGp2fk4/Mk/W9jI4lR8' -G '10' -U
> '101'
>
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
