Skip to content

Fix installation of project plugins if Poetry's environment is not writable - #10998

Open
pratyushsinghal7 wants to merge 1 commit into
python-poetry:mainfrom
pratyushsinghal7:fix/requires-plugins-path-resolution
Open

Fix installation of project plugins if Poetry's environment is not writable#10998
pratyushsinghal7 wants to merge 1 commit into
python-poetry:mainfrom
pratyushsinghal7:fix/requires-plugins-path-resolution

Conversation

@pratyushsinghal7

Copy link
Copy Markdown

Pull Request Check List

Resolves: #10341

  • Added tests for changed code.
  • Documentation is not required; this fixes [tool.poetry.requires-plugins] to behave as already documented.

Summary

When project plugins are installed, ProjectPluginCache._install() redirects purelib and platlib to the project's plugin cache (.poetry/plugins). Env.scheme_dict then computed the prefix used to relocate read-only paths below userbase as the common path of scripts and the redirected purelib. When Poetry runs from a non-writable environment (e.g. a system-wide install used by an unprivileged user), those two paths only share something close to the filesystem root, and the subsequent plain string replacement (str.replace) mangled every path separator. As a result, plugins were silently installed into bogus directories instead of the plugin cache and never activated (as diagnosed by @dimbleby in the issue).

This change makes scheme_dict:

  • relocate only the scheme paths that are actually read-only, so the explicitly redirected (and writable) plugin cache is never touched,
  • compute the prefix as the common path of the read-only paths alone,
  • build candidates with os.path.relpath instead of a naive string replacement, and
  • return the original paths if the read-only paths only have the filesystem root in common, instead of producing bogus locations.

Behavior for the plain "all system scheme paths are read-only" case is unchanged: all paths are still relocated below userbase exactly as before.

Testing

  • New regression tests: read-only-env variant of test_project_plugins_are_installed_in_project_folder in tests/plugins/test_plugin_manager.py, plus two scheme_dict tests in tests/utils/env/test_env.py mirroring the issue. All three fail without the fix.
  • .venv/bin/pytest tests/plugins tests/installation tests/utils/env/test_env.py -q — 680 passed, 1 skipped
  • PATH="/tmp/poetry-test-bin:$PATH" .venv/bin/pytest -q — 3164 passed, 14 failed, 27 skipped; the same 14 tests fail identically on current main on this machine (local Python discovery issues), so no regressions from this change
  • .venv/bin/mypy src/poetry/utils/env/base_env.py tests/plugins/test_plugin_manager.py tests/utils/env/test_env.py — success
  • .venv/bin/ruff check / ruff format --check on the touched files — passed

…itable

When installing project plugins declared via tool.poetry.requires-plugins,
ProjectPluginCache._install() redirects "purelib" and "platlib" to the
project's plugin cache. Env.scheme_dict then computed the prefix to relocate
below "userbase" as the common path of "scripts" and the (redirected)
"purelib", which degenerates to something close to the filesystem root.
The subsequent naive string replacement mangled every path, so plugins were
silently installed into bogus directories instead of the plugin cache and
never activated.

Relocate only the scheme paths that are actually read-only, compute the
prefix from those paths alone, use proper path arithmetic instead of string
replacement, and bail out if the read-only paths only have the filesystem
root in common.

@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 base_path == os.path.dirname(base_path) check used to detect a root-only common path may behave unexpectedly on Windows (e.g. different drives, UNC paths) and other exotic path schemes; consider using Path(base_path).anchor or a more explicit root/common-prefix check to avoid subtle edge cases.
  • In scheme_dict, the read_only_keys/scheme_names logic has grown complex; you may want to refactor the read-only detection and prefix-relocation into a small helper function to make the behavior and invariants easier to reason about.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The `base_path == os.path.dirname(base_path)` check used to detect a root-only common path may behave unexpectedly on Windows (e.g. different drives, UNC paths) and other exotic path schemes; consider using `Path(base_path).anchor` or a more explicit root/common-prefix check to avoid subtle edge cases.
- In `scheme_dict`, the `read_only_keys`/`scheme_names` logic has grown complex; you may want to refactor the read-only detection and prefix-relocation into a small helper function to make the behavior and invariants easier to reason about.

## Individual Comments

### Comment 1
<location path="src/poetry/utils/env/base_env.py" line_range="301-310" />
<code_context>
+            overrides: dict[str, str] = {}
</code_context>
<issue_to_address>
**issue (bug_risk):** Relocation candidates are computed but never applied to `paths`, making the override logic ineffective.

In the previous version, `overrides` was populated and then used to adjust the scheme paths. In this version, `overrides` remains empty and `paths` is never updated with the relocated candidates, so `scheme_dict` still returns the original read-only paths even when writable candidates exist. Please either populate `overrides` and merge it into `paths`, or directly update `paths[key]` for each `read_only_key` after confirming writability.
</issue_to_address>

### Comment 2
<location path="tests/plugins/test_plugin_manager.py" line_range="649" />
<code_context>
         assert not any(p.name.startswith("demo") for p in orig_platlib.iterdir())
+
+
+def test_project_plugins_are_installed_in_project_folder_if_env_read_only(
+    poetry_with_plugins: Poetry,
+    io: BufferedIO,
+    system_env: Env,
+    fixture_dir: FixtureDirGetter,
+    tmp_path: Path,
+    mocker: MockerFixture,
+) -> None:
+    """https://github.com/python-poetry/poetry/issues/10341
+
+    If the scheme paths of Poetry's environment are not writable
+    (e.g. Poetry was installed system-wide and is run as an unprivileged user),
+    project plugins must still be installed into the project's plugin cache.
+    """
+    orig_purelib = system_env.purelib
+    orig_platlib = system_env.platlib
+
+    read_only_paths = {system_env.paths[key] for key in SCHEME_NAMES}
+    original_is_dir_writable = is_dir_writable
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider excluding `userbase` from the mocked read-only paths to better mirror real-world behavior

Here `read_only_paths` mirrors all `SCHEME_NAMES`, which usually includes `userbase`. That causes the mock `is_dir_writable` to treat the current `userbase` as read-only, while the intended scenario is “system schemes read-only, user base writable”. The test still passes because relocation candidates differ from the original `userbase`, but the setup is misleading. To better match real behavior and clarify the intent, build `read_only_paths` from non-`userbase` schemes only, e.g. `{system_env.paths[key] for key in SCHEME_NAMES if key != "userbase"}`.

```suggestion
    read_only_paths = {
        system_env.paths[key]
        for key in SCHEME_NAMES
        if key != "userbase"
    }
```
</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/utils/env/base_env.py
Comment thread tests/plugins/test_plugin_manager.py
@pratyushsinghal7

Copy link
Copy Markdown
Author

Regarding the Windows concern about base_path == os.path.dirname(base_path): ntpath.dirname is idempotent exactly at drive and UNC roots, which is what this check relies on. ntpath.commonpath(['C:\\usr\\lib', 'C:\\opt\\bin']) returns 'C:\\' and ntpath.dirname('C:\\') is 'C:\\', so a drive root is correctly detected; likewise commonpath of two paths on the same UNC share returns '\\\\server\\share\\' whose dirname is itself. Paths on different drives (or different UNC shares) never reach this check because os.path.commonpath raises ValueError for them, which is handled just above. Non-root common prefixes like 'C:\\usr' or '\\\\server\\share\\x' are not misdetected (dirname strips a component), so the current check behaves the same as an anchor-based one on all these inputs. The root-only regression test also builds its paths from system_env.path.anchor, so it exercises the drive-root case when run on Windows.

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.

Plugin support silently fails using tool.poetry.requires-plugins

1 participant