install: make --sync compatible with virtualenvs.options.system-site-packages = true

Without this fix, we try to uninstall untracked system site packages.

Author: radoering

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

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)
 

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,

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

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:

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,

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