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

[Alec Warner]

On Sat, Jan 20, 2018 at 7:41 PM, Zac Medico <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>> 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
>
> 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.

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.

-A


--
> Thanks,
> Zac
>
>