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  > wrote:
> 
> On 01/20/2018 04:23 PM, Alec Warner wrote:
> > On Sat, Jan 20, 2018 at 7:10 PM, Zac Medico  
> > >> 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.

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

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  wrote:

> On 01/20/2018 04:23 PM, Alec Warner wrote:
> > On Sat, Jan 20, 2018 at 7:10 PM, Zac Medico  > > 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
>
>

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

[Zac Medico]

On 01/20/2018 04:23 PM, Alec Warner wrote:
> On Sat, Jan 20, 2018 at 7:10 PM, Zac Medico  > 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.
-- 
Thanks,
Zac



signature.asc
Description: OpenPGP digital signature

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

[Alec Warner]

On Sat, Jan 20, 2018 at 7:10 PM, Zac Medico  wrote:

> Fix new_slot_count to exclude virtual packages, since they are considered
> to have zero-cost. This solves an issue where the catalyst stage1 build
> would unexpectedly pull in static-dev to satisfy virtual/dev-manager,
> but eudev is the preferred choice.
>

> Bug: https://bugs.gentoo.org/645190
> Fixes: 9fdaf9bdbdf5 ("dep_check: use DNF to optimize overlapping virtual
> || deps (bug 632026)")
> Reported-by: Ben Kohler 
> ---
>  pym/portage/dep/dep_check.py   |  3 +-
>  .../resolver/test_virtual_minimize_children.py | 61
> ++
>  2 files changed, 63 insertions(+), 1 deletion(-)
>
> 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.)


>
> this_choice = _dep_choice(atoms=atoms, slot_map=slot_map,
> cp_map=cp_map, all_available=all_available,
> diff --git a/pym/portage/tests/resolver/test_virtual_minimize_children.py
> b/pym/portage/tests/resolver/test_virtual_minimize_children.py
> index 6eb0409f2..287445e58 100644
> --- a/pym/portage/tests/resolver/test_virtual_minimize_children.py
> +++ b/pym/portage/tests/resolver/test_virtual_minimize_children.py
> @@ -226,3 +226,64 @@ class VirtualMinimizeChildrenTestCase(TestCase):
> finally:
> playground.debug = False
> playground.cleanup()
> +
> +   def testVirtualDevManager(self):
> +   ebuilds = {
> +   'sys-fs/eudev-3.1.5': {},
> +   'sys-fs/static-dev-0.1': {},
> +   'sys-fs/udev-233': {},
> +   'virtual/dev-manager-0': {
> +   'RDEPEND': '''
> +   || (
> +   virtual/udev
> +   sys-fs/static-dev
> +   )'''
> +   },
> +   'virtual/udev-0': {
> +   'RDEPEND': '''
> +   || (
> +   >=sys-fs/eudev-2.1.1
> +   >=sys-fs/udev-217
> +   )'''
> +   },
> +   }
> +
> +   test_cases = (
> +   # Test bug 645190, where static-dev was pulled in
> instead
> +   # of eudev.
> +   ResolverPlaygroundTestCase(
> +   [
> +   'virtual/dev-manager',
> +   ],
> +   success=True,
> +   mergelist=(
> +   'sys-fs/eudev-3.1.5',
> +   'virtual/udev-0',
> +   'virtual/dev-manager-0',
> +   ),
> +   ),
> +   # Test static-dev preference.
> +   ResolverPlaygroundTestCase(
> +   [
> +   'sys-fs/static-dev',
> +   'virtual/dev-manager',
> +   ],
> +   all_permutations=True,
> +   success=True,
> +   mergelist=(
> +   'sys-fs/static-dev-0.1',
> +   'virtual/dev-manager-0',
> +   ),
> +   ),
> +   )
> +
> +   playground = ResolverPlayground(debug=False,
> ebuilds=ebuilds)
> +
> +