Repository navigation
Prefer resolved executable symlinks in POSIX virtual environments - #9058
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to The change makes Unix virtual environments prefer resolved executable symlinks and adds tests for it. No merge-blocking problem was found in the supplied context. macOS framework behavior still needs its hosted CI run. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 32.35% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 68 functions across 38 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: preferring resolved executable symlinks in POSIX virtual environments.
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
-
-
-
-
-
🧪 Generate unit tests (beta)
-
-
-
-
-
-
- Create a new PR
-
-
-
-
-
-
-
-
-
-
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68ea9412c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let end = bytes.iter().rposition(|&b| b == b'/'); | ||
| let dirname = &bytes[..end.unwrap_or(bytes.len())]; | ||
| let mut joined = PathBuf::from(std::ffi::OsStr::from_bytes(dirname)); |
There was a problem hiding this comment.
When a venv executable is directly under / (for example, /python with /pyvenv.cfg) and the executable symlink has a relative target such as usr/bin/python, rposition returns 0 and this slice drops the only root separator. The kernel resolves that target as /usr/bin/python, but calculate_base_executable now publishes usr/bin/python as sys._base_executable, so its meaning depends on the startup working directory and consumers such as venv can select the wrong interpreter; retain the root separator when the final component starts immediately after / (and likewise for a // root).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This edge is deliberate CPython 3.14 parity. Both 3.14.7 and 3.14.8 truncate the final separator before joining a relative target: getpath.c. The join helper adds no separator for an empty dirname: fileutils.c. Consequently, /python with target usr/bin/python becomes usr/bin/python, while //python becomes /usr/bin/python. The current helper reproduces that behavior; retaining these roots would diverge from the reference contract. This particular edge was checked against the source, without creating or testing root-level filesystem paths. I am keeping the implementation unchanged.
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f29bb731e2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
|
|
||
| # A venv's symlink target takes precedence over home/<invoked name>. | ||
| if os.name == "posix" and sys.platform not in ("wasi", "emscripten"): |
There was a problem hiding this comment.
This block adds synthetic test data, control flow, and assertions to an existing test file, but the repository policy permits test-file changes only for adding or removing expected-failure decorators; the regression coverage therefore needs to be removed from stdlib_sys.py rather than added here.
AGENTS.md reference: AGENTS.md:L273-L279
Useful? React with 👍 / 👎.
f29bb73 to
1601d76
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
1601d76 to
7a0e83f
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
7a0e83f to
7032ba3
Compare
7032ba3 to
e87b00c
Compare
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [ ] test: cpython/Lib/test/test_mmap.py (TODO: 19) dependencies: dependent tests: (2 tests)
Legend:
|
Resolve executable symlinks for virtual-environment paths while retaining macOS framework launcher behavior. Include directory-canonicalization and launcher regressions, using core for UTF-8 checks. Assisted-by: OpenAI Codex:model identifier unavailable
e87b00c to
915a683
Compare
2 participants
Current revision
Targets
mainafter #8950, with one logical commit:915a6836eb71e5d1c345457c76c0d7b2be1f72f7(parenta10744303f6fb113a6082dde133958ad882d25a4). The unique patch is byte-identical after rebasing onto current Python 3.15 main, including the merged shared IO correction; original author attribution and assistance trailers are retained. No files underLib/are changed.Normal hooks, whitespace checks, and independent tree/history review passed. The temporary
[skip ci]marker is removed and normal CI is enabled for this final head. No new local build or runtime test is claimed. Validation recorded below belongs to earlier revisions unless explicitly stated otherwise.Summary
Extract the Python 3.14-compatible venv executable-symlink behavior from #8954 (
43065e79ddc46eafeca58cb39398ac007b5d25f7), without copying its broader custom metadata traversal.When
pyvenv.cfgsupplies a home directory, follow final-component executable symlinks before falling back tohome/<invoked name>. Preserve directory aliases, copied-executable fallback, non-venv invocation paths, Windows behavior, and existing launcher-override precedence.The resolver follows CPython 3.14.7's getpath helper, including its bounded traversal, rather than using filesystem canonicalization, which would incorrectly resolve directory aliases. The selection contract is in getpath.py.
Validation
-Sprefix behavior differs; validation was moved to the correct reference version without weakening assertions.-D warnings.All new assertions are Python-side. Children only print sys attributes, run with
-B -Sand timeouts, and execute the trusted interpreter or its copy inside owned temporary directories. No package installation, configuration-provided executable, dangling/cyclic launch, or root-path probe is part of the tests.Boundaries
The local character-count rule follows RustPython's existing UTF-8/surrogateescape filesystem contract; arbitrary non-UTF-8 CPython locale parity is not claimed. Root-level edge behavior was source-reviewed only. WASI/Emscripten are excluded from new runtime coverage; non-Linux native execution remains unverified.
No cfg parser, custom
base-executable/executablemetadata, PYTHONHOME handling, shared path converter, or canonical test assertion/marker is changed. Canonicaltest_getpath.pyis absent from this checkout; no canonical-suite pass is claimed. Hosted CI remains required.Summary by CodeRabbit
macOS framework reference correction
Follow-up
f29bb73corrects the new test fixture's expectations for the installed macOS framework launcher. The launcher recreates__PYVENV_LAUNCHER__, presets the framework base, and canonicalizes the invocation directory (launcher source, getpath precedence). The fixture now combines the configured framework feature with exact installed-launcher file identity; direct embedded-app binaries keep the ordinary path expectations. All cases and exact assertions remain; no implementation-name guard or production change is added.The corrected full snippet passes native RustPython and both CPython 3.14.7/3.14.8 references on Linux; the reference/native pytest harness also passes. Independent source review and normal hooks passed. Production files are identical to
68ea941, whose native binary was reused for this test-only validation. Actual macOS framework validation requires the new hosted run; it has not been reproduced locally. An arbitrary copied framework stub as the original parent interpreter remains outside this fixture's pre-existing scope.