From 4994505eb8a6c089ca0cd68a9f830f26bb13c776 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Randy=20D=C3=B6ring?= <30527984+radoering@users.noreply.github.com> Date: Wed, 20 Nov 2024 16:59:11 +0100 Subject: [PATCH] install: make `--sync` compatible with `virtualenvs.options.system-site-packages = true` Without this fix, we try to uninstall untracked system site packages. --- src/poetry/installation/installer.py | 5 +- src/poetry/plugins/plugin_manager.py | 3 +- src/poetry/puzzle/transaction.py | 15 +++-- .../repositories/installed_repository.py | 30 +++++++-- tests/conftest.py | 8 +-- tests/puzzle/test_transaction.py | 65 +++++++++++++++++-- .../repositories/test_installed_repository.py | 51 ++++++++++++++- 7 files changed, 157 insertions(+), 20 deletions(-) diff --git a/src/poetry/installation/installer.py b/src/poetry/installation/installer.py index efa16ff5385..eff9aeffdd5 100644 --- a/src/poetry/installation/installer.py +++ b/src/poetry/installation/installer.py @@ -39,7 +39,7 @@ def __init__( locker: Locker, pool: RepositoryPool, config: Config, - installed: Repository | None = None, + installed: InstalledRepository | None = None, executor: Executor | None = None, disable_cache: bool = False, ) -> None: @@ -332,6 +332,9 @@ def _do_install(self) -> int: synchronize=self._requires_synchronization, skip_directory=self._skip_directory, extras=set(self._extras), + system_site_packages={ + p.name for p in self._installed_repository.system_site_packages + }, ) # Validate the dependencies diff --git a/src/poetry/plugins/plugin_manager.py b/src/poetry/plugins/plugin_manager.py index 042985da1bb..38b777dc1f0 100644 --- a/src/poetry/plugins/plugin_manager.py +++ b/src/poetry/plugins/plugin_manager.py @@ -19,7 +19,6 @@ from poetry.packages import Locker from poetry.plugins.application_plugin import ApplicationPlugin from poetry.plugins.plugin import Plugin -from poetry.repositories import Repository from poetry.repositories.installed_repository import InstalledRepository from poetry.toml import TOMLFile from poetry.utils._compat import metadata @@ -288,7 +287,7 @@ def _install( self._poetry.config, # Build installed repository from locked packages so that plugins # that may be overwritten are not included. - Repository("poetry-repo", locked_packages), + InstalledRepository(locked_packages), ) installer.update(True) diff --git a/src/poetry/puzzle/transaction.py b/src/poetry/puzzle/transaction.py index 3f18e7f5095..05a9432818f 100644 --- a/src/poetry/puzzle/transaction.py +++ b/src/poetry/puzzle/transaction.py @@ -42,16 +42,20 @@ def get_solved_packages(self) -> dict[Package, TransitivePackageInfo]: def calculate_operations( self, + *, with_uninstalls: bool = True, synchronize: bool = False, - *, skip_directory: bool = False, extras: set[NormalizedName] | None = None, + system_site_packages: set[NormalizedName] | None = None, ) -> list[Operation]: from poetry.installation.operations import Install from poetry.installation.operations import Uninstall from poetry.installation.operations import Update + if not system_site_packages: + system_site_packages = set() + operations: list[Operation] = [] extra_packages: set[NormalizedName] = set() @@ -105,7 +109,8 @@ def calculate_operations( # Extras that were not requested are always uninstalled. if is_unsolicited_extra: uninstalls.add(installed_package.name) - operations.append(Uninstall(installed_package)) + if installed_package.name not in system_site_packages: + operations.append(Uninstall(installed_package)) # We have to perform an update if the version or another # attribute of the package has changed (source type, url, ref, ...). @@ -156,7 +161,8 @@ def calculate_operations( for installed_package in self._installed_packages: if installed_package.name == current_package.name: uninstalls.add(installed_package.name) - operations.append(Uninstall(installed_package)) + if installed_package.name not in system_site_packages: + operations.append(Uninstall(installed_package)) if synchronize: # We preserve pip when not managed by poetry, this is done to avoid @@ -178,7 +184,8 @@ def calculate_operations( if installed_package.name not in relevant_result_packages: uninstalls.add(installed_package.name) - operations.append(Uninstall(installed_package)) + if installed_package.name not in system_site_packages: + operations.append(Uninstall(installed_package)) return sorted( operations, diff --git a/src/poetry/repositories/installed_repository.py b/src/poetry/repositories/installed_repository.py index 891e8f6b79e..91c9a23f2dd 100644 --- a/src/poetry/repositories/installed_repository.py +++ b/src/poetry/repositories/installed_repository.py @@ -16,18 +16,26 @@ from poetry.repositories.repository import Repository from poetry.utils._compat import getencoding from poetry.utils._compat import metadata +from poetry.utils.env import VirtualEnv if TYPE_CHECKING: - from poetry.utils.env import Env + from collections.abc import Sequence + from poetry.utils.env import Env logger = logging.getLogger(__name__) class InstalledRepository(Repository): - def __init__(self) -> None: - super().__init__("poetry-installed") + def __init__(self, packages: Sequence[Package] | None = None) -> None: + super().__init__("poetry-installed", packages) + self.system_site_packages: list[Package] = [] + + def add_package(self, package: Package, *, is_system_site: bool = False) -> None: + super().add_package(package) + if is_system_site: + self.system_site_packages.append(package) @classmethod def get_package_paths(cls, env: Env, name: str) -> set[Path]: @@ -242,6 +250,12 @@ def load(cls, env: Env, with_dependencies: bool = False) -> InstalledRepository: seen = set() skipped = set() + base_env = ( + env.parent_env + if isinstance(env, VirtualEnv) and env.includes_system_site_packages + else None + ) + for entry in env.sys_path: if not entry.strip(): logger.debug( @@ -283,6 +297,14 @@ def load(cls, env: Env, with_dependencies: bool = False) -> InstalledRepository: package.add_dependency(dep) seen.add(package.name) - repo.add_package(package) + repo.add_package( + package, + is_system_site=bool( + base_env + and base_env.is_path_relative_to_lib( + Path(str(distribution._path)) # type: ignore[attr-defined] + ) + ), + ) return repo diff --git a/tests/conftest.py b/tests/conftest.py index ba9423f336f..39d71e172fc 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -29,8 +29,8 @@ from poetry.factory import Factory from poetry.layouts import layout from poetry.packages.direct_origin import _get_package_from_git -from poetry.repositories import Repository from poetry.repositories import RepositoryPool +from poetry.repositories.installed_repository import InstalledRepository from poetry.utils.cache import ArtifactCache from poetry.utils.env import EnvManager from poetry.utils.env import SystemEnv @@ -379,8 +379,8 @@ def tmp_venv(tmp_path: Path) -> Iterator[VirtualEnv]: @pytest.fixture -def installed() -> Repository: - return Repository("installed") +def installed() -> InstalledRepository: + return InstalledRepository() @pytest.fixture(scope="session") @@ -412,7 +412,7 @@ def project_factory( tmp_path: Path, config: Config, repo: TestRepository, - installed: Repository, + installed: InstalledRepository, default_python: str, load_required_fixtures: None, ) -> ProjectFactory: diff --git a/tests/puzzle/test_transaction.py b/tests/puzzle/test_transaction.py index a8874ebe4eb..a68c981d159 100644 --- a/tests/puzzle/test_transaction.py +++ b/tests/puzzle/test_transaction.py @@ -135,6 +135,59 @@ def test_it_should_remove_installed_packages_if_required() -> None: ) +def test_it_should_not_remove_system_site_packages() -> None: + """ + Different types of uninstalls: + - c: tracked but not required + - e: not tracked + - f: root extra that is not requested + """ + extra_name = canonicalize_name("foo") + package = ProjectPackage("root", "1.0") + dep_f = Dependency("f", "1", optional=True) + dep_f._in_extras = [extra_name] + package.add_dependency(dep_f) + package.extras = {extra_name: [dep_f]} + opt_f = Package("f", "6.0.0") + opt_f.optional = True + transaction = Transaction( + [Package("a", "1.0.0"), Package("b", "2.0.0"), Package("c", "3.0.0")], + { + Package("a", "1.0.0"): get_transitive_info(1), + Package("b", "2.1.0"): get_transitive_info(2), + Package("d", "4.0.0"): get_transitive_info(0), + opt_f: get_transitive_info(0), + }, + installed_packages=[ + Package("a", "1.0.0"), + Package("b", "2.0.0"), + Package("c", "3.0.0"), + Package("e", "5.0.0"), + Package("f", "6.0.0"), + ], + root_package=package, + ) + + check_operations( + transaction.calculate_operations( + synchronize=True, + extras=set(), + system_site_packages={ + canonicalize_name(name) for name in ("a", "b", "c", "e", "f") + }, + ), + [ + { + "job": "update", + "from": Package("b", "2.0.0"), + "to": Package("b", "2.1.0"), + }, + {"job": "install", "package": Package("a", "1.0.0"), "skipped": True}, + {"job": "install", "package": Package("d", "4.0.0")}, + ], + ) + + def test_it_should_not_remove_installed_packages_that_are_in_result() -> None: transaction = Transaction( [], @@ -236,7 +289,9 @@ def test_calculate_operations_with_groups( for name in sorted({"a", "b", "c"}.difference(expected), reverse=True): expected_ops.insert(0, {"job": "remove", "package": Package(name, "1")}) - check_operations(transaction.calculate_operations(sync), expected_ops) + check_operations( + transaction.calculate_operations(with_uninstalls=sync), expected_ops + ) @pytest.mark.parametrize( @@ -273,7 +328,9 @@ def test_calculate_operations_with_markers( for name in sorted({"a", "b"}.difference(expected), reverse=True): expected_ops.insert(0, {"job": "remove", "package": Package(name, "1")}) - check_operations(transaction.calculate_operations(sync), expected_ops) + check_operations( + transaction.calculate_operations(with_uninstalls=sync), expected_ops + ) @pytest.mark.parametrize( @@ -366,8 +423,8 @@ def test_calculate_operations_extras( check_operations( transaction.calculate_operations( - with_uninstalls, - sync, + with_uninstalls=with_uninstalls, + synchronize=sync, extras={extra_name} if extras else set(), ), ops, diff --git a/tests/repositories/test_installed_repository.py b/tests/repositories/test_installed_repository.py index 622b8ea0cfa..f3bef620f35 100644 --- a/tests/repositories/test_installed_repository.py +++ b/tests/repositories/test_installed_repository.py @@ -1,6 +1,7 @@ from __future__ import annotations import os +import platform import shutil import zipfile @@ -415,6 +416,48 @@ def test_load_pep_610_compliant_editable_directory_packages( assert package.develop +@pytest.mark.parametrize("with_system_site_packages", [False, True]) +def test_system_site_packages( + tmp_path: Path, + mocker: MockerFixture, + poetry: Poetry, + site_purelib: Path, + with_system_site_packages: bool, +) -> None: + venv_path = tmp_path / "venv" + site_path = tmp_path / "site" + cleo_dist_info = "cleo-0.7.6.dist-info" + shutil.copytree(site_purelib / cleo_dist_info, site_path / cleo_dist_info) + + EnvManager(poetry).build_venv( + path=venv_path, flags={"system-site-packages": with_system_site_packages} + ) + env = VirtualEnv(venv_path) + standard_dist_info = "standard-1.2.3.dist-info" + shutil.copytree(site_purelib / standard_dist_info, env.purelib / standard_dist_info) + orig_sys_path = env.sys_path + if with_system_site_packages: + mocker.patch( + "poetry.utils.env.virtual_env.VirtualEnv.sys_path", + orig_sys_path[:-1] + [str(site_path)], + ) + mocker.patch( + "poetry.utils.env.generic_env.GenericEnv.get_paths", + return_value={"purelib": str(site_path)}, + ) + + installed_repository = InstalledRepository.load(env) + + expected_system_site_packages = {"cleo"} if with_system_site_packages else set() + expected_packages = {"standard"} | expected_system_site_packages + if platform.system() == "FreeBSD": + expected_packages.add("sqlite3") + assert {p.name for p in installed_repository.packages} == expected_packages + assert { + p.name for p in installed_repository.system_site_packages + } == expected_system_site_packages + + def test_system_site_packages_source_type( tmp_path: Path, mocker: MockerFixture, poetry: Poetry, site_purelib: Path ) -> None: @@ -427,12 +470,17 @@ def test_system_site_packages_source_type( for dist_info in {"cleo-0.7.6.dist-info", "directory_pep_610-1.2.3.dist-info"}: shutil.copytree(site_purelib / dist_info, site_path / dist_info) mocker.patch("poetry.utils.env.virtual_env.VirtualEnv.sys_path", [str(site_path)]) - mocker.patch("site.getsitepackages", return_value=[str(site_path)]) + mocker.patch( + "poetry.utils.env.generic_env.GenericEnv.get_paths", + return_value={"purelib": str(site_path)}, + ) EnvManager(poetry).build_venv(path=venv_path, flags={"system-site-packages": True}) env = VirtualEnv(venv_path) + installed_repository = InstalledRepository.load(env) + assert installed_repository.packages == installed_repository.system_site_packages source_types = { package.name: package.source_type for package in installed_repository.packages } @@ -458,6 +506,7 @@ def test_pipx_shared_lib_site_packages( installed_repository = InstalledRepository.load(env) assert len(installed_repository.packages) == 1 + assert installed_repository.system_site_packages == [] cleo_package = installed_repository.packages[0] cleo_package.to_dependency() # There must not be a warning