On 08/25/10 06:36 PM, Dan Price wrote:
On Fri 20 Aug 2010 at 04:26PM, Brock Pytlik wrote:
Hey Dan, here's the wad for the pkgdep stuff I talked w/ you about the
other day. Thanks for taking a look at it,
Brock
-------- Original Message --------
Subject: [pkg-discuss] Code review for 15958 please
Date: Thu, 19 Aug 2010 23:28:13 -0700
From: Brock Pytlik [1]<[email protected]>
To: pkg discuss [2]<[email protected]>
Webrev:
[3]http://cr.opensolaris.org/~bpytlik/ips-15958-v1/
Bugs:
15958 generate gets partially satisfied internal dependencies wrong
16807 VariantSets is the wrong abstraction
16808 pkgdepend generate should complain if a package's variants don't
make sense
This is the first step in getting ON pkgdep clean. It fixes a bug
currently bothering ON (15958) and reworks the abstractions around
variants significantly, hopefully leading to code that's easier to
understand and debug.
Couple of minor things. In MissingPackageVariantError, I see that the
error will print out like this (also seen in the corresponding test case):
The action delivering var/log/syslog is tagged with a variant ...
The actions's variants are:
VariantTemplate({'variant.opensolaris.zone': set(['global'])})
The packages variants are: VariantTemplate({})
So: actions's -> action's packages -> package's
Also, I wonder if this should be pretty-printed as per our usual form?
The action's variants are: variant.opensolaris.zone=global
The package's variants are:<none>
So I guess VariantCombinationTemplate or whatever would need a
__str__().
I've changed this so that it prints as... '...are:
variant.opensolaris.zone="global","nonglobal" variant.arch="x86","sparc"'
[snip]
VariantCombinations.__init__()-- it would help me understand the code if I knew
what the arguments to __init__() actually represented... just a comment will
help. Also, would the code be any simpler by use the combinatorics stuff from
itertools here? Perhaps not-- but anyway I found the logic at 138-151 tough
to suss out.
I couldn't find a way to simplify it using itertools. I did add some
more comments to try to make things clearer.
VariantCombinations.copy() -- needed because...? Would a deep
copy do the trick here and be more future proof?
(http://docs.python.org/library/copy.html)?
I've changed it to be a deep copy of the sets, but a shallow copy of the
sets' elements. Since the contents of the sets are immutable, sharing
them should be fine.
simplify: does vct need to be a param, or could it just be saved in the
constructor?
It has to be a parameter because sometimes simplification is done
against a superset of the template the Combinations was created with.
The dup_variant_deps example in t_pkgdep.py (it's part of test 14118),
contains examples where this will happen. The package has
variant.foo=bar, baz, but the dependencies are tagged with
variant.foo=bar, and variant.foo=baz (for dependencies on f3 and f4
respectively). When simplifying the variants for those dependencies
later on, the simplification needs to be done against the package
variants, not the variants the dependencies had at the time of creation.
I have stored the template used at creation time as it can be used to
sanity check things like merge to ensure they're not mixing templates.
[snip]
Meta-comment: Does simplify actually need to be destructive? Maybe that's part
of the pretty printing, leaving the object undamaged.
I don't think either of these will really be possible. It can't be part
of the pretty printing because simplification happens prior to
conversion to standard dependency actions, while only standard
dependency actions are produced (unless an api debugging switch is
flipped).
Meta-comment: would implementing iterators here help you.
I don't think iterators exactly would help me, but I've made an attempt
to make simplify non-destructive and used properties to make everything
work. I think it makes the code a bit more complicated, but it does mean
that simplify isn't destructive.
dependencies.py:
[snip]
I'll be putting out a new webrev shortly once I check the code coverage.
Thanks,
Brock
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
