Fix virtual package resolution

apt/private/apt_deb_repository.bzl

@@ -92,14 +92,45 @@ def _parse_repository(state, contents, root):
             pkg = {}
 
 def _add_package(state, package):
-    util.set_dict(state.packages, value = package, keys = (package["Architecture"], package["Package"], package["Version"]))
+    util.set_dict(
+        state.packages,
+        value = package,
+        keys = (package["Architecture"], package["Package"], package["Version"]),
+    )
 
     # https://www.debian.org/doc/debian-policy/ch-relationships.html#virtual-packages-provides
     if "Provides" in package:
-        provides = version_constraint.parse_dep(package["Provides"])
-        vp = util.get_dict(state.virtual_packages, (package["Architecture"], provides["name"]), [])
-        vp.append((provides, package))
-        util.set_dict(state.virtual_packages, vp, (package["Architecture"], provides["name"]))
+        for virtual in version_constraint.parse_depends(package["Provides"]):
+            providers = util.get_dict(
+                state.virtual_packages,
+                (package["Architecture"], virtual["name"]),
+                [],
+            )
+
+            # If multiple versions of a package expose the same virtual package,
+            # we should only keep a single reference for the one with greater
+            # version.
+            for (i, (provider, provided_version)) in enumerate(providers):
+                if package["Package"] == provider["Package"] and (
+                    virtual["version"] == provided_version
+                ):
+                    if version_constraint.relop(
+                        package["Version"],
+                        provider["Version"],
+                        ">>",
+                    ):
+                        providers[i] = (package, virtual["version"])
+
+                    # Return since we found the same package + version.
+                    return
+
+            # Otherwise, first time encountering package.
+            providers.append((package, virtual["version"]))
+            util.set_dict(
+                state.virtual_packages,
+                providers,
+                (package["Architecture"], virtual["name"]),
+            )
 
 def _virtual_packages(state, name, arch):
     return util.get_dict(state.virtual_packages, [arch, name], [])
@@ -149,13 +180,17 @@ def _create_test_only():
         virtual_packages = dict(),
     )
 
+    def reset():
+        state.packages.clear()
+        state.virtual_packages.clear()
+
     return struct(
         package_versions = lambda **kwargs: _package_versions(state, **kwargs),
         virtual_packages = lambda **kwargs: _virtual_packages(state, **kwargs),
         package = lambda **kwargs: _package(state, **kwargs),
         parse_repository = lambda contents: _parse_repository(state, contents, "http://nowhere"),
         packages = state.packages,
-        reset = lambda: state.packages.clear(),
+        reset = reset,
     )
 
 DO_NOT_DEPEND_ON_THIS_TEST_ONLY = struct(

apt/private/apt_dep_resolver.bzl

@@ -6,14 +6,41 @@ load(":version_constraint.bzl", "version_constraint")
 def _resolve_package(state, name, version, arch):
     # First check if the constraint is satisfied by a virtual package
     virtual_packages = state.repository.virtual_packages(name = name, arch = arch)
-    for (provides, package) in virtual_packages:
-        provided_version = provides["version"]
-        if not provided_version and version:
-            continue
-        if provided_version and version:
-            if version_constraint.is_satisfied_by(version, provided_version):
+
+    candidates = [
+        package
+        for (package, provided_version) in virtual_packages
+        # If no version constraint, all candidates are acceptable.
+        # else, only candidates matching is_satisfied_by are acceptable.
+        if not version or (
+            provided_version and version_constraint.is_satisfied_by(version, provided_version)
+        )
+    ]
+
+    if len(candidates) == 1:
+        return candidates[0]
+
+    if len(candidates) > 1:
+        for package in candidates:
+            # Return 'required' packages immediately since it is implicit that
+            # they should exist on a default debian install.
+            # https://wiki.debian.org/Proposals/EssentialOnDiet.
+            #
+            # Packages would ideally specify a default through an alternative:
+            #
+            #  Depends: mawk | awk
+            #
+            # In the case of required packages, these defaults are not specified.
+            if "Priority" in package and package["Priority"] == "required":
                 return package
 
+        # Otherwise, we can't disambiguate the virtual package providers so
+        # choose none and warn.
+        print("Multiple candidates for virtual package '{}': {}".format(
+            name,
+            [package["Package"] for package in candidates],
+        ))
+
     # Get available versions of the package
     versions_by_arch = state.repository.package_versions(name = name, arch = arch)
     versions_by_any_arch = state.repository.package_versions(name = name, arch = "all")
@@ -66,7 +93,7 @@ def _resolve_all(state, name, version, arch, include_transitive = True):
         (name, version, dependency_group_idx) = stack.pop()
 
         # If this iteration is part of a dependency group, and the dependency group is already met, then skip this iteration.
-        if dependency_group_idx > -1 and dependency_group[dependency_group_idx]:
+        if dependency_group_idx > -1 and dependency_group[dependency_group_idx][0]:
             continue
 
         package = _resolve_package(state, name, version, arch)
@@ -82,7 +109,7 @@ def _resolve_all(state, name, version, arch, include_transitive = True):
 
         # If this package was requested as part of a dependency group, then mark it's group as `dependency met`
         if dependency_group_idx > -1:
-            dependency_group[dependency_group_idx] = True
+            dependency_group[dependency_group_idx] = (True, dependency_group[dependency_group_idx][1])
 
         # set the root package, if this is the first iteration
         if i == 0:
@@ -113,14 +140,21 @@ def _resolve_all(state, name, version, arch, include_transitive = True):
             if type(dep) == "list":
                 # create a dependency group
                 new_dependency_group_idx = len(dependency_group)
-                dependency_group.append(False)
-                for gdep in dep:
+                dependency_group.append((False, " | ".join([p["name"] for p in dep])))
+
+                # Dependencies should be searched left to right, given it is a
+                # stack it means we need to push in reverse order.
+                for gdep in reversed(dep):
                     # TODO: arch
                     stack.append((gdep["name"], gdep["version"], new_dependency_group_idx))
             else:
                 # TODO: arch
                 stack.append((dep["name"], dep["version"], -1))
 
+    for (met, dep) in dependency_group:
+        if not met:
+            unmet_dependencies.append((dep, None))
+
     return (root_package, dependencies, unmet_dependencies)
 
 def _create_resolution(repository):

apt/tests/resolution_test.bzl

@@ -185,37 +185,71 @@ resolve_architecture_specific_packages_test = unittest.make(_resolve_architectur
 def _resolve_aliases(ctx):
     env = unittest.begin(ctx)
 
-    idx = _make_index()
-
-    idx.add_package(package = "foo", depends = "bar (>= 1.0)")
-    idx.add_package(package = "bar", version = "0.9")
-    idx.add_package(package = "bar-plus", provides = "bar (= 1.0)")
-
-    (root_package, dependencies, _) = idx.resolution.resolve_all(
-        name = "foo",
-        version = ("=", _test_version),
-        arch = "amd64",
-    )
-    asserts.equals(env, "foo", root_package["Package"])
-    asserts.equals(env, "amd64", root_package["Architecture"])
-    asserts.equals(env, "bar-plus", dependencies[0]["Package"])
-    asserts.equals(env, 1, len(dependencies))
-    idx.reset()
-
-    idx.add_package(package = "foo", depends = "bar (>= 1.0)")
-    idx.add_package(package = "bar", version = "0.9")
-    idx.add_package(package = "bar-plus", provides = "bar (= 1.0)")
-    idx.add_package(package = "bar-clone", provides = "bar")
-
-    (root_package, dependencies, _) = idx.resolution.resolve_all(
-        name = "foo",
-        version = ("=", _test_version),
-        arch = "amd64",
-    )
-    asserts.equals(env, "foo", root_package["Package"])
-    asserts.equals(env, "amd64", root_package["Architecture"])
-    asserts.equals(env, "bar-plus", dependencies[0]["Package"])
-    asserts.equals(env, 1, len(dependencies))
+    def with_package(**kwargs):
+        def add_package(idx):
+            idx.add_package(**kwargs)
+
+        return add_package
+
+    def check_resolves(with_packages, resolved_name):
+        idx = _make_index()
+
+        for package in with_packages:
+            package(idx)
+
+        (root_package, dependencies, _) = idx.resolution.resolve_all(
+            name = "foo",
+            version = ("=", _test_version),
+            arch = "amd64",
+        )
+        asserts.equals(env, "foo", root_package["Package"])
+        asserts.equals(env, "amd64", root_package["Architecture"])
+
+        if resolved_name:
+            asserts.equals(env, 1, len(dependencies))
+            asserts.equals(env, resolved_name, dependencies[0]["Package"])
+        else:
+            asserts.equals(env, 0, len(dependencies))
+
+    # Version match
+    check_resolves([
+        with_package(package = "foo", depends = "bar (>= 1.0)"),
+        with_package(package = "bar", version = "0.9"),
+        with_package(package = "bar-plus", provides = "bar (= 1.0)"),
+    ], resolved_name = "bar-plus")
+
+    # Version match, ignores un-versioned
+    check_resolves([
+        with_package(package = "foo", depends = "bar (>= 1.0)"),
+        with_package(package = "bar", version = "0.9"),
+        with_package(package = "bar-plus", provides = "bar (= 1.0)"),
+        with_package(package = "bar-clone", provides = "bar"),
+    ], resolved_name = "bar-plus")
+
+    # Un-versioned match
+    check_resolves([
+        with_package(package = "foo", depends = "bar"),
+        with_package(package = "bar-plus", provides = "bar"),
+    ], resolved_name = "bar-plus")
+
+    # Un-versioned match, multiple provides
+    check_resolves([
+        with_package(package = "foo", depends = "bar"),
+        with_package(package = "bar-plus", provides = "bar, baz"),
+    ], resolved_name = "bar-plus")
+
+    # Un-versioned match, versioned provides
+    check_resolves([
+        with_package(package = "foo", depends = "bar"),
+        with_package(package = "bar-plus", provides = "bar (= 1.0)"),
+    ], resolved_name = "bar-plus")
+
+    # Un-versioned does not match with multiple candidates
+    check_resolves([
+        with_package(package = "foo", depends = "bar"),
+        with_package(package = "bar-plus", provides = "bar"),
+        with_package(package = "bar-plus2", provides = "bar"),
+    ], resolved_name = None)
 
     return unittest.end(env)