Hi Joe, This all looks good now! Thanks!
- Keith Joseph J VLcek wrote: > 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 >
