Fix edge-cases of virtual packages #203
Conversation
- Sometimes multiple versions of the same package are available, just pick the most recent one - Fix comparison in version_constraint
apt/private/apt_dep_resolver.bzl
Outdated
| # First check if the constraint is satisfied by a virtual package | ||
| virtual_packages = state.repository.virtual_packages(name = name, arch = arch) | ||
| if not len(virtual_packages): | ||
| virtual_packages = state.repository.virtual_packages(name = name, arch = "all") |
There was a problem hiding this comment.
should this be merged with other arch virtual packages even if there are some?
There was a problem hiding this comment.
That's a good point. In the case we encountered there was no arch-specific virtual package but some all virtual package but I don't think it's impossible to have some of both?
I have changed the code to extend the initial list with the one from the all architecture.
| fail("Per https://www.debian.org/doc/debian-policy/ch-relationships.html only = is allowed for Provides field.") | ||
|
|
||
| return _version_relop(va[1], vb[1], va[0]) | ||
| return _version_relop(vb[1], va[1], va[0]) |
There was a problem hiding this comment.
can you explain this change?
There was a problem hiding this comment.
_version_relop(va, vb, op) will return the result of va {op} vb
In the context of is_satisfied_by, va is a constraint (e.g. >= 3.8.1) and vb is a version to be tested (e.g. = 3.8.3-1.1ubuntu4) so before this change we end up testing {constraint_version} {constraint_op} {test_version} (e.g. 3.8.1 >= 3.8.3-1.1ubuntu4) which rejects viable candidates (and could accept wrong candidates).
With this change we do the correct operation.
Note that there is some tests here but they all pass because for these specific test cases the inversion doesn't matter. In particular if you change the last one to (">> 1.1", "= 1.0", False) or the second one to ("<= 1.1", "= 1.0", True), then the test fail on main. I'm happy to add a couple extra cases to this test in the PR 🙏
gergondet-woven
left a comment
gergondet-woven
left a comment
There was a problem hiding this comment.
Thanks for the quick review.
I'm happy to add extra test cases to _is_satisfied_by_test to highlight the issue that was fixed 🙏
apt/private/apt_dep_resolver.bzl
Outdated
| # First check if the constraint is satisfied by a virtual package | ||
| virtual_packages = state.repository.virtual_packages(name = name, arch = arch) | ||
| if not len(virtual_packages): | ||
| virtual_packages = state.repository.virtual_packages(name = name, arch = "all") |
There was a problem hiding this comment.
That's a good point. In the case we encountered there was no arch-specific virtual package but some all virtual package but I don't think it's impossible to have some of both?
I have changed the code to extend the initial list with the one from the all architecture.
| fail("Per https://www.debian.org/doc/debian-policy/ch-relationships.html only = is allowed for Provides field.") | ||
|
|
||
| return _version_relop(va[1], vb[1], va[0]) | ||
| return _version_relop(vb[1], va[1], va[0]) |
There was a problem hiding this comment.
_version_relop(va, vb, op) will return the result of va {op} vb
In the context of is_satisfied_by, va is a constraint (e.g. >= 3.8.1) and vb is a version to be tested (e.g. = 3.8.3-1.1ubuntu4) so before this change we end up testing {constraint_version} {constraint_op} {test_version} (e.g. 3.8.1 >= 3.8.3-1.1ubuntu4) which rejects viable candidates (and could accept wrong candidates).
With this change we do the correct operation.
Note that there is some tests here but they all pass because for these specific test cases the inversion doesn't matter. In particular if you change the last one to (">> 1.1", "= 1.0", False) or the second one to ("<= 1.1", "= 1.0", True), then the test fail on main. I'm happy to add a couple extra cases to this test in the PR 🙏
2 participants
This fixes some edge-cases with virtual packages resolution introduced in #146
One way to reproduce this is to use the following manifest slightly adapted from the examples folder to:
Example manifest
This will build but you should see the following warning messages with the current main version:
The first two are solved by picking whichever of the package has the most recent version.
For the
libgnutls30issue reported here, I had to add some extra ouptut to figure things out. In this version of the snapshot, the package is required with a constraint>= 3.8.1and the packages available in the snapshot have versions3.8.3-1.1ubuntu3and3.8.3-1.1ubuntu4. They should both satisfy the constraint but because the operands are inverted they are rejected.The last point is more peculiar. We saw this happen with the OpenVINO apt distribution (see here) but I think it's fairly reasonable to fallback to the all architecture just in case 🤔