Re: [gentoo-portage-dev] [PATCH] dep_zapdeps: exclude virtuals from new_slot_count (bug 645190)

[Zac Medico]

On 01/20/2018 07:26 PM, Alec Warner wrote:
> On Sat, Jan 20, 2018 at 7:41 PM, Zac Medico <zmedico@g.o
> <mailto:zmedico@g.o>> wrote:
>
>     On 01/20/2018 04:23 PM, Alec Warner wrote:
>     > On Sat, Jan 20, 2018 at 7:10 PM, Zac Medico <zmedico@g.o <mailto:zmedico@g.o>
>     > <mailto:zmedico@g.o <mailto:zmedico@g.o>>> 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