On 06/10/12 22:57, Edward Pilatowicz wrote:
...
On Tue, Jun 05, 2012 at 04:43:59PM -0700, Shawn Walker wrote:
On 05/22/12 01:45, Edward Pilatowicz wrote:
...
we could get rid of it by documenting that all api consumers manually
perform a publisher check before creating an image plan... but that
requires updating all api consumers.
i've added the following docstring for this paramter:
'pubcheck' indicates that we should skip the child image
publisher check before creating a plan for this image. only
pkg.1 should use this parateter, other callers should never
specify it.
s/parateter/parameter/
...
----------------------------------------
src/modules/client/pkgremote.py:
----------------------------------------
lines 177-182, 231-236, 444-446, 469-471, 491-494, 503-508, 559-562:
seems like these need to be try/finally to ensure lock gets
released even if execution is interrupted
i don't think so.
first, the locks are all in memory process locks.
second, all the code under locks has been written such that the only way
it will fail is due to a bug. almost all the code under locks is
actually simple variable assignments (the exceptions are Thread.start(),
assert calls, and debug messages if they are enabled). if any of these
operations fail we are not going to recover and the client is going to
die with a stack trace and exit, there by making process lock recovery
irrelevant.
If a user hits ctrl+c at just the right point, we won't unlock, and
that's not a bug, that's just interrupted execution. And I wouldn't
expect a stack trace in most cases from that.
...
----------------------------------------
src/modules/client/plandesc.py
----------------------------------------
lines 421, 477, 501: why do we have a method that just simply
returns a property value?
these were all pre-existing methods on the current PlanDescription
object. i was keeping them around for compatability. but i've gone
ahead and removed them.
If we're going to change the API incompatibly, and there are very few
consumers of this, then...
----------------------------------------
src/modules/misc2.py
----------------------------------------
Not thrilled about having a separate module only because of
pylinting :-(
i've gone ahead and folded misc2.py back into misc.py
(this required cleaning up some more pylint issues in misc.py)
lines 250, 514: I don't see where 'type' gets assigned, and I
don't think we should have a variable named the same as a builtin
function either.
'type' doesn't get assigned anywhere. this is a reference to the builtin.
Then I'm super-confused :-/
I think there are more bugs here then:
250 if type(desc) == type:
For exampleL
>>> print type("foo")
<type 'str'>
>>> print type("foo") == type
False
>>> print type("foo") is type
False
In short, I'm struggling to figure out what this code is supposed to do.
----------------------------------------
src/modules/prstart.c
----------------------------------------
Weird to have a whole module just for this function
carved out of the namespace.
i didn't think the module namespace was a scarce resource. i'm open to
suggestion for putting this functionality into an existing module.
It's not a scarce resource, but I don't think we want a namespace for
every piece of functionality either necessarily. I'm not sure what the
right answer is here; I'd be interested to see what Danek thinks.
The timings stuff also seems off somehow. For example, running 'pkg
update -nv' on my system takes a total of 7.00s user, but timings
reports a total of 0.217s user. It looks like it's only timing
client startup.
could you include the output from the command? i've been doing some
runs with timing enabled and the results seem ok.
I did that as an unprivileged user and in the case where a solver threw
an error if that makes any difference.
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
