From 06b1ca28bccafa5b8cc23128e5b0715b1d0523c7 Mon Sep 17 00:00:00 2001 From: Tiago Nobrega Date: Thu, 3 Aug 2023 18:47:33 -0300 Subject: [PATCH] sources: only add "compatible" architectures (#96) This commit both fixes the order of the parameters in the call to "dpkg --add-architecture" (the parser is picky) and updates the logic to only make said call if the target architecture (that is, the one coming from the package-repository definition) is "compatible" with the host architecture (that is, the architecture of the system running the code). This is necessary because incompatible mixes typically break the system. For example, adding "armhf" on an "amd64" system breaks subsequent calls to "apt update" because apt will try to fetch package listings for the "armhf" architecture on the default Ubuntu archives, which don't have them. Therefore, only the following pairs result in adding the target arch: - host amd64, target i386; - host arm64, target armhf. --- craft_archives/repo/apt_sources_manager.py | 26 ++++++-- tests/unit/repo/test_apt_sources_manager.py | 72 +++++++++++++++++---- 2 files changed, 80 insertions(+), 18 deletions(-) diff --git a/craft_archives/repo/apt_sources_manager.py b/craft_archives/repo/apt_sources_manager.py index 181256a..143eaf9 100644 --- a/craft_archives/repo/apt_sources_manager.py +++ b/craft_archives/repo/apt_sources_manager.py @@ -299,9 +299,25 @@ def install_package_repository_sources( def _add_architecture(architectures: List[str], root: Path) -> None: """Add package repository architecture.""" + current_arch = _get_current_architecture() + compatible_pairs = {"amd64": "i386", "arm64": "armhf"} for arch in architectures: - logger.info(f"Add repository architecture: {arch}") - subprocess.run( - ["dpkg", "--add-architecture", arch, "--root", str(root)], - check=True, - ) + # We can only add architectures if they are compatible with the running host, + # because of the way the default repositories are typically setup. + if compatible_pairs.get(current_arch) == arch: + logger.info(f"Add repository architecture: {arch}") + subprocess.run( + # Note: the order of parameters matters here, as "--add-architecture" + # must come last because of the way dpkg parses the command. + ["dpkg", "--root", str(root), "--add-architecture", arch], + check=True, + ) + + +def _get_current_architecture() -> str: + """Get the "main" architecture of the running system, as reported by dpkg.""" + return ( + subprocess.check_output(["dpkg", "--print-architecture"]) + .decode("utf-8") + .strip() + ) diff --git a/tests/unit/repo/test_apt_sources_manager.py b/tests/unit/repo/test_apt_sources_manager.py index f2431bb..9a4fb9e 100644 --- a/tests/unit/repo/test_apt_sources_manager.py +++ b/tests/unit/repo/test_apt_sources_manager.py @@ -28,6 +28,7 @@ from craft_archives.repo.apt_sources_manager import ( _DEFAULT_SOURCES_DIRECTORY, AptSourcesManager, + _add_architecture, ) from craft_archives.repo.package_repository import ( PackageRepositoryApt, @@ -187,6 +188,14 @@ def test_install( mocker, ): run_mock = mocker.patch("subprocess.run") + get_architecture_mock = mocker.patch( + "subprocess.check_output", return_value=b"fake" + ) + add_architecture_mock = mocker.spy( + apt_sources_manager, + "_add_architecture", + ) + mocker.patch("urllib.request.urlopen") apt_sources_mgr = create_apt_sources_mgr( @@ -216,23 +225,19 @@ def test_install( assert sources_path.read_bytes() == content if use_signed_by_root: - expected_root = str(tmp_path) + expected_root = tmp_path else: - expected_root = "/" + expected_root = Path("/") if isinstance(package_repo, PackageRepositoryApt) and package_repo.architectures: - assert run_mock.mock_calls == [ - call( - ["dpkg", "--add-architecture", "amd64", "--root", expected_root], - check=True, - ), - call( - ["dpkg", "--add-architecture", "arm64", "--root", expected_root], - check=True, - ), + assert add_architecture_mock.mock_calls == [ + call(package_repo.architectures, root=expected_root) ] - else: - assert run_mock.mock_calls == [] + assert get_architecture_mock.called + + # Regardless of host architecture, "dpkg --add-architecture" must _not_ be called, + # because the fantasy archs in the test repos are not compatible. + assert run_mock.mock_calls == [] run_mock.reset_mock() @@ -294,3 +299,44 @@ def test_preferences_path_for_root(): assert AptSourcesManager.sources_path_for_root(Path("/my/root")) == Path( "/my/root/etc/apt/sources.list.d" ) + + +@pytest.mark.parametrize( + ("host_arch, repo_arch"), + [ + (b"amd64\n", "i386"), + (b"arm64\n", "armhf"), + ], +) +def test_add_architecture_compatible(mocker, host_arch, repo_arch): + """Test calling _add_architecture() with compatible pairs of (host, repo).""" + check_output_mock = mocker.patch("subprocess.check_output", return_value=host_arch) + run_mock = mocker.patch("subprocess.run") + + _add_architecture([repo_arch], root=Path("/")) + + check_output_mock.assert_called_once_with(["dpkg", "--print-architecture"]) + assert run_mock.mock_calls == [ + call( + ["dpkg", "--root", "/", "--add-architecture", repo_arch], + check=True, + ), + ] + + +@pytest.mark.parametrize( + ("host_arch, repo_arch"), + [ + (b"amd64\n", "arm64"), + (b"arm64\n", "i386"), + ], +) +def test_add_architecture_incompatible(mocker, host_arch, repo_arch): + """Test calling _add_architecture() with incompatible pairs of (host, repo).""" + check_output_mock = mocker.patch("subprocess.check_output", return_value=host_arch) + run_mock = mocker.patch("subprocess.run") + + _add_architecture([repo_arch], root=Path("/")) + + check_output_mock.assert_called_once_with(["dpkg", "--print-architecture"]) + assert not run_mock.called