Skip to content

feat: add builtin uninstaller as alternative to calling pip - variant without rollback#10931

Draft
radoering wants to merge 1 commit into
python-poetry:mainfrom
radoering:builtin-uninstaller3
Draft

feat: add builtin uninstaller as alternative to calling pip - variant without rollback#10931
radoering wants to merge 1 commit into
python-poetry:mainfrom
radoering:builtin-uninstaller3

Conversation

@radoering

Copy link
Copy Markdown
Member

Pull Request Check List

Closes: #10904

  • Added tests for changed code.
  • Updated documentation for changed code.

Introduce a builtin uninstaller (adapted from pip's code, dropping legacy handling and the option to rollback) so that we do not have to run a pip subprocess for each uninstall. This makes uninstalls (and updates) faster. The new builtin uninstaller is not used by default. You have to set installer.builtin-uninstall to use it. (We can switch it later and remove the setting even later, just as we did with installer.modern-installation.)

Alternative to #10904 without the option to rollback uninstalls as proposed in #10904 (comment).

I started from #10904 removing the rollback mechansim, which resulted in radoering@0d1d42e. (This is another alternative on another branch.) This still keeps the collect-and-compress logic, to remove directories in cases where all children should be removed.

Then, I wondered if it would not be simpler and faster if we just remove all files and check afterwards if there are empty directories to remove. After having it implemented (in this PR), I would say it is similarly complex, but according to my tests it is faster - it only requires 0.6 to 0.9 of the time. That is because we avoid the os.walk.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 2 issues, and left some high level feedback:

  • The uninstaller relies on the private importlib.metadata.Distribution._path attribute in multiple places; consider centralising this behind a small helper or wrapper (with clear comments about the assumption) so that if the attribute changes in future Python versions there is a single place to update or to plug in an alternative source for the dist-info location.
  • The env-prefix and os.sep-guard logic for checking whether a path is inside an environment is duplicated between uninstall_distribution and UninstallPathSet._permitted; factoring this into a shared helper would reduce the risk of the two diverging over time and make future changes (e.g. handling edge cases with symlinks) easier.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The uninstaller relies on the private `importlib.metadata.Distribution._path` attribute in multiple places; consider centralising this behind a small helper or wrapper (with clear comments about the assumption) so that if the attribute changes in future Python versions there is a single place to update or to plug in an alternative source for the dist-info location.
- The env-prefix and `os.sep`-guard logic for checking whether a path is inside an environment is duplicated between `uninstall_distribution` and `UninstallPathSet._permitted`; factoring this into a shared helper would reduce the risk of the two diverging over time and make future changes (e.g. handling edge cases with symlinks) easier.

## Individual Comments

### Comment 1
<location path="src/poetry/installation/uninstaller.py" line_range="101" />
<code_context>
+        self._listdir_cached = functools.lru_cache(maxsize=None)(_safe_listdir)
+        # Normalized path of the .dist-info directory, so remove() can delete it
+        # last (see remove() for why).
+        dist_info_path: Path = dist._path  # type: ignore[attr-defined]
+        self._dist_info = self._normalize_path_cached(str(dist_info_path))
+        # Directories that must never be removed even when they end up empty.
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Relying on `dist._path` ties behavior to a private importlib.metadata attribute.

This dependency on private internals may break across Python versions or alternative metadata backends. Prefer deriving `dist_info_path` from public APIs (e.g., `locate_file`, `files()`, or the distribution’s `__file__`/`read_text` location) so the uninstaller is resilient to upstream changes.
</issue_to_address>

### Comment 2
<location path="tests/installation/test_executor.py" line_range="2104-2113" />
<code_context>
+    return Update(initial, target)
+
+
+def test_update_uninstalls_then_installs_when_uninstall_succeeds(
+    config: Config,
+    pool: RepositoryPool,
+    io: BufferedIO,
+    env: MockEnv,
+    mocker: MockerFixture,
+    tmp_path: Path,
+) -> None:
+    config.merge({"installer": {"builtin-uninstall": True}})
+    uninstall_mock = mocker.patch(
+        "poetry.installation.executor.uninstall_distribution",
+        return_value=mocker.Mock(),
+    )
+    wheel_install = mocker.patch.object(WheelInstaller, "install")
+    fake_archive = tmp_path / "demo-2.0.0-py3-none-any.whl"
+    fake_archive.write_bytes(b"")
+    mocker.patch.object(Executor, "_prepare_archive", return_value=fake_archive)
+
+    executor = Executor(env, pool, config, io)
+    assert executor._install(_make_update_op(tmp_path)) == 0
+
+    # The old version is uninstalled and the new version is then installed.
+    uninstall_mock.assert_called_once_with(env, "demo")
+    wheel_install.assert_called_once()
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for the update path where builtin uninstall finds nothing to remove (returns None) but the wheel install still proceeds.

Currently we only test update behavior when builtin uninstall succeeds or raises. Please also cover the case where `uninstall_distribution` returns `None` (package not installed / refused) under `builtin-uninstall=True`, and verify that `_install` still calls `WheelInstaller.install` (because `_remove` returns 0). That would complete coverage of all three builtin-uninstall outcomes for updates.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread src/poetry/installation/uninstaller.py
Comment thread tests/installation/test_executor.py
@radoering radoering force-pushed the builtin-uninstaller3 branch from fa82605 to a538f54 Compare May 31, 2026 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants