FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Prefer resolved executable symlinks in POSIX virtual environments by youknowdot · Pull Request #9058 · RustPython/RustPython · GitHub

Repository navigation

Prefer resolved executable symlinks in POSIX virtual environments - #9058

Open
youknowdot wants to merge 3 commits into
RustPython:mainfrom
youknowdot:fix-venv-executable-symlinks
Open

youknowdot wants to merge 3 commits into
RustPython:mainfrom
youknowdot:fix-venv-executable-symlinks

Conversation

youknowdot commented Oct 10, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Summary

Extract the Python 3.14-compatible venv executable-symlink behavior from #8954 (43065e79ddc46eafeca58cb39398ac007b5d25f7), without copying its broader custom metadata traversal.

When pyvenv.cfg supplies a home directory, follow final-component executable symlinks before falling back to home/<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

  • Independent source review caught and corrected directory-alias handling and the macOS-framework limitation in the initial test design before publication.
  • Exact CPython 3.14.7 and the native final head pass the complete existing sys snippet, including nine bounded scenarios: absolute/relative/chained links, both config locations, non-venv preservation, copied fallback, directory aliases, double-leading slash, and Linux launcher override.
  • Both dedicated snippet pytest entries pass under Python 3.14.7. The original generic local runner used Python 3.12, whose -S prefix behavior differs; validation was moved to the correct reference version without weakening assertions.
  • Normal hooks, VM Clippy, and separately configured CAPI Clippy pass with -D warnings.

All new assertions are Python-side. Children only print sys attributes, run with -B -S and 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/executable metadata, PYTHONHOME handling, shared path converter, or canonical test assertion/marker is changed. Canonical test_getpath.py is absent from this checkout; no canonical-suite pass is claimed. Hosted CI remains required.

Summary by CodeRabbit

  • Bug Fixes
    • Virtual environments now correctly identify the executable behind absolute, relative, and chained symlinks, improving executable path reporting when launching through a symlink.
    • When a virtual environment specifies a home directory, a resolved executable path is used when available; otherwise, the existing fallback is retained.
    • Executable paths remain unchanged for non-virtual-environment launches and copied executables.

macOS framework reference correction

Follow-up f29bb73 corrects 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.

Assisted-by: OpenAI Codex:model identifier unavailable
Assisted-by: OpenAI Codex:model identifier unavailable

coderabbitai Bot commented Oct 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: RustPython/RustPython/.coderabbit.yml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: df2c9cda-a68e-4d2a-b8fc-626a74df034e


📥 Commits

Reviewing files that changed from the base of the PR and between 68ea941 and f29bb73.



📒 Files selected for processing (1)
  • extra_tests/snippets/stdlib_sys.py


Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.




📝 Walkthrough
📝 Walkthrough

Walkthrough

Unix executable-path calculation now follows final-component symlinks and can use a resolved target as a venv base executable. POSIX tests cover symlinks, copied executables, directory aliases, and launcher cases.

Changes

Unix executable paths

Layer / File(s) Summary
Symlink resolution and venv base selection
crates/vm/src/getpath.rs, extra_tests/snippets/stdlib_sys.py
A Unix-only helper follows up to 40 final-component symlinks and normalizes relative targets lexically. For venv executables, a nonempty resolved target that differs from the executable takes precedence over the home-based fallback. POSIX tests cover symlink targets, copied executables, directory aliases, and launcher overrides.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: youknowone



Merge Risk: ⚪ Minimal · up to f29bb

No actionable issue is identified in this review; the change is mergeable after normal CI checks.

Pre-merge checks | 4 | 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

youknowone marked this pull request as ready for review October 10, 2026 11:56
youknowone enabled auto-merge (squash) October 10, 2026 11:56

chatgpt-codex-connector Bot commented Oct 10, 2026 •
edited
Loading

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-10T13:49:52.726784Z f29bb73 New commits
ℹ️ 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" or "@codex security review".

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 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

💡 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".

Comment thread crates/vm/src/getpath.rs
Comment on lines +321 to +323
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));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Preserve the root when joining relative link targets

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 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Identify the installed framework launcher by build configuration and file identity, then retain exact path assertions for launcher presets and directory canonicalization. Keep all cases and the production resolver unchanged.

Assisted-by: OpenAI Codex:model identifier unavailable
auto-merge was automatically disabled October 10, 2026 13:41

Head branch was pushed to by a user without write access

chatgpt-codex-connector 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

💡 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"):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Remove the prohibited regression-test body

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 👍 / 👎.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants


Back | FazBrowse Home | New Git URL