[pkg-discuss] Re: review request: linked image facet inheritance
- From: Edward Pilatowicz <
- To: Shawn Walker <
- Subject: [pkg-discuss] Re: review request: linked image facet inheritance
- Date: Wed, 7 Aug 2013 12:14:29 -0700
replies are inline below.
a new webrev is available at:
a diff against the previous webrev (57 loc) is at:
On Tue, Aug 06, 2013 at 05:00:17PM -0700, Shawn Walker wrote:
> On 08/06/13 09:33, Edward Pilatowicz wrote:
> >replies are inline below.
> >a new webrev is available at:
> > https://ips.java.net/webrev/edp/15743323.3/
> >a diff against the previous webrev (145 loc) is at:
> > https://ips.java.net/webrev/edp/15743323.3.diff/
> My replies are against the diff.
> Be sure to file a separate bug just for the man page changes for Alta.
> >On Sun, Aug 04, 2013 at 07:26:41PM -0700, Shawn Walker wrote:
> >>On 07/26/13 18:28, Edward Pilatowicz wrote:
> >> line 4745: I see you accounted for list_masked here, but
> >> we don't actually have any command line options for it
> >> or do anything with it. Any need to keep this?
> >list_facet() and list_variant() both use the common options processing
> >and both use the same options table definition, client.py`opts_list_varcet.
> >so i couldn't add list_masked to just list_facet() without creating
> >separate opts_list_facet() and opts_list_variant() options tables.
> But 'opts_list_varcet' is just a list. It seems like you could
> easily just do this and then drop line 5846 and the list_masked from
> opts_list_facet = \
> [opts_cb_varcet] + \
> opts_list_varcet + \
> ("list_masked", False),
doh. i missed the fact that there's already separate opts_list_facet
and opts_list_variant arrays. i thought there was one opts_list_varcet
array and it was shared for both subcommands. i've updated this so that
only opts_list_facet has the new option.
> >> line 894: A *brief* comment explaining why it's ok to ignore wildcard
> >> matches and fallback to _match_src might be helpful.
> >i've added the following comment:
> > # this facet is not set (directly or
> > indirectly
> > # via wildcards) so the system will
> > assign it
> > # a default value.
> Here's why that's confusing to me -- the condition that's being
> checked for is "if name not in facets"; but doesn't that only return
> True for exact entries and not wildcard matches?
oops. i screwed that up. i changed the code to read:
# check if the facet is explicitly set.
if name not in facets:
# The image's Facets dictionary will return
# the effective value for any facets not
# explicitly set in the image (wildcards or
# implicit). _match_src() will tell us how
# that effective value was determined (via a
# local or inherited wildcard facet, or via a
# system default).
# This is an explicitly set facet.
does this make things clearer?
> >> lines 3219-3220: Using the 'known' catalog from the image here
> >> should be faster and use less memory than using the manifest.
> >i don't understand what you mean. are your suggesting using different
> >interfaces to iterate over the manifests and actions in place of
> >get_manifest() and gen_actions*()?
> I was suggesting using the catalog interfaces instead of the
> Manifest interfaces; for an example, see lines 4521-4528 in
> But this is really just potential optimisation work, so you could
> file an ER for that to avoid delaying pushing your changes.
ah. ok. i switched to using the catalog interfaces for iterating over
dependency actions. this allowed me to delete all my changes to
i have clean baselines and one more set of manual tests i want to run,
so assuming this looks ok i think i can probably still get this