More responses.
Thanks,
Jean
transfer_cpio.py:
64: This variable might be more flexible as a tuple of ("-p", "-d",
"-u", "-m")
Not really. The design spec calls for just passing args through
without any processing. So holding it as a string is in fact easier.
This works for the current set of arguments, but if we ever want to
change the default args to use an option that requires an argument
(e.g., "-C bufsize"), it will be easier to do so if this variable is a
list/tuple - since subprocess.Popen/call/check_call takes a list of
arguments, it'll be looking for something of the form:
["cpio", "-i", "-C", "16384", "-p", "-du", "-m"]
"-pdum" works currently because cpio can handle that as a single
argument. (Even if this were changed to be a single item list,
["-pdum"], it leaves the door more flexible for future changes).
OK. I get what you're talking about now. Seems reasonable.
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.
[...]
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.
754-760, 960-966: These are init'ed in AbstractCPIO - is there a
reason to reset them here?
They are actually going away. But there is/was a reason. I wanted a
clean attribute every time parse_input is called.
transfer_err.py:
Philosophically speaking, i don't see a need for these exception
classes. Uses of "TransferValueError" should definitely be directly
replaced with the basic Python ValueError, which callers would
reasonably expect from a function if they pass in invalid parameters.
Which is how it was originally but was changed after the big talk
about exceptions and ( to be honest) a comment you made at one point
about ValueError.
Hrm. I find exceptions to be tricky... I've waffled in my philosophies
on them at times, that much I know.
And per our discussion with Drew and Karen, we'll now move to the model
of most of the try/except clauses will be removed and almost all errors
are caught and processed by the engine. This means that transfer_err.py
will be removed.
[...]
604-639: These lists can be stored in memory, right? Would it be
better to do so? A temporary file seems prone to issues across
pause/resume, particularly if the system is rebooted in between.
Not really. We are passing things through the DOC and I'm not sure we
really want to carry what could be 1000's of entries around. Doesn't
pause/resume
reread the manifest?
Out-of-process resume will only reread the manifest if the app chooses
to do so. For DC, this is done, since the user may modify the manifest
in between. For other apps, this may not be the case.
If there's concern about passing 1000s of entries around, it may be
necessary to look into defining whatever special functions are used by
the pickle module to load/unload an object, so that when this object
is serialized, the entries from the file are stored in the DOC
snapshot as well.
[...]
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?
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.
[...]
134: Can you explain what this math is doing here? It appears to be
that each pkg is assumed to be "roughly" the same size based on a
pre-generated average, and the number of packages given is
correlated against that?
Yes. Eventually we'd like to get package size information from IPS
but it's not there yet. Then this would be far more accurate of a
calculation.
Got it, that makes sense. Will there be a bug filed to update this
code when the relevant pkg API is available?
Yes.
215-219: Use set operations:
not_allowed = set(["prefix", "repo_uri", "origins", "mirrors"])
image_args = set(self.image_args)
overlap = a & b:
if overlap:
# error
217-219: Malformed format string
Are you sure? I've actually seen this work I believe.
Assuming not_allowed = "repo_uri", the output here will end up being
literally:
"%s to use should be repo_urispecified in the Source, not the args"
i.e., the string will be concatenated, not formatted. I think what you
want is:
raise ValueError("%s to use should be specified in the Source, not the
args" % not_allowed)
If that needs to line wrap, use implicit concatenation - without a
"+", i.e.:
raise ValueError("%s to use should be "
"specified in the source, "
"not the args" % not_allowed)
Will look at this. On the other hand it'll probably disappear with our
new try/except methodology.
569: What is this clause catching? The message seems rather specific
to a failure at line 563, and I imagine it could be constrained to a
single exception class.
Either 563 or 565 or 566. If any of these raise an exception the
destination hasn't been specified correctly.
Sorry, I wasn't clear on that. What exception types are we catching
here? They should be listed explicitly, rather than having a bare
'except' clause.
Gone.
67, 87: You've popped the first publisher twice - was that intended?
Yes. In this case, the first publisher is the p5i file and the 2nd is
the preferred and I want both of those dealt with and only additional
ones left.
Ok. When I reread the code and the comments again, that makes a bit
more sense. It may or may not be worth adding a comment around 67, or
in a docstring, about the structure of pub_list.
Will do.
transfer_prog.py: [...]
101: When would this occur?
The df code (on tree usr/src/cmd/fs.d/df.c allows for the case where
-1 could be there.
Would it be better to raise the error after line 81 then?
Will leave it to Ginnie to decide.
[...]
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.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
