fix sys path #6537
fix sys path #6537
Conversation
|
Warning Rate limit exceeded@youknowone has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 19 minutes and 7 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe changes reorder sys.path[0] initialization in lib.rs to align with CPython's behavior: site import now precedes path setup, with per-mode conditional path0 computation. Additionally, settings.rs filters empty environment variable values before path splitting to prevent empty strings from propagating through PATH-like variables. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Lib/test/test_cmd_line_script.pyis excluded by!Lib/**
📒 Files selected for processing (2)
src/lib.rssrc/settings.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
src/lib.rssrc/settings.rs
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to Lib/**/*.py : Minimize modifications to CPython standard library files in the `Lib/` directory; modifications should be minimal and only to work around RustPython limitations
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to Lib/**/*.py : Minimize modifications to CPython standard library files in the `Lib/` directory; modifications should be minimal and only to work around RustPython limitations
Applied to files:
src/lib.rs
🔇 Additional comments (2)
src/settings.rs (1)
342-342: LGTM!The filter correctly prevents empty environment variables from being split and propagated as empty path entries. This improves robustness when handling PATH-like environment variables.
src/lib.rs (1)
172-181: LGTM!The reordering to import site before configuring sys.path[0] correctly aligns with CPython's behavior. The comment clearly explains the rationale, and error handling is appropriate.
| // _PyPathConfig_ComputeSysPath0 - set sys.path[0] after site import | ||
| if !vm.state.config.settings.safe_path { | ||
| let path0: Option<String> = match &run_mode { | ||
| RunMode::Command(_) => Some(String::new()), | ||
| RunMode::Module(_) => env::current_dir() | ||
| .ok() | ||
| .and_then(|p| p.to_str().map(|s| s.to_owned())), | ||
| RunMode::Script(_) | RunMode::InstallPip(_) => None, // handled by run_script | ||
| RunMode::Repl => Some(String::new()), | ||
| }; | ||
|
|
||
| if let Some(path) = path0 { | ||
| vm.insert_sys_path(vm.new_pyobj(path))?; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for current_dir usage patterns and error handling
rg -nP -C3 'current_dir\(\)' --type=rustRepository: RustPython/RustPython
Length of output: 944
🏁 Script executed:
#!/bin/bash
# Read the context around the reviewed lines to understand the broader context
cat -n src/lib.rs | sed -n '170,210p'Repository: RustPython/RustPython
Length of output: 1904
🏁 Script executed:
#!/bin/bash
# Search for RunMode enum definition and Module variant usage
rg -nP 'enum RunMode|RunMode::Module' --type=rust -B2 -A2 | head -50
# Search for tests related to this initialization code
rg -nP 'test.*current_dir|test.*path0|test.*Module' --type=rust | head -30Repository: RustPython/RustPython
Length of output: 1151
Consider logging a warning for Module mode when current directory is unavailable, consistent with site import error handling.
The sys.path[0] initialization for RunMode::Module silently converts env::current_dir() errors to None (lines 187-189), which is inconsistent with the site import error handling above it (lines 177-180). While graceful degradation is acceptable, adding a warning log for Module mode failures would improve debugging and match the established error handling pattern in this initialization sequence.
🤖 Prompt for AI Agents
In src/lib.rs around lines 183 to 197, the RunMode::Module branch silently drops
errors from env::current_dir(); instead of swallowing the error, capture the
Result error and emit a warning using the same warning/logging mechanism used
above for site import failures, then fall back to None; i.e., if
env::current_dir() fails, log a warning that includes the error and context
(Module mode failing to determine current directory) and then proceed with None
so behavior is unchanged but diagnostics are improved.
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.