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