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.
> >----------
> >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...
> >- 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.
> >----------
> >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"]
> >- iirc you said that if there were any error you don't want to return
> > any parsable results. if that's the case thein in __rvdict2rv()
> > perhaps we should do:
> > err = apx.LinkedImageException(bundle=errs)
> > p_dicts = []<--- add this line
> Actually, what I meant to say was that if there's an error, we don't
> want to print parsable output to the screen. I think that emptying
> p_dicts is overkill from the point of the consumer, but if I'm fine
> w/ it if that's how you'd like it.
i guess we can leave it up to the consumer to determine that.
> >- 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).
> >----------
> >src/client.py
> >
> >- why is __display_parsable_plan() part of the cli client? shouldn't it
> > be part of the PlanDescription class itself? having the
> > PlanDescription output format be part of the client seems weird.
> > other API consumers might want to output this format, etc. also,
> > presumably we might eventually want to have the plan description
> > reload a parsable plan so a python consumer could manipulate it
> > programatically?
> It's part of the cli client because that's where the rest of our
> display logic is, and this ISN'T a general purpose parsable output,
> it's being made for one specific consumer.
>
> If/when we decide we want to do something more general, we can
> rearrange the code as desired.
>
hm. i was thinking that json is a generic output agnostic format. the
fact that the json is being emitted to stdout is a detail of the client,
but the json data itself and it's format seemed more directly related to
the plan description logic itself. but if other folks don't want this
as part of the plan description object then i'm ok with leaving it as
part of the client.
> >- 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".
> >- 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.
ed
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
