Keith, Thank you very much for all of your help on this!
The webrev has been updated: http://cr.opensolaris.org/~joev/PY26 I have updated bug 6256 for the work that will be done as part of that bug. http://defect.opensolaris.org/bz/show_bug.cgi?id=6256 I have performed brief retesting. Thank you, Joe Keith Mitchell wrote: > 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. done > > 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. 6256 updated to include description of this. > > 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. done > 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? 6256 updated to include description of this. > > 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? Oversight on my part. I will replace the errno and rework this as part of 6256. 6256 updated to include description of this. > > 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). 6256 updated to include description of this. > > 192-193: Duplicated line here. good catch. Fixed. > > 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)" 6256 updated to include description of this. > > 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. 6256 updated to include description of this. > > 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. done > > 392-394: For 6256, this except clause should probably set DEBUGLVL = -1. done > > 449, 471, 515, 753: Should be "if rline.startswith('#'):" or "if > line.startswith('#'):" done > > 452: At some point, I think this portion of code needs a comment. done > > 544-547: Should be in a "finally" clause. 6256 updated to include description of this. > > 608: Not related to your changes, but it looks like the zero here should > be in a string, not an integer. Good catch! Fixed. > > 755: "if '=' not in line:" done > > 1163, 1168, 1979, 1981, 2184-2194: Re-align with open parens/braces from > previous line if space allows done > > 1627: Should just be "if browser_install_status:" (or "if > os.path.exists(browsercfg):") done > > 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. > B=B removed 6256 updated to include name length 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. 6256 updated to include description of this. > > 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) 6256 updated to include description of this. > > 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." The pylint: disable-msg=W0142 has been removed as pylint rating of 10 is not required. > > 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) Done > > 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
