[pkg-discuss] Re: [review] pkg facet/variant subcommand enhancements
- From: Edward Pilatowicz <
- To: Shawn Walker <
- Subject: [pkg-discuss] Re: [review] pkg facet/variant subcommand enhancements
- Date: Tue, 4 Jun 2013 23:15:15 -0700
On Tue, Jun 04, 2013 at 06:23:00PM -0700, Shawn Walker wrote:
> On 05/29/13 00:10, Edward Pilatowicz wrote:
> >>Replaced webrev:
> >> http://ips.java.net/webrev/srwalker/pkg-facet-variant-1/
> >- in list_variant() and list_facet(), if the user specified a
> > variant/facet to match and none was found, is it really right to
> > return EXIT_OOPS? returning EXIT_OOPS (instead of EXIT_OK) means that
> > a caller can't distinguish between a runtime failure and the lack of
> > matching variants/facets.
> By runtime failure, I assume you mean invalid input? Yes, but that
> is the same set of rules we apply for pkg info, list, etc. so I
> think that ship has sailed already :-)
no. a runtime error is something like running out of memory. or
getting an interrupted system call, etc.
> My inclination is to leave this as-is here and make a single pass to
> clean-up all of them later.
> At least this uses the new options parsing whereas many of the older
> output-format-using functions don't at all.
> I don't know what the right answer is long term.
i don't either. i was just pointing it out. i'm fine with that.
> >- in gen_facets(), you initialize "facets" as to contain all facets
> > explicitly set in the image, but when you yield the results you index
> > "facets" by facets that are not necessarily set, but are just present
> > in the image.
> What's the concern?
> The facets object is a bit magical in that if you index facets not
> explicitly set in the image, it gives you back the effective setting
> for that facet.
yeah. i eventually figured that out after digging into the facet
object. it just wasn't clear what the behavior would be from reading
this code though.
> >- in __plan_install(), it seems like you should call
> > self.__evaluate_varcets() before checking if there is no work to do
> > (this is what you do in plan_change_varcets()).
> Hmm, I've made that change but now I wonder. The old code didn't do
> that (see original __plan_install_solver).
yeah, the old code didn't do that, but your new code did. it seems
inconsistent to be doing the same thing two different ways so i'll let
you figure out which way is right and make sure they both do the same
> >- it seems to me like the optimization that you added to
> > plan_change_varcets() to calculate fmri_changes should actually live
> > in __plan_install(). that way if we eventually provided an way for
> > callers to install packages and change frmis at the same time (which
> > we want to do), we wouldn't either lose or have to refactor this
> > optimization.
> That will have to be left for later; I'm not keen on adding an
> optimisation for functionality that doesn't exist and therefore
> can't be tested yet :-) For now, I've added a comment to
> __plan_install noting that.
i really think that this code should all be in __plan_install(). the
idea behind __plan_install() is that things like install, sync, change
facet, and change variant are really all supposed to go through a common
this would not just benefit a unified operation mode. if we want to fix
15743323 (having linked images should inherit some facets from parents)
then we'd also want to move this code into __plan_install(), since
inherited facets could change during any of these child image
i won't insist on it, but since it's likely i'll end up fixing 15743323
at some point i'm just trying to save myself some work. :)
> >- in _gen_attrs_to_str() you do:
> > if name[:8] == "variant.":
> > instead of hard coding a string and it's length, why not just use
> > startswith()? this type of comparison is done in multiple other
> > places.
> Because slices are ~5% faster on x86 and ~14% faster on some SPARC
> systems since startswith involves a function call:
> >- in _gen_attrs_to_str(), you do:
> > vfacets = facets.values()
> > vcfacets = vfacets.intersection(*vfacets[1:])
> > ...
> > for varkey, fnames in facets.items():
> > fnames.difference_update(vcfacets)
> > if not fnames:
> > del facets[varkey]
> > since vcfacets is a set of all known facets, doesn't that guarantee
> > that the call to difference_update() will leave fnames as an empty
> > set? it seems that the "for" loop above is unnecessary because once
> > it's done your guaranteed to have removed ever entry from facets
> No, some variants may only share a subset of facets.
i thought that in the code above we've already determined that it's safe
to merge facets, so all facets apply to all variants. (also, the code
above doesn't look at any variant values.)
> >- in _gen_attrs_to_str(), you do:
> > for ref in refs:
> > ...
> > if not ref:
> > csize += rcsize
> > size += rsize
> > continue
> > refs is a defaultdict using sets as keys, so it seems like it's only
> > possible to have a single key for which "not ref" is true (an empty
> > set). if that's the case there is no reason to use "+=" in the
> > statements above (since to me the use of += implies that the
> > statements could be executed multiple times).
> No, the += is necessary because I'm not guaranteed to get the "not
> ref" case first.
but it doesn't matter when you get the "not ref" case (first, last, or
in the middle). as long as there is only one instance when "not ref" is
true, then you don't need "+=", you can just use "=", right?
> >- in get_size(), you try to get size/csize from the attributes, and if
> > you calculate them you add them as attributes, but this ignores
> > different "excludes" values that could be passed in. ie, if you cache
> > a value for excludes=EmptyI, and then this function is called again
> > with a different excludes value, it seems like you'll return the wrong
> > result.
> The cache is always generated with no excludes applied. In
> addition, if the manifest is ever reloaded or exclude_content is
> called, self.attributes will be cleared.
it'd be nice to have a comment saying that.
> >- in gen_facets(), it'd be nice if the docstring described the format of
> > the returned data similar to how gen_variants() does.
> Except they're very different; gen_facets() returns the supported
> facet attributes (strings) while gen_variants() returns tuples. You
> want me to add the (strings) bit?
so we're just returning facet name strings because all facets defined in
packages can only have a value of true, right? (it's only when you have
an image that you can set facet values to false.) if so it would be
nice having a comment that explains all that.
ps - a related question about facets in general. while thinking about
15743323, i was playing around and i discovered that package level
variants get applied to all actions within a package, but package level
facets do not. ie, if i define a package with:
set name=facet.foo value=true
set name=variant.foo value=bar
and install that package, then setting facet.foo=false has no effect on
the image content delivered by the package. ie, any actions that were
delivered by the package continue to be delivered. but if i set
variant.foo=baz then the package get's uninstalled from the image. is
this the expected behavior?