[pkg-discuss] Re: review request: linked image facet inheritance
- From: Shawn Walker <
- Cc: Edward Pilatowicz <
- Subject: [pkg-discuss] Re: review request: linked image facet inheritance
- Date: Sun, 04 Aug 2013 19:26:41 -0700
On 07/26/13 18:28, Edward Pilatowicz wrote:
thanks for the quick review bart.
replies are inline below.
a new webrev is available at:
general: none of the sentences following the first
in each paragraph are indented as the other
entries in this file are
line 16: s/so we're/we're/
line 19: s/./;/
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?
line 4850: s/"values"/"value"/
line 1739: What made you choose to have it list all facets,
instead of making it list only the masked ones or having
it be an option that combined with '-a' or '-i' or the
default output? I'm not saying this approach is wrong,
I'm just looking for reasoning and thoughts.
line 1203: s/. /. / (man page convention is to use one space)
line 628: They're really unnecessary because we don't need them
for conflict checking, right? Perhaps it would be useful to
state that as currently you might be left wondering why they're
line 629: s/that actions has/those actions have/
line 894: A *brief* comment explaining why it's ok to ignore wildcard
matches and fallback to _match_src might be helpful.
line 352: I'd feel safer with range(2); we'd be guaranteed to never
infinite loop. Then you could put an assert after the loop to
ensure we never retried more than we expected.
line 398: extra newline
line 462: s/Solve./Solve;/
What about pkg change-facet for a masked facet?
In other words, if I make a local facet change, but all
affected facets are already masked, and --reject wasn't
used, then that should always be the fastpath. That
doesn't seem to be accounted for here.
Likewise, if I make a local facet change, but I've already
inherited that effective setting, I see no need to invoke
the solver or load package data.
If that looks too complicated (wildcards could be a problem?)
then file an RFE for that.
line 654: Is this another bug fix? If so, oops. Thanks for that.
A separate bugdb entry might be nice to spotlight that.
general: thanks for the fastpath cleanup; that's better now;
wouldn't have thought of arranging it that way
line 1906: s/=/ = /
line 3545: Oh...how did this manifest itself?
line 3213: s/package wide/package-wide/
lines 3219-3220: Using the 'known' catalog from the image here
should be faster and use less memory than using the manifest.
I'd check; also, you really care about faceted actions not for
the current variant? Or was that just to avoid variant change
line 3268: Same as above; I'd expect it to be better to use catalog
instead. Also not thrilled about adding is_constrained() to
Manifest; seems awfully specific to solver / linked image stuff.
And the name is fairly generic for something specific to parent
dependencies. I'd suggest "is_own_grandfather()" but that would
be silly :-)
line 547: No _() concerns?
line 750: s/this the/this is the/ ?
lines 46-50: I want to make 'facet.debug' False by default in the
future; do you foresee any issues with the changes made here
lines 278-280: You saw this right? No concerns?
lines 357-380: Since this is only being used for incorporate
dependency actions, this being written in Python is probably fine. But
if we were to ever apply this to more than just those, I'd rather this
be rewritten in CPython just as the existing facet matching is done
today. It really does make a big difference.
lines 395, 408: No way to raise the exception directly instead of
generating on synthetically?
line 491: s/read only/read-only/
line 497: Insert newline above please.
lines 859-864: This is a hot path; it's unfortunate that we've had
to add attribute filtering here. To mitigate that, setting
attr_filter=None and then doing a boolean check on attr_filter
might be faster. To test, you could add something to
tests/perf/actionbench.py or create a new 'manbench.py'.
line 870: Do we really need to check for programmer error here?
Wasn't this primarily useful for debugging?
line 1302: s/childred/children/ ?
lines 1634, 1688, 1698, 1711, 1763, 1773, 2324, 2394, 2403:
s/we/when we/ ?
lines 1245, 1312, 2532, 2869, 2871, 3131: s/it's/its/
general: I did not look at every test in detail, I only looked
to see if you had sufficient coverage.
Also, what happens in the situation that an admin disables the
version-lock facet for the cacao-incorporation and then updates
entire with a specific version? I'm guessing the install holds
don't get relaxed. If so, that's ok; same thing happens now.
We should talk to Bart about whether install-hold relationships
should always apply even if the incorporate dependency is faceted
line 624: s/uninstalled/uninstall/ ?
Eyes glazed over,