On 02/26/10 09:04 PM, Clay Baenziger wrote:
> Hi Shawn,
> Thank you for your comments, my apologies as your review made me aware I
> had not yet pylint'd or spell(1)'d my code which caused you extra work.
> Your view coming from IPS was very valuable to catch systemic issues in
> some of the AI code. My responses in-line.
> New webrev:
> http://cr.opensolaris.org/~clayb/11510/webrev2
> Differential webrev:
> http://cr.opensolaris.org/~clayb/11510/webrev2.diff
> Thank you,
> Clay
>
> On Wed, 24 Feb 2010, Shawn Walker wrote:
>
>> On 02/23/10 04:01 PM, Clay Baenziger wrote:
>>> Hi all,
>>> I've written a bit of code to change out the create-client.sh shell
>>> script to a Python version. This gets us yet closer to a 100% Python
>>> installadm(1). Please let me know what looks rough here; this is only
>>> going back to the default branch of slim_source.
>>>
>>> Bugs:
>>> 11510 create-client: accepts a SPARC image path for an X86 service 12159
>>> create-client accepts non-existent service names
>>> 14735 delete-service: should use SystemExit instead of SystemError
>>> exception
>>>
>>> Webrev:
>>> http://cr.opensolaris.org/~clayb/11510/
>>
>> src/cmd/installadm/create_client.py:
>> line 107, 141: setattr() is equivalent to "x.y = z"; the latter seems
>> more natural to me; this is likely many other places too
>
> I agree about which seems more natural! I've corrected where this can be
> changed (lines 101, 107, 141), however, I believe line 62 must remain
> setattr() as I don't have a hard coded attribute name.
Python doesn't care if the attributes existed when you created the
object or not. Barring special class setup, you can add arbitrary
attributes to any object you want without using setattr. Some consider
this a feature, some do not.
>> line 247: I'd suggest importing the warnings module at the top of this
>> program so that you can make any python warnings fatal errors:
>> warnings.simplefilter("error")
>
> I think this carries over to more Python code than just this script. I
> also don't feel that I understand the warnings module enough yet,
> however, to track this, I've filed Bug 14915 - Look into using the
> Python warnings module instead of wrapping execution in try blocks.
No, you still need to wrap execution blocks with try/except/finally.
All this does is if python prints a warning at runtime, such as a
deprecation message about a particular language filter, it will cause
that to be a fatal error instead.
This keeps your unit tests from silently spewing warnings without anyone
noticing.
>> line 255: I think this should be:
>> gettext.install("ai", "/usr/share/locale")
>
> That path is used across all of our code. It appears that the install
> message files are in /usr/lib/locale:
> [1] clayb at xsplat:ls -l /usr/lib/locale/zh.UTF-8/LC_MESSAGES/
> total 29
> -rwxr-xr-x 1 root bin 4974 Apr 23 2009 SUNW_INSTALL_INSTALLADM.mo
> -rwxr-xr-x 1 root bin 8343 Apr 23 2009 SUNW_INSTALL_LIBORCHESTRATOR.mo
Ah, my bad. We seem to have our messages files split a bit oddly for
different projects.
>> Also, due to a python bug [1], you probably need to try setting the
>> locale yourself first to avoid unexpected tracebacks later. For
>> pkg(5), a special setlocale() routine [2] is used before calling
>> gettext.install() to deal with this as follows:
>>
>> misc.setlocale(locale.LC_ALL, "")
>> gettext.install("pkg", "/usr/share/locale")
>
> Thank you for pointing out our lack of guard against bogus locales. FYI,
> currently we fail like:
> root at jumprope:~# export LANG=fooBaz
> root at jumprope:~# export LC_CTYPE=fooBaz
> root at jumprope:~# export LC_MESSAGES=fooBaz
> root at jumprope:~# export LC_ALL=fooBaz
> -bash: warning: setlocale: LC_ALL: cannot change locale (fooBaz)
> root at jumprope:~# installadm create-client -e c0ffeec0ffee -n
> install_test_ai_x8
> 6
> fooBaz: unknown locale
> fooBaz: unknown locale
> fooBaz: unknown locale
> Unable to setup x86 client: Failure running subcommand
> /usr/lib/installadm/setup
> -tftp-links client install_test_ai_x86 unknown
> /var/ai/install_test_ai_x86 01C0F
> FEEC0FFEE null.
> Got output:
> fooBaz: unknown locale
> fooBaz: unknown locale
> fooBaz: unknown locale
> fooBaz: unknown locale
>
> (Clearly this impacted running the shell scripts, etc. too)
> To track this, I've filed bug 14918 - AI subcommands need to guard
> against bogus locales
It's actually not just bogus locales which make this useful.
Just today, I ran into an issue where a test worked just fine on my
system, but then failed when run on the test server. It turns out that
this was because no LC_* variables were set in the test server
environment, and since the client was setting up the default locale, but
the test suite wasn't, some tests failed.
In particular, if you don't setup the default locale, and you use
functions like strftime(), users may see different results than they
might otherwise expect.
Either way, you've got a bug open, so I'm happy.
>> line 310: this will catch KeyboardInterrupt (^C) too and print an ugly
>> traceback, is that intended?
>
> That's a good point. I would prefer (since we don't have any sort of
> graceful clean-up to eat a control-C but that really seems ill behaved).
> What's IPS do; it looks like just raise a KeyboardInterrupt?
The internal API will ensure it gets raised all along the way, although
in some cases, it will catch the KeyboardInterrupt, perform cleanup
logic, and then re-raise.
The client then has a top-level error handler that catches
KeyboardInterrupt and sets the exit code to 1.
>> line 312: why not use traceback.print_exc(file=sys.stderr) instead?
>> (Note stderr instead of stdout since you're printing errors.)
>
> Thank you for catching the stdout instead of stderr. I hadn't looked
> into why traceback.print_exception(), instead of traceback.print_exc().
> It sounds likes print_exc() is better.
print_exc() is just shorthand, you don't have to call sys.exc_info()
first and then pass that along first.
...
>> line 314: Something that will help you immensely in debugging is
>> storing the hg changeset version in a variable by substituting it in
>> during the build process. You can then include that in this message as
>> the "version" of the program.
>
> I would like to do that. We have explicitly removed any version
> information from installadm for the time being, though it was not an SCM
> connected version as it should've been. Too see the logic behind this
> choice at this time, see:
> http://defect.opensolaris.org/bz/show_bug.cgi?id=6699
I can see that, but this really isn't "versioning" so much as it is
debugging information. If you're going to print a traceback, you might
as well include the changeset id so you can actually make a 1:1 mapping
between the error and the code. It takes all the guesswork out. You
could print it as part of the debug message and call it whatever you
want if labelling it "version" is the issue at hand.
Cheers,
--
Shawn Walker
