Bug 645914 – virtual deps no longer correctly satisfied when using ||() deps

• Product: Portage Development
• Status: CONFIRMED ---

[Description SpanKY 2018-01-27 20:24:57 UTC]

the change that went in for bug 526160 broke virtual dep checking in ||() deps.

https://gitweb.gentoo.org/proj/portage.git/commit/?id=b7fdd57d50d0956420d73b958eb826ba6eab513a
_dep_check_composite_db: fix bug #526160

here's a test case:

$ cat virtual/target-os/target-os-1.ebuild 
EAPI="5"
SLOT="0"
KEYWORDS="*"
RDEPEND="
        virtual/implicit-system
        virtual/awk
"

$ cat virtual/implicit-system/implicit-system-1.ebuild 
EAPI="5"
SLOT="0"
KEYWORDS="*"
RDEPEND="
        sys-apps/mawk
"

we first build up binpkgs (i don't think it's a requirement, but does make it easier to see).
# emerge -b virtual/target-os

then we look at the deptree:
$ ROOT=/var/empty emerge -eKp --tree virtual/target-os
[binary  N     ] virtual/target-os-1::xxx to /var/empty/
[binary  N     ]  virtual/implicit-system-1::xxx to /var/empty/
[binary  N     ]   sys-apps/mawk-1.3.4_p20171017-r1::gentoo to /var/empty/
[binary  N     ]    app-eselect/eselect-awk-0.2::gentoo to /var/empty/
[binary  N     ]  virtual/awk-1::gentoo to /var/empty/
[binary  N     ]   sys-apps/gawk-4.1.4::gentoo to /var/empty/
[binary  N     ]    sys-libs/readline-7.0_p3:0/7::gentoo to /var/empty/
[binary  N     ]     sys-libs/ncurses-6.0-r2:0/6::gentoo to /var/empty/

before this commit, gawk wouldn't be pulled in ... the explicitly selected mawk in the depgraph would have satisfied the ||() dep in virtual/awk.

[Comment 1 Zac Medico 2018-01-27 21:32:01 UTC]

Created attachment 516924 [details]
unit test for virtual/target-os

This unit test is passing with current master branch (5c888fcd2e87270523f55fb180ced5c3c8689f76) and portage-2.3.20.

If it also works for you in practice, we can go ahead and include this in our test suite in order to guard against regressions.

[Comment 2 Zac Medico 2018-01-27 21:43:13 UTC]

Created attachment 516926 [details]
unit test for virtual/target-os with targetroot=True

This the the same as the other unit test, but sets targetroot=True, and it still succeeds.

[Comment 3 SpanKY 2018-01-27 23:46:45 UTC]

i'm testing with portage-2.3.20 already ... the emerge output i posted here was from that.  i wasn't showing behavior from the older CrOS versions (although that's where i discovered it initially).

are my test ebuilds not showing the same problem for you w/2.3.20 ?

[Comment 4 Zac Medico 2018-01-28 02:38:58 UTC]

Created attachment 516934 [details]
modified version of pym/portage/tests/emerge/test_simple.py

(In reply to SpanKY from comment #3)
> are my test ebuilds not showing the same problem for you w/2.3.20 ?

Right, I can't reproduce it. Attached is a modified version of pym/portage/tests/emerge/test_simple.py which calls emerge with some generated binary package stubs, and the result for your `ROOT=/var/empty emerge -eKp --tree virtual/target-os` command looks like this:

> $ pym/portage/tests/runTests.py pym/portage/tests/emerge/test_emerge_virtual_target_os.py
> testSimple (portage.tests.emerge.test_emerge_virtual_target_os.SimpleEmergeTestCase) ... 
> These are the packages that would be merged, in reverse order:
> 
> Calculating dependencies... done!
> [binary  N     ] virtual/target-os-1 to /tmp/tmpybcrw1nr/cross_root/tmp/tmpybcrw1nr/
> [binary  N     ]  virtual/implicit-system-1 to /tmp/tmpybcrw1nr/cross_root/tmp/tmpybcrw1nr/
> [binary  N     ]   sys-apps/mawk-1.3.4_p20171017-r1 to /tmp/tmpybcrw1nr/cross_root/tmp/tmpybcrw1nr/
> [binary  N     ]    app-eselect/eselect-awk-0.2 to /tmp/tmpybcrw1nr/cross_root/tmp/tmpybcrw1nr/

[Comment 5 Zac Medico 2018-01-28 03:13:10 UTC]

Created attachment 516936 [details]
unit test for virtual/target-os

I just noticed that my test cases use || ( virtual/implicit-system virtual/awk ) for the target-os-1 RDEPEND that's not what you have. I can reproduce the problem with RDEPEND="virtual/implicit-system virtual/awk".

[Comment 6 Zac Medico 2018-01-28 03:51:38 UTC]

It seems like maybe it chose mawk for you before out of luck, since the outcome depends on the order of evaluation of the virtual/implicit-system and virtual/awk dependencies. Reversing the order to atoms in the virtual/target-os-1 dependencies is enough to change the outcome:

RDEPEND="
        virtual/awk
        virtual/implicit-system
"

The fact that virtual/implicit-system is a virtual is working against you here, since portage delays evaluation of virtuals by queuing them in the depgraph's _dep_disjunctive_stack. If virtual/implicit-system was not a virtual, then it's dependencies would be evaluated earlier, and the preference for mawk would propagate to virtual/awk when it is evaluated later.

Ultimately, the plan is to evaluate all overlapping || deps and virtuals in a combined DNF expression (like the solution for bug 632026), which will cause the preference for mawk to be appropriately recognized during evaluation of the DNF expression.

[Comment 7 Zac Medico 2018-01-28 04:40:12 UTC]

I have an unfinished patch solves the problem by making it evaluate the virtual dependencies of virtual/target-os-1 all together:

https://github.com/zmedico/portage/tree/bug_645914

[Comment 8 SpanKY 2018-01-29 06:55:11 UTC]

we didn't actually need the virtual/awk dep, so i just dropped it to avoid the issue.  hopefully it doesn't show up elsewhere ;).

do we still need to delay virtual evaluation anymore now that they're "normal" packages ?  the test case i posted was reduced down ... in reality we have virtuals on top of virtuals, so doing immediate evaluation on the top node wouldn't have helped.

[Comment 9 Zac Medico 2018-01-29 07:38:04 UTC]

Yes, the delayed virtual evaluation solves many problems. The idea is that we evaluate virtuals late so what we have the benefit of all of the earlier package selections to guide the selection of packages to satisfy the virtual. This means that the selection of packages to satisfy the virtual is less likely to choose the wrong package, since more information is available.

This test case is a reasonable demonstration:

# Test swapping of providers for a new-style virtual package,
# which relies on delayed evaluation of disjunctive (virtual
# and ||) deps as required to solve bug #264434. Note that
# this behavior is not supported for old-style PROVIDE virtuals,
# as reported in bug #339164.
ResolverPlaygroundTestCase(
	["media-video/libav"],
	success=True,
	mergelist = [
		'media-video/libav-0.7_pre20110327',
		'[uninstall]media-video/ffmpeg-0.7_rc1',
		'!media-video/ffmpeg',
	])

Since media-video/libav is added to the graph earlier, it makes it possible to prefer the media-video/libav choice when satisfying virtual/ffmpeg, and then uninstall media-video/ffmpeg since it is no longer needed.

This test case would work the same way if media-video/libav was a dependency of some random package in @world, since that dependency would be evaluated early in comparison to the delayed evaluation of virtual/ffmpeg.