feat: add builtin uninstaller as alternative to calling pip - variant without rollback#10931
Draft
radoering wants to merge 1 commit into
Draft
feat: add builtin uninstaller as alternative to calling pip - variant without rollback#10931radoering wants to merge 1 commit into
radoering wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The uninstaller relies on the private
importlib.metadata.Distribution._pathattribute 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 betweenuninstall_distributionandUninstallPathSet._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>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
gmashrsa-design
approved these changes
May 30, 2026
2 tasks
fa82605 to
a538f54
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Check List
Closes: #10904
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-uninstallto use it. (We can switch it later and remove the setting even later, just as we did withinstaller.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.