On 08/08/11 11:21, Edward Pilatowicz wrote:
On Sun, Aug 07, 2011 at 07:39:33PM -0700, Brock Pytlik wrote:
On 08/05/11 16:32, Edward Pilatowicz wrote:
On Tue, Aug 02, 2011 at 06:37:22PM -0700, Brock Pytlik wrote:
On 07/22/11 14:25, Brock Pytlik wrote:
Webrev:
https://cr.opensolaris.org/action/browse/pkg/bpytlik/18439-v1
New webrev:
https://cr.opensolaris.org/action/browse/pkg/bpytlik/18439-v2
sorry for the delay.
i still need to review the tests.
comments on the code are below.
ed
----------
general
- i thought that the last time we talked about this we had agreed on
putting a copy of the "parsable_version" into the progress tracker
class? if we did this we wouldn't have to add an extra parameter to
all the linked image functions, and we wouldn't have to scan the
arguments in __pkg_cmd().
I don't remember agreeing on that, I remember discussing it. When I
got back and looked at the code more, I decided that I didn't like
it inside the progress tracker since it doesn't have anything to do
w/ the ProgressTracker, and just stuffing it inside didn't strike me
as right.
we've got verbose and quiet in there, so i think parsable fits pretty
well, i also think it simplify the code a bunch because you wouldn't
need to add new parameters to so many functions.
I disagree. The progress tracker doesn't emit parsable output. The
options it currently has effect its output, while the output of a
progress tracker with quiet set would be indistinguishable from the
output of one with quiet and parsable set.
----------
src/modules/client/api.py
- in __plan_op() we should not plan all children and then return the
results. instead do_recurse() should yield results as they become
available.
If you can make that work, you're welcome to tell me how. I spent
the better part of an afternoon trying to convince it to do what we
wanted it to do without rewriting the entire stack of code. Since
all the parsable output has to be gathered up together in any case,
I decided to punt on the issue.
i assume that __plan_op() would call do_recurse in a for loop and
yeild() each result. do_recurse would itself become an iterator and
yeild() a result for each child as processes them. but i guess we can
re-write this later...
I believe that's exactly what I tried that didn't work, but I'll give it
a shot one more time.
- the detach_linked_children() docstring describes the tuple returned by
linked image operations. please update this for the new value your
adding.
Sounds like I won't need to do this given the comment above.
the comment still needs updating since the return format is the same for
all the *_linked_children() functions, and the other *_linked_children()
functions have doc strings which say:
For a description of the return value, refer to the
'detach_linked_children' function."""
the return value for detach_linked_children() should be handled
similarly to the return value for audit_linked_children(), p_dict should
always be None.
Ok.
----------
src/modules/client/linkedimage/common.py
- in __rvdict2rv(), when you merge all the p_dict values into a p_dicts
list why not drop any None values?
Why should we?
because sine the p_dict values are put into a list and they are self
identifying (wrt linked image names), so what's the point of having None
values in the list?
also, this make it harder for consumers of the value to do anything with
it. before using any value they have to check if it's None or a
dictionary. why make every client more complex? even your own plan
description display code doesn't check for None values. see:
space_available = child_images[0]["space-available"]
Sure.
[snip]
- i'm not sure how the python subrpocess module deals with pipes and
signals. have you tested what happens if you kill a child process
while the parent attempting to read from the stdout pipe? (this can
generate a signal in the parent and unless python catches it and
handles it you might want to use a temporary file instead of pipe.)
I'll check and if necessary add something to handle the pipe exceptions.
an easy easy way to avoid the entire issue would be to use temporary
files (like we do for capturing stderr).
Sure.
[snip]
- in __display_parsable_plan(), why pass in an api_inst instead of just
passing in the plan for the current image?
Because, among other things, we may need to retrieve the license text.
where is this done?
the only reference to the api_inst object that i see in
__display_parsable_plan() is:
plan = api_inst.describe()
everything after that just accesses "plan".
You're right, I had the wrong bit of code in my head last night.
- in display_plan(), if we're in parsable output mode we don't display
any licenses or mark them as installed. is this correct? if so it
seems like parsable output mode can't actually be used to install
packages? if so, perhaps -n should be required for parsable output
mode?
No. We include the licenses and text in the parsable output.
well, perhaps i'm reading the code wrong, but it seems to me that since
you don't call display_plan_licenses(), you also don't call
set_plan_license_status(), so any licenses that requires acceptance
won't be marked as being accepted, so executing any plan that requires
license acceptance will fail in parsable output mode.
Thanks for catching that, I'll make sure that set_plan_license_status is
set and that we test that bit of functionality.
Brock
ed
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
