Brock Pytlik wrote:
image.py:
set_package_state: It looks to me like there are paths through the code
which could end up hitting line 978 wihtout defining icat. For example,
suppose 'state' is not installed nor uninstalled, but installed is in
'states' wouldn't that mean icat wouldn't be set when you hit line 978?
Yes, icat can be undefined, thanks for spotting that particular case.
It might be too late for this, but I wonder if a dictionary for the
states wouldn't be a better data structure and result in clearer code.
With the sets approach, there's nothing inherent in the structure that
prevents a package from bring both installed and uninstalled. A
structure like state[installed] = True|False, state[version]=0|1, etc...
might make things easier to deal with.
A dictionary (in the actual catalog entry) doesn't work due to
performance issues with simplejson. Hence, a list. I'm aware that
there's a possibility for bad state information here, but the only
feasible way I have to deal with it for now is through some extra sanity
checks.
So, the path I'll likely take here is to add some simple state checks
that raise an appropriate exception if something bizarre happens.
1167-1168: might we want to store the optional dependencies as well? I'd
think the times when we want the require dependencies, we'd often want
the optional or incorporate ones as well.
I'm lost about the storing bit; this code is exactly the same as it was
before logic-wise. This code isn't "storing" dependencies afaik.
With that said, note the comment on the __build_dependents function:
"""Build a dictionary mapping packages to the list of packages
that have required dependencies on them.""
^^^^^^^^
I don't know if it should only be doing required dependencies, but
that's what the comment indicates and what it was doing before...
imageplan.py:
307: Why is this check necessary since you've ask the catalog for
dependency actions? Or maybe I should ask why cat.DEPENDENCY spits back
set actions along with depend actions...
If you see the comments on the various functions in catalog, DEPENDENCY
gives back any *dependency* related actions. Variants, facets,
obsolete, and rename, as well as depend actions all play into dependency
calculation. It's a bit late to change this. cat.DEPENDENCY also
matches the name of the corresponding catalog part (e.g.
catalog.dependency.C).
catalog.py:
1054+: I'm confused about the logic here and why DEPENDENCY results in
the set of actions that it does. If nothing else, renaming the constant
might be helpful.
It's called dependency, because it implies *dependency-related* things.
1326: 'excludes' is the list of things that should be allowed? .... and
it's empty be default...
Ask Bart; this is the way we describe and use excludes in Manifest, etc.
It made me do a double-take the first time too.
1371, 1488, 2231, 2315: this seems like it should raise an exception
rather than assert, unless you've done a check higher up to prevent this
from happening
These are primarily sanity checks to ensure consumers are using the API
properly. I debated whether there should really be formal exceptions
for these since essentially, this is the result of a broken API consumer.
Cheers,
--
Shawn Walker
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
