repo: fix for multi-arch virtual-packages

wine-devel-i386 does not have the explicit ":i386" tag, but
the virtual package is provided by "wine-devel-i386:i386".

Account for this possibility by resolving the package and
checking the architecture first, if no arch is specified by
the user.

- Revert the use of or_dependendencies back to target_versions
now that target_arch should correct the previously faulty
behavior (where target_versions ended up switching to the
incorrect arch after following an :any package).

- Fix apt.Package.architectures to architectures() method call.

Signed-off-by: Chris Patterson <chris.patterson@canonical.com>

snapcraft/internal/repo/_deb.py

@@ -405,43 +405,42 @@ def _package_name_parts(
 
     @classmethod
     def _get_package_for_arch(
-        cls, *, package_name: str, target_arch: str
+        cls, *, package_name: str, target_arch: Optional[str]
     ) -> apt.Package:
         name, arch, version = cls._package_name_parts(package_name)
+        logger.debug(f"Getting package {package_name!r} for {target_arch!r}.")
 
         cache = cls._get_apt_cache()
 
-        # Check for explicit <package-name>:<arch> first.
-        target_package_name = f"{name}:{target_arch}"
-        if target_package_name in cache:
-            return cache[target_package_name]
-
-        # Now check if package is :all in case it is not arch-specific.
-        target_package_name = f"{name}:all"
-        if target_package_name in cache:
-            return cache[target_package_name]
-
-        # Failing these, try the name in best effort scenario.
-        if name in cache:
+        if target_arch is not None and f"{name}:{target_arch}" in cache:
+            # First check for explicit <package-name>:<arch>.
+            package = cache[f"{name}:{target_arch}"]
+        elif f"{name}:all" in cache:
+            # Then check if package is :all in case it is not arch-specific.
+            package = cache[f"{name}:all"]
+        elif f"{name}:{arch}" in cache:
+            # Then rely on the arch that was specified in the name.
+            package = cache[f"{name}:{arch}"]
+        elif name in cache:
+            # Rely on the default selected by apt.Cache(), we'll check/warn
+            # on the architecture below if we think it might be incorrect.
             package = cache[name]
-            if package.architecture != target_arch:
-                logger.warning(
-                    f"Possible incorrect architecture for package {name!r}. "
-                    f"Found architecture {package.architecture!r}, "
-                    f"intended architecture is {target_arch!r}."
-                )
-            return package
+        else:
+            raise errors.PackageNotFoundError(package_name)
 
-        raise errors.PackageNotFoundError(package_name)
+        if target_arch is not None and package.architecture() != target_arch:
+            logger.warning(
+                f"Possible incorrect architecture for package {name!r}. "
+                f"Found architecture {package.architecture()!r}, "
+                f"intended architecture is {target_arch!r}."
+            )
+
+        return package
 
     @classmethod
     def _get_resolved_package(
         cls, package_name: str, *, target_arch: Optional[str] = None
     ) -> apt.package.Package:
-        # Default to host architecture if unspecified.
-        if target_arch is None:
-            target_arch = _get_host_arch()
-
         name, arch, version = cls._package_name_parts(package_name)
 
         cache = cls._get_apt_cache()
@@ -473,7 +472,21 @@ def _mark_package_dependencies(
         unfiltered_packages: List[str],
         target_arch: Optional[str],
     ) -> None:
+        # If we come across a package that explicitly requests a target arch
+        # via <package-name>:<arch>, update target_arch as we are crossing
+        # an architecture boundary.
+        _, arch, _ = cls._package_name_parts(package_name)
+        if arch:
+            target_arch = arch
+
         package = cls._get_resolved_package(package_name, target_arch=target_arch)
+
+        # Virtual packages may resolve to a foreign architecture. For
+        # example: 'wine-devel-i386' resolved to 'wine-devel-i386:i386'
+        # Use the resolved package's architecture as the target arch.
+        if target_arch is None:
+            target_arch = package.architecture()
+
         if package.name in marked_packages:
             # already marked, ignore.
             return
@@ -494,7 +507,7 @@ def _mark_package_dependencies(
         marked_packages[package.name] = package.candidate
 
         for pkg_dep in package.candidate.dependencies:
-            dep_name = pkg_dep.or_dependencies[0].name
+            dep_name = pkg_dep.target_versions[0].package.name
             cls._mark_package_dependencies(
                 package_name=dep_name,
                 marked_packages=marked_packages,
@@ -514,6 +527,11 @@ def install_stage_packages(
 
         logger.debug(f"Requested stage-packages: {sorted(package_names)!r}")
 
+        # Some projects will modify apt configuration directly.  To ensure
+        # our cache is up-to-date, refresh the cache whenever stage-packages
+        # are requested.
+        cls._refresh_cache()
+
         # First scan all packages and set desired version, if specified.
         # We do this all at once in case it gets added as a dependency
         # along the way.
@@ -524,8 +542,9 @@ def install_stage_packages(
                 cls._set_package_version(package, version)
 
         for name in package_names:
+            logger.debug(f"Marking package dependencies for {name!r}.")
             name, arch, _ = cls._package_name_parts(name)
-            package = cls._get_resolved_package(name, target_arch=arch)
+
             cls._mark_package_dependencies(
                 package_name=name,
                 marked_packages=marked_packages,
@@ -546,7 +565,13 @@ def install_stage_packages(
             essential = sorted(skipped_essential)
             logger.debug(f"Skipping priority essential packages: {essential!r}")
 
-        for pkg_name, pkg_version in marked_packages.items():
+        # Install the package in reverse sorted manner.  This way, packages
+        # that are cross-arch, e.g. "foo:i386", will get installed before the
+        # native "foo".  In this manner, host-arch will will be the last
+        # written file in case there are overlapping paths (e.g. i386 bins).
+        # TODO: detect and warn when we are overwriting a staged file with
+        # another one.
+        for pkg_name, pkg_version in sorted(marked_packages.items(), reverse=True):
             try:
                 dl_path = pkg_version.fetch_binary(cls._cache_dir)
             except apt.package.FetchError as e:

tests/unit/repo/test_deb.py

@@ -56,12 +56,17 @@ def __init__(
         self,
         *,
         name: str,
+        architecture: str,
         candidate: Optional[MagicMock] = None,
         installed: Optional[MagicMock] = None,
     ) -> None:
         self.name = name
         self.candidate = candidate
         self.installed = installed
+        self._architecture = architecture
+
+    def architecture(self):
+        return self._architecture
 
 
 class FakeAptCache:
@@ -89,11 +94,12 @@ def add_fake_package(
         if architecture is None:
             architecture = repo._deb._get_host_arch()
 
-        package = MagicMock(wraps=FakeAptPackage(name=name))
+        package = MagicMock(wraps=FakeAptPackage(name=name, architecture=architecture))
         type(package).name = PropertyMock(return_value=name)
-        type(package).architecture = PropertyMock(return_value=architecture)
+        package.architecture.return_value = architecture
 
         self.packages[name] = package
+        self.packages[f"{name}:{architecture}"] = package
 
         package_dependencies = [
             self.dependencies.get(name) for name in dependency_names
@@ -127,8 +133,8 @@ def add_fake_package(
         base_dependency = MagicMock(spec=["name"])
         type(base_dependency).name = PropertyMock(return_value=name)
 
-        dependency = MagicMock(spec=["or_dependencies"])
-        type(dependency).or_dependencies = PropertyMock(return_value=[base_dependency])
+        dependency = MagicMock(spec=["target_versions"])
+        type(dependency).target_versions = PropertyMock(return_value=[apt_version])
 
         self.dependencies[name] = dependency
 
@@ -260,6 +266,50 @@ def test_install_stage_package_with_deps(self):
             ),
         )
 
+    def test_install_virtual_package_with_implicit_arch(self):
+        self.fake_apt_cache.add_fake_package(
+            name="package-dep",
+            priority="normal",
+            version="0.1",
+            installed=True,
+            dependency_names=[],
+            architecture="i737",
+        )
+
+        self.fake_apt_cache.add_fake_package(
+            name="package-i737",
+            priority="normal",
+            version="0.1",
+            installed=True,
+            dependency_names=["package-dep"],
+            architecture="i737",
+        )
+
+        self.fake_apt_cache.add_fake_package(
+            name="virtual-package-i737",
+            priority="normal",
+            version="0.2",
+            installed=False,
+            dependency_names=[],
+        )
+
+        installed_packages = repo.Ubuntu.install_stage_packages(
+            package_names=["virtual-package-i737"], install_dir=self.path
+        )
+
+        self.assertThat(
+            self.fake_apt_cache["package-i737:i737"].candidate.mock_calls,
+            Contains(call.fetch_binary(self.path)),
+        )
+
+        self.assertThat(
+            self.fake_apt_cache["virtual-package-i737"].candidate.mock_calls, Equals([])
+        )
+
+        self.assertThat(
+            installed_packages, Equals(["package-i737=0.1", "package-dep=0.1"])
+        )
+
     def test_get_package_fetch_error(self):
         self.fake_apt_cache.packages[
             "fake-package"