On 10/19/10 07:12 AM, jean.mccormack wrote:
More responses.
Thanks,
Jean
[...]
159: I'm not sure what the reason is for this method - can you
explain? Will other checkpoints feasibly need something similar (i.e.,
should there be something more global in the engine or logging module)?
If the user had not called the get_progress_estimate function then
reporting progress is stupid. So give_progress is set when they
call it and progress logging only occurs in that case.
Will other checkpoints need something similar? Possibly.
My line of thought here is that, if get_progress_estimate hasn't been
called, there probably aren't any ProgressHandlers assigned to the
logging module. In which case, calls to report_progress() are
hopefully being filtered out appropriately.
I think the reason this was put in was that there was a problem. Since
Ginnie is making the changes and she's the logger expert, I'll leave
this to her to decide.
In other words, the logging module is designed to perform filtering
of logging calls based on logging level, engine status, what handlers
exist, etc. I don't think the checkpoints should have to worry about
it - they should just call "report_progress", and if the logging
module determines that there's nowhere to send the message, then it
just gets ignored - just like if the logging level is set globally to
"logging.WARN", logging messages at the debug level are simply
ignored by the logger.
And if I coded around a bug, Ginnie can fix it.
Ok.
[...]
667: Would rather not see yet another "run/exec/cmd" function in
our source code. Given the current single use case for this
function, simply embed the check_call in _transfer - would even be
fine without the try/except clause, I think.
I'd rather leave this as a method in case it needs some beefing up.
If a special wrapper for subprocess.call/check_call/Popen is really
needed, it should probably be implemented in a way that's usable
across the entire code base. This is one of my major pet peeves in
our slim_source code - every module seems to have 1-3 special
wrappers for executing a subprocess. Last time I counted, we had
about 8 separate methods for this, most of which do nearly the same
thing.
https://defect.opensolaris.org/bz/show_bug.cgi?id=15957
http://opensolaris.org/jive/message.jspa?messageID=474361#474361
Personally, I don't think we need any wrapper around the subprocess
functions; but even if we do, 1 or 2 at most seems like it would be
enough. I don't think we need 8, 9, or more.
If embedding the subprocess.check_call() call is truly not desired
here, would it at least be possible to use one of the existing
wrappers, rather than rolling up a new one?
The only ones I find under lib are the old transfer mod which will
eventually go away, install_utils which at this point in time isn't
what we want, finalizer which again should go away, DefValProc.py
which will probably go away and isn't what we want and the one here.
So, I could move it out of run_exec which I don't think really
resolves your issue of having a bunch of calls. But those bunch of
calls aren't going to be around much longer and/or aren't relevant.
To me the more relevant question is, do we want to create one in
install_utils that does work for CUD? But since at this point there is
no one else needing this and we hope that never happens that doesn't
really feel right either.
I understand that some of them are going away (which will be great); I
just want to make sure we're set-up such that we don't end up in a
similar situation in a year or two, with several modules having several
different one-off implementations of run/exec/cmd functions that act as
very thin wrappers around subprocess calls - wrappers that make them
irrelevant to other potential consumers.
[...]
transfer_ips.py:
73: My understanding of gettext is that gettext.install should
*only* be called by an application, never by a module - modules
should use something like:
_ = gettext.translation
gettext.install() modifies the global namespace, so running it will
affect all imported modules - which means this line here will
override, or be overridden by, the call to gettext.install() that
will come from DC.
Similarly, I don't think a checkpoint should set locale.LC_ALL
globally.
But the IPS calls don't work without this code. ;-(
We'll need to work through this to make sure that our modules and
pkg's can be localized properly in the same process-space. The
pkg.client code appears* to be violating some of the rules set out by
the gettext module for use in libraries:
http://docs.python.org/library/gettext.html#localizing-your-module
* I say appears, because I'm really not sure. Localization is
something that eludes my grasp quite frequently, and it's all to easy
to blame someone else's code for the problem, but I don't think it
should be necessary for a consumer of an API to have to work around
the localization code used within that module.
Doesn't localization elude almost everyone's grasp at times?
Almost certainly!
I *suspect* that use of gettext.bindtextdomain("pkg",
"/usr/share/locale") will make their code work. If adding that call
at the top level of the module causes the pkg.client code to run,
then the best approach is probably a context manager that sets the
text domain to "pkg" before each pkg.client call, and resets it
afterwards.
If this isn't easily resolvable, the best approach is probably to
file 2 bugs - 1 against pkg.client, and 1 against this code, and try
to work together to figure out what needs to be done in order to make
both sides localizable in a way that neither side interferes with the
other. (Actually, filing those bugs may be needed anyway - even if
the bindtextdomain method works, in a perfect world that wouldn't be
necessary).
Seems reasonable. I'm already filing some bugs as it is, two more
makes almost no difference.
Thanks.
[...]
103-133, 162-190: This seems to overlap with the same functions in
prior files; would a common meta-class be worthwhile, defining
these basic things?
Possibly? So you're thinking a class like TransferClass which
TransferCPIO, TransferIPS...... are all children of? And they would
inherit some of these methods from?
Yes, something like that. Very lightweight, minimal number of methods
and a small set of common attributes.
Not sure here. Already getting quite a stacking of inheritance here
and it's pretty minor issue. And if some of these methods did change
we might have to unroll it. There's nothing that says they have to be
the same between the different classes.
Ok, sounds like it probably doesn't give us much by doing this, so no
need then.
Thanks,
Keith
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
