replies are inline below.
a new webrev is available at:
a diff against the previous webrev (145 loc) is at:
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.
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
i know the stripped action cache is used for conflict checking but i
wasn't sure if that was its only purpose, or it was used for anything
else (like pkg verify, fix, etc).
i updated the comment to read:
"""Strip actions of attributes which are unnecessary once
those actions have been installed in an image. Stripped
actions are saved in an images stripped action cache and used
for conflicting actions checks during image planning
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
# via wildcards) so the system will assign it
# a default value.
line 3545: Oh...how did this manifest itself?
this was one of those things that really annoyed me. ;)
if you use the solver to evaluate facet changes, we'll list every
package in the image as changing, even if they are not. (your fast path
optimization allow us to avoid this problem if we avoid the solver, but
that doesn't always happen.)
so here's an example of what happens without this change when we do a
change-facet operation on a machine with 4 zones (when we recurse we do
# pkg change-facet -n \
facet.version-lock.consolidation/osnet/osnet-incorporation=false 2>&1 | \
grep "Packages to change"
Packages to change: 1
| Packages to change: 244
| Packages to change: 244
| Packages to change: 281
| Packages to change: 281
with this optimization the "Packages to change" for each zone becomes 1.
also, having lots of empty pkg_plan objects unnecessarily bloats the
serialized plan description object. by stripping out empty pkg_plan
object for a small-server zone the serialized plan description went from
about 175 Kb to 98 Kb.
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'd check; also, you really care about faceted actions not for
the current variant? Or was that just to avoid variant change
i care about all facet values regardless of what they evaluate to, and
since "excludes" evaluates facets and variants at the same time i'm
forced to ignore variants (because i can't ignore things based on
i've thought about it a bit, and technically i probably should be
filtering based on variants, but given that (as i stated above) variant
and facet filtering are combined, i'd need to jump through some extra
hoops to do variant filtering (and then i'd need to write some test
cases for it, etc). given our current use of faceted dependencies i
don't think this will cause any problems, but let me know if you want me
to fix it and i will.
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 :-)
i changed is_constrained() to try and make it more generic. i renamed
has_dependency(self, fmri, dtype, excludes=EmptyI)
it returns true if the manifest has a dependency of the type specified
that on the specified fmri.
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
i don't think so. the tricky bit with doing this would be deciding if
you actually want facet.debug.* to be present within the facet
dictionary. (in which case what happens when a user un-sets it, etc.)
my guess is that this would best be implemented via hard coded behavior
in the Facet object and _allow_facet().
in either case, i'm assuming we'd still report SYSTEM as the source for
any facets which match the facet.debug.* pattern and don't match any
other user specified patterns.
lines 278-280: You saw this right? No concerns?
yes, i saw this and i looked at _allow_facet() and i have no concerns.
essentially, i haven't changed the behavior of __getitem__() directly.
what i did change was how we manage __keylist, since inherited facets
are now prioritized over local facets. but this is ok because both
__getitem__() and _allow_facet() use __keylist to do facet matching, so
the change affects both of them equally.
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.
yeah, i was trying to avoid doing this in C. ;)
my desktop has 1501 packages installed and 4305 installed incorporate
dependency actions. i'm guessing i won't see a large perf boost with
that limited number of actions, but i agree if this function is ever
used on larger sets of actions then we would want to re-write it.
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'.
i didn't realize this was a hot path. we don't have to do the filtering
here. to avoid slowing this down i've backed out my changes to
gen_actions_by_type() and created a new function
gen_filtered_actions_by_type(), that simply calls gen_actions_by_type()
and filters the results. so only new callers of new function
gen_filtered_actions_by_type() will incur any extra filtering overhead.
line 870: Do we really need to check for programmer error here?
Wasn't this primarily useful for debugging?
it would have saved me some debugging time so i added it. i assume
you're concerned about performance here since this is a hot path?
i've gone ahead and removed it.
[pkg-discuss] Re: review request: linked image facet inheritance