hey brock,
thanks for looking at this. an updated webrev is at:
https://cr.opensolaris.org/action/browse/pkg/edp/pkgplan_cleanup2/webrev
19137 package plan object should be easier to serialize
more comments are inline below.
ed
On Fri, Jan 20, 2012 at 07:17:47PM -0800, Brock Pytlik wrote:
> On 01/20/12 15:06, Edward Pilatowicz wrote:
> >hey all,
> >
> >could i get a review for:
> >
> > https://cr.opensolaris.org/action/browse/pkg/edp/pkgplan_cleanup/
> > 19137 package plan object should be easier to serialize
> >
> >the changes should be pretty strait forward and boring. the main
> >interesting points are:
> >
> >- stop caching "image", "check_cancelation", and "progtrack" within
> > pkgplan objects, instead we now pass pointer to these objects to
> > member functions that need them.
> >
> >- switch from using pickle to json for pkgplan action serialization
> >
> >- the license.py actions loses it's preinstall() callback since we
> > dropped filelist support ages ago.
> >
> >- add a test case that verifies action serialization during staged
> > execution in child images.
> >
> >i pulled these changes out of my parallel linked images gate in an
> >effort to reduce the delta in that gate and make it easier to review.
> >(it should also have the added benefit of making it easier for me to
> >sync up with changes in the pkg gate. ;)
> >
> >thanks,
> >ed
> >_______________________________________________
> >pkg-discuss mailing list
> >[email protected]
> >http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
> General comments:
> Why all the renames from "img" to "image"? I'd prefer "img" since it
> doesn't conflict with a module that we commonly import as "image".
>
all the actions used a mixture of "image" and "img" variable names. i
just picked "image" and standardized on that. i don't have a preference
between "image" and "img" variables so i've gone ahead and updated the
actions and pkgplan to use "img" consistently (so now you'll see some
renames of "image" to "img").
> I don't understand why "image" needed to be removed from
> PkgPlan.__init__ nor why it's better to pass it in to a bunch of
> functions instead of having it set once. Just because the "image" is
> set in the PkgPlan doesn't mean it has to be part of what's
> serialized. To load a PkgPlan, you'd create one with the image you
> want it to operate on (or whatever image you're going to pass into
> evaluate or install or ...) and use setstate on it.
>
i don't think i understand the benefits to keeping an image pointer in
the PkgPlan. the disadvantages to keeping it are that it makes it more
difficult to load serialized PkgPlan objects and pass around PkgPlan
objects.
to elaborate on these two points. first, in my parallel linked image
gate i've added the ability to save an entire PlanDescription object to
disk (including PkgPlan objects) and then load them independently of
having an image. loading becomes more difficult if the PkgPlan wants to
cache an image pointer.
second, one of the eventual goals for gen_plan_*() api functions was to
have them return PlanDescription objects for all child linked images.
if PlanDescription objects reference PkgPlan objects then this becomes
difficult since the parent doesn't have an Image object for those child
images associated with the PlanDescription (and associating them with
any current Image object doesn't make any sense).
> Since progtrack and check_cancel are only used in download (from
> what I can see), I don't feel strongly about whether that should be
> treated like "image" or only passed into the download function.
>
> pkgplan:
> 111: Why is something being compared to the type of None? What else
> can be in state[i] other than a string or None? Also, while it might
> be ok in this one case, I'd prefer that isinstance was used instead
> of comparing the types directly since if something was ever a
> subclass of NoneType, an object of that type would fail this test
> (which sort of brings me back to asking why the type is being used
> at all).
>
> 151: Similar question, why isn't this just 'if state[i] is not
> None:'? What else can be in the origin or destination fmri other
> than None or a PkgFmri?
>
> 159, 161: Same questions about using type(None).
>
i did this because if you have an Action object and you try doing:
if src != None:
then you'll get an exception:
---8<---
File
"/export/ws/pkg.pkgplan_cleanup/proto/root_i386/usr/lib/python2.6/vendor-packages/pkg/actions/generic.py",
line 431, in __ne__
if self.name == other.name and \
AttributeError: 'NoneType' object has no attribute 'name'
---8<---
because the __ne__() function for action objects assumes your comparing
the current action against another action (and not some other type of
object).
so i changed all getstate() and setstate() code to use:
type(XXX) != type(None)
because that makes the comparison independent of any __ne__()
implementation in the class your comparing against (and for consistency,
i did this for all the "None" comparisons in the *state()). if you'd
like me to do this some other way then just let me know what's
preferable.
> pkg5unittest:
> 2800-2809: Shouldn't this also happen on update? I think it's
so any pkg operation that can install or update an fmri could use this
functionality (sync, update, change facet, and change variant), but
there are no other tests that would use this functionality so i didn't
add it.
> probably better located in _api_finish since update also shares
> this, and I think potentially change-variant/facet could as well
> (though that would be one weird package). It's probably also worth
i've gone ahead and moved some of the functionality into _api_finish().
> filing a test suite bug to remind us that there are instances in the
> test suite where we used self.pkg("... --accept ...") instead of the
> api because the api interface was lacking and that those should be
> changed to use the api interface now.
>
i'm not sure i understand. afaik the pkg client "api" never lacked any
functionality for updating license viewed/accepted status. is this not
the case? or are you saying that because the "pkg5unittest api" lacked
this functionality and other folks used pkg.1 interfaces instead?
> While the rest of the changes seem fine, I'd like to see another
> webrev once image gets put back into PkgPlan as that caused a lot of
> noise which made it hard to see the real changes here.
>
i think that the removal of image from the PkgPlan is beneficial for the
reasons i mentioned above.
i'm also failing to understand the benefits of keeping the image pointer
in PkgPlan. as best as i can tell the main objection to the removal of
image from the PkgPlan is that it's extra delta. is the concern over
this delta related to risk management in an update? would it help if i
isolated the image removal change into it's own wad? is there some
other reason for keeping it that i'm missing?
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
