On 01/20/2018 07:26 PM, Alec Warner wrote: > On Sat, Jan 20, 2018 at 7:41 PM, Zac Medico <zmed...@gentoo.org > <mailto:zmed...@gentoo.org>> wrote: > > On 01/20/2018 04:23 PM, Alec Warner wrote: > > On Sat, Jan 20, 2018 at 7:10 PM, Zac Medico <zmed...@gentoo.org > <mailto:zmed...@gentoo.org> > > <mailto:zmed...@gentoo.org <mailto:zmed...@gentoo.org>>> wrote: > > > > diff --git a/pym/portage/dep/dep_check.py > b/pym/portage/dep/dep_check.py > > index 7cf338819..c56f545ec 100644 > > --- a/pym/portage/dep/dep_check.py > > +++ b/pym/portage/dep/dep_check.py > > @@ -499,7 +499,8 @@ def dep_zapdeps(unreduced, reduced, myroot, > > use_binaries=0, trees=None): > > cp_map[avail_pkg.cp] = avail_pkg > > > > new_slot_count = (len(slot_map) if graph_db is > None else > > - sum(not graph_db.match_pkgs(slot_atom) for > > slot_atom in slot_map)) > > + sum(not graph_db.match_pkgs(slot_atom) for > > slot_atom in slot_map > > + if not > slot_atom.cp.startswith("virtual/"))) > > > > > > I see this logic all over dep_zapdeps. But AFAIK its already using > > instances of Atom here. > > Can't we encode the cost inside of Atom, so that we don't need to > carve > > out that virtual atoms are 0 cost? > > > > Then we could write something like: > > > > new_slot_count = len(slot_map) if graph_db is None else > > sum(slot_atom.cost() for slot_atom in slot_map if not > > graph_db.match_pkgs(slot_atom)) > > > > Then virtuals would just have a cost() of 0. And others could have > other > > costs (or just 1.) > > That seems like a very practical approach. However, the cost is actually > a property of the matched package rather than the atom itself. It's only > because of GLEP 37 that we can treat the "virtual" category specially, > but GLEP 37 actually says nothing about the "virtual" category! There's > also a java-virtuals category! > > > This mess was the motivation for my PROPERTIES=virtual suggestion: > > > https://archives.gentoo.org/gentoo-dev/message/9d449a18a96a25a547fcfd40544085cf > > <https://archives.gentoo.org/gentoo-dev/message/9d449a18a96a25a547fcfd40544085cf> > > If we implement something like PROPERTIES=virtual, then cost becomes a > property of the package instance rather than the atom. > > > So I have two goals with my comments: > > 1) Avoid code repetition. If we are checking "if > foo.startswith('virtual/')" a bunch of places; it might be useful to > factor that out into either a helper: > > def is_virtual(atom): > return atom.startswith('virtual/') > > Or attach this directly to the Atom class so callers can just do > atom.is_virtual() > > I can kind of see why the latter might be icky (because its not > necessarily an intrinsic part of the Atom interface.) > I'm not sure its necessary to wait for PROPERTIES=virtual support to > make this change; its simply refactoring existing logic.
Maybe something like this:
diff --git a/pym/portage/dep/__init__.py b/pym/portage/dep/__init__.py
index 6ff6adcb9..bd18b5ec8 100644
--- a/pym/portage/dep/__init__.py
+++ b/pym/portage/dep/__init__.py
@@ -1322,6 +1322,8 @@ class Atom(_unicode):
self.__dict__['cpv'] = cpv
self.__dict__['version'] = extended_version
self.__dict__['repo'] = repo
+ self.__dict__['category'], self.__dict__['pn'] = catsplit(cp)
+ self.__dict__['virtual'] = self.category == 'virtual'
if slot is None:
self.__dict__['slot'] = None
self.__dict__['sub_slot'] = None
> 2) Avoid 'needing to know about virtuals'. I concede this is likely
> difficult with the current state of things. the dbapi.match calls don't
> return Packages; so we can't
> rely on packages to communicate state. This ends up with code complexity
> at call sites (e.g. if not match(atom) and isVirtual(atom) # action). I
> concede this isn't
> worth fixing right now.
The dep_zapdeps function does deal with Package instances (returned from
match_pkgs method) when processing dependencies for the depgraph, but
it's also capable of working with a regular dbapi.
--
Thanks,
Zac
signature.asc
Description: OpenPGP digital signature
