Introduce PyConfig to implement Paths correctly #6461
Conversation
WalkthroughAdds a Paths struct and PyConfig combining Settings+Paths; introduces a new getpath module to compute runtime paths; changes VM construction and many modules to read configuration from Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
youknowone
force-pushed
the
getpath
branch
2 times, most recently
from
d2c4418 to
82e213e
Compare
|
Code has been automatically formatted The code in this PR has been formatted using git pull origin getpath |
youknowone
force-pushed
the
getpath
branch
2 times, most recently
from
77307f5 to
561304d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib.rs (1)
235-247:flame-itprofiling still references removedsettingsfield
PyGlobalStatenow storesconfig: PyConfig, sovm.state.as_ref().settingsno longer exists. Underfeature = "flame-it"this will fail to compile.Update the call to pull
Settingsfromconfig:Proposed fix
- if let Err(e) = write_profile(&vm.state.as_ref().settings) { + if let Err(e) = write_profile(&vm.state.as_ref().config.settings) { error!("Error writing profile information: {}", e); }
🧹 Nitpick comments (3)
src/interpreter.rs (2)
50-55: Redundantinit_path_configcall beforeInterpreter::with_initHere you call
rustpython_vm::getpath::init_path_config(&settings)and ignore the result, whilerustpython_vm::Interpreter::with_initnow also computes paths and wraps them inPyConfig.Unless you intend side effects inside
init_path_config(it appears pure), this call is redundant work on every startup and can be removed.Proposed simplification
- let settings = self.settings.unwrap_or_default(); - - // Initialize path configuration before interpreter creation (like getpath.py) - rustpython_vm::getpath::init_path_config(&settings); - - Interpreter::with_init(settings, |vm| { + let settings = self.settings.unwrap_or_default(); + Interpreter::with_init(settings, |vm| { for hook in self.init_hooks { hook(vm); } })
112-131: Stdlib path injection into bothpath_listandmodule_search_pathslooks soundCollecting
additional_pathsfromBUILDTIME_RUSTPYTHONPATH(orrustpython_pylib::LIB_PATH) and then prepending them to bothstate.config.settings.path_listandstate.config.paths.module_search_pathsensures:
- legacy
Settings::path_listconsumers still see the stdlib paths, andsys.path(driven bymodule_search_paths) has stdlib ahead of user paths.This matches the intent of the new
Pathsmodel and aligns CLI behavior with the VM config. You might later consider de‑duplicating paths ifgetpathends up inserting the same entries, but functionally this is correct.crates/vm/src/getpath.rs (1)
376-381: Use platform-agnostic path for test portability.The hardcoded
/tmppath won't exist on Windows. Consider usingstd::env::temp_dir()or a cross-platform approach.🔎 Suggested fix
#[test] fn test_search_up() { // Test with a path that doesn't have any landmarks - let result = search_up_file("/tmp", &["nonexistent_landmark_xyz"]); + let result = search_up_file(std::env::temp_dir(), &["nonexistent_landmark_xyz"]); assert!(result.is_none()); }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.cspell.dict/cpython.txt(1 hunks)crates/stdlib/src/syslog.rs(1 hunks)crates/vm/src/getpath.rs(1 hunks)crates/vm/src/import.rs(1 hunks)crates/vm/src/lib.rs(1 hunks)crates/vm/src/stdlib/builtins.rs(2 hunks)crates/vm/src/stdlib/imp.rs(1 hunks)crates/vm/src/stdlib/io.rs(1 hunks)crates/vm/src/stdlib/signal.rs(1 hunks)crates/vm/src/stdlib/sys.rs(7 hunks)crates/vm/src/vm/compile.rs(1 hunks)crates/vm/src/vm/interpreter.rs(2 hunks)crates/vm/src/vm/mod.rs(11 hunks)crates/vm/src/vm/setting.rs(1 hunks)src/interpreter.rs(2 hunks)src/lib.rs(4 hunks)
🧰 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:
crates/stdlib/src/syslog.rscrates/vm/src/vm/setting.rssrc/lib.rscrates/vm/src/lib.rscrates/vm/src/stdlib/imp.rscrates/vm/src/import.rssrc/interpreter.rscrates/vm/src/getpath.rscrates/vm/src/stdlib/builtins.rscrates/vm/src/vm/compile.rscrates/vm/src/stdlib/io.rscrates/vm/src/vm/mod.rscrates/vm/src/stdlib/signal.rscrates/vm/src/vm/interpreter.rscrates/vm/src/stdlib/sys.rs
🧠 Learnings (1)
📚 Learning: 2025-12-09T08:46:58.660Z
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6358
File: crates/vm/src/exception_group.rs:173-185
Timestamp: 2025-12-09T08:46:58.660Z
Learning: In crates/vm/src/exception_group.rs, the derive() method intentionally always creates a BaseExceptionGroup instance rather than preserving the original exception class type. This is a deliberate design decision that differs from CPython's behavior.
Applied to files:
crates/vm/src/stdlib/builtins.rs
🧬 Code graph analysis (4)
crates/stdlib/src/syslog.rs (1)
crates/vm/src/stdlib/sys.rs (1)
argv(121-129)
crates/vm/src/vm/setting.rs (1)
crates/vm/src/vm/mod.rs (1)
new(117-220)
crates/vm/src/vm/mod.rs (1)
crates/vm/src/vm/setting.rs (1)
new(32-34)
crates/vm/src/vm/interpreter.rs (4)
crates/vm/src/getpath.rs (1)
init_path_config(108-171)src/interpreter.rs (2)
settings(63-66)new(47-49)crates/vm/src/vm/mod.rs (1)
new(117-220)crates/vm/src/vm/setting.rs (1)
new(32-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run snippets and cpython tests (windows-latest)
🔇 Additional comments (34)
.cspell.dict/cpython.txt (1)
45-45: Correct dictionary maintenance for new terminology.The new entry
platstdlibis correctly inserted in alphabetical order betweenpatmaandposonlyarg. This addition supports the new path configuration terminology introduced by thePyConfigrefactor.crates/stdlib/src/syslog.rs (1)
29-29: LGTM! Configuration access path updated correctly.The migration from
vm.state.settings.argvtovm.state.config.settings.argvis consistent with the broader refactoring to use PyConfig-based configuration.crates/vm/src/stdlib/imp.rs (1)
94-94: LGTM! Configuration access updated correctly.The change from
vm.state.settings.check_hash_pycs_modetovm.state.config.settings.check_hash_pycs_modealigns with the PyConfig-based configuration model.crates/vm/src/vm/setting.rs (1)
4-21: LGTM! Well-structured path configuration.The
Pathsstruct provides a clean separation between user-configurable settings and computed runtime paths, with clear documentation for each field mapping to Python'ssysmodule attributes.crates/vm/src/vm/compile.rs (1)
41-41: LGTM! Configuration access path updated correctly.The migration from
vm.state.settings.safe_pathtovm.state.config.settings.safe_pathis consistent with the PyConfig-based configuration model.crates/vm/src/lib.rs (1)
63-63: LGTM! Public getpath module added.Adding the
getpathmodule to the public API surface is necessary to expose path configuration functionality for VM initialization, aligning with the PR's objective to implement Python-like path configuration.crates/vm/src/import.rs (1)
209-209: LGTM! Configuration access path updated correctly.The migration from
vm.state.settings.verbosetovm.state.config.settings.verboseis consistent with the broader PyConfig-based refactoring.crates/vm/src/stdlib/signal.rs (1)
117-117: LGTM! Configuration access path updated correctly.The migration from
vm.state.settings.install_signal_handlerstovm.state.config.settings.install_signal_handlersis consistent with the PyConfig-based configuration model.crates/vm/src/stdlib/builtins.rs (2)
135-135: LGTM! Configuration access path updated correctly.The migration from
vm.state.settings.optimizetovm.state.config.settings.optimizeis consistent with the PyConfig-based configuration model. The logic correctly defaults to the configured optimization level when the compile() function's optimize parameter is -1.
1083-1083: LGTM! Configuration access path updated correctly.The migration from
vm.state.settings.optimizetovm.state.config.settings.optimizeis consistent with the PyConfig-based configuration model. The debug mode derivation logic remains unchanged.src/lib.rs (2)
172-180: Config-based safe-path check looks correctUsing
vm.state.config.settings.safe_pathinstead of the oldvm.state.settings.safe_pathkeeps behavior the same while aligning with the newPyConfiglayout. No issues here.
192-203: Faulthandler flag and banner gating correctly migrated toconfig.settingsSwitching to
vm.state.config.settings.faulthandler,quiet, andverbosepreserves semantics and matches the new configuration model. The REPL banner conditions remain unchanged logically.crates/vm/src/stdlib/io.rs (1)
2397-2412: UTF-8 mode lookup correctly switched toconfig.settingsUsing
vm.state.config.settings.utf8_modein theencodingselection preserves previous behavior and is consistent with the newPyConfiglayout. No further changes needed here.crates/vm/src/vm/interpreter.rs (1)
46-58: Config + path initialization inInterpreter::with_initis well-structuredCreating
paths = getpath::init_path_config(&settings)and thenPyConfig::new(settings, paths)before callingVirtualMachine::new(config, ctx)cleanly encapsulates all runtime configuration intoPyConfigwhile keeping the public API asSettings.This is the right place to centralize path configuration for both
without_stdliband stdlib-enabled interpreters.crates/vm/src/vm/mod.rs (6)
53-106: Global state now correctly wraps settings inPyConfigRe‑exporting
PathsandPyConfig, and changingPyGlobalStateto holdconfig: PyConfig, is consistent with the new configuration model. This keeps all VM settings and derived paths together while preserving access to other global fields.
115-190:VirtualMachine::newcorrectly adapted toPyConfigUsing
config.settings.hash_seedandconfig.settings.int_max_str_digitsto derive the hash secret and digit limit matches previous behavior, and storing the wholeconfigintoPyGlobalStateensures later code can access both raw settings and paths viaself.state.config. No structural or initialization issues spotted.
230-259: Encoding import diagnostics now referenceconfig.settings.path_listSwitching
path_contains_envto iterate overself.state.config.settings.path_listkeeps the guidance messages accurate under the new config structure; callers still configure search paths viaSettings::path_list, just accessed throughconfig.
303-359: Stdio initialization usingconfig.settings.buffered_stdiolooks correctReading
buffered_stdiofromself.state.config.settingsinsidemake_stdiomaintains the previous buffered/unbuffered behavior and centralizes the flag in the new config. The rest of the logic (choosing buffering mode, line buffering, and wrapping inTextIOWrapper) is unchanged.
367-395: External library and stdlib presence checks correctly migrated
- The
allow_external_librarygate now usesself.state.config.settings.allow_external_library, keeping semantics for importlib package initialization._expect_stdlibnow checks!self.state.config.settings.path_list.is_empty(), which is consistent with the new config layout and still signals whether stdlib paths are configured.Both changes are simple, accurate migrations.
506-509: Compile options now correctly readoptimizefromconfig.settings
compile_opts()usingself.state.config.settings.optimizekeeps behavior identical while routing throughPyConfig. No issues here.crates/vm/src/getpath.rs (7)
1-11: LGTM! Clear module documentation and appropriate imports.The module structure follows Python's getpath.py reference with proper landmark-based search approach.
12-66: LGTM! Platform-specific landmarks correctly mirror CPython's getpath.py.The Windows/POSIX split with appropriate directory structures (
lib/pythonX.YvsLib,lib-dynloadvsDLLs) aligns with standard Python installations.
68-100: LGTM! Well-designed upward search helpers with safe termination.The loop correctly terminates when
current.pop()returnsfalseat the filesystem root, preventing infinite loops.
108-171: LGTM! Clear step-by-step implementation following CPython's getpath.py algorithm.The function properly handles venv detection, build directory detection, and prefix calculation with clear documentation for each step.
186-215: LGTM!The function correctly checks both standard venv layouts (
../pyvenv.cfgand./pyvenv.cfg). The_executableparameter is appropriately prefixed to indicate intentional non-use.
235-259: LGTM! Correct priority order for prefix calculation.The search order (build directory → ZIP landmark → stdlib landmarks → default) follows CPython's getpath.py behavior.
300-333: LGTM! Path construction matches CPython behavior.Adding the ZIP path regardless of existence is correct—CPython's
sys.pathalso includes computed paths without validating their existence.crates/vm/src/stdlib/sys.rs (7)
97-112: LGTM! Prefix accessors correctly migrated to config.paths.The return type change from
&'static strtoStringis appropriate since these values are now computed at runtime rather than being compile-time constants.
120-129: LGTM!Correctly accesses
argvfromconfig.settingsas it's a user-provided setting.
159-172: LGTM! Executable paths correctly sourced from config.paths.Both
_base_executableandexecutableproperly read from the computed paths rather than re-computing them.
208-217: LGTM!
sys.pathcorrectly reads fromconfig.paths.module_search_paths, which is computed byinit_path_config.
246-269: LGTM!Both
_xoptionsandwarnoptionscorrectly access their data fromconfig.settings.
406-409: LGTM!Flags correctly derived from
config.settingsviaFlagsData::from_settings.
164-167: LGTM!Correctly accesses
write_bytecodefromconfig.settings.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/vm/src/vm/setting.rs (1)
23-35: Consider derivingDebugandCloneforPyConfig.
PathsderivesDebug, Clone, Default, butPyConfighas no derives. For debugging and potential cloning needs (e.g., spawning sub-interpreters or diagnostics), consider adding:+#[derive(Debug, Clone)] pub struct PyConfig { pub settings: Settings, pub paths: Paths, }Note:
Settingswould also needDebugandClonederives for this to work. IfSettingscan't easily derive these due to conditional fields, this can be deferred.crates/vm/src/getpath.rs (1)
350-362: Silent failure on pyvenv.cfg read errors.
parse_pyvenv_homeuses.ok()?which silently ignores file read errors. While this is acceptable for optional venv detection, consider logging at verbose/debug level when running with-vfor better diagnostics.This is a minor enhancement that could be deferred.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.cspell.dict/cpython.txt(1 hunks)crates/stdlib/src/syslog.rs(1 hunks)crates/vm/src/getpath.rs(1 hunks)crates/vm/src/import.rs(1 hunks)crates/vm/src/stdlib/builtins.rs(2 hunks)crates/vm/src/stdlib/imp.rs(1 hunks)crates/vm/src/stdlib/io.rs(1 hunks)crates/vm/src/stdlib/signal.rs(1 hunks)crates/vm/src/stdlib/sys.rs(7 hunks)crates/vm/src/vm/compile.rs(1 hunks)crates/vm/src/vm/interpreter.rs(2 hunks)crates/vm/src/vm/mod.rs(11 hunks)crates/vm/src/vm/setting.rs(1 hunks)src/interpreter.rs(1 hunks)src/lib.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- crates/vm/src/stdlib/io.rs
- crates/vm/src/vm/compile.rs
- src/lib.rs
- crates/vm/src/import.rs
- crates/vm/src/stdlib/signal.rs
- crates/vm/src/stdlib/imp.rs
- src/interpreter.rs
- crates/stdlib/src/syslog.rs
- .cspell.dict/cpython.txt
🧰 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:
crates/vm/src/stdlib/builtins.rscrates/vm/src/vm/interpreter.rscrates/vm/src/vm/setting.rscrates/vm/src/vm/mod.rscrates/vm/src/getpath.rscrates/vm/src/stdlib/sys.rs
🧠 Learnings (1)
📚 Learning: 2025-12-09T08:46:58.660Z
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6358
File: crates/vm/src/exception_group.rs:173-185
Timestamp: 2025-12-09T08:46:58.660Z
Learning: In crates/vm/src/exception_group.rs, the derive() method intentionally always creates a BaseExceptionGroup instance rather than preserving the original exception class type. This is a deliberate design decision that differs from CPython's behavior.
Applied to files:
crates/vm/src/stdlib/builtins.rs
🧬 Code graph analysis (4)
crates/vm/src/vm/interpreter.rs (4)
crates/vm/src/getpath.rs (1)
init_path_config(108-171)src/interpreter.rs (2)
settings(59-62)new(47-49)crates/vm/src/vm/setting.rs (1)
new(32-34)crates/vm/src/vm/mod.rs (1)
new(117-220)
crates/vm/src/vm/setting.rs (1)
crates/vm/src/vm/mod.rs (1)
new(117-220)
crates/vm/src/vm/mod.rs (1)
crates/vm/src/vm/setting.rs (1)
new(32-34)
crates/vm/src/getpath.rs (2)
crates/vm/src/stdlib/sys.rs (8)
std(443-443)std(584-584)std(1069-1069)std(1099-1099)path(209-217)executable(170-172)prefix(98-100)exec_prefix(106-108)crates/vm/src/vm/setting.rs (1)
default(171-205)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check the WASM package and demo
🔇 Additional comments (21)
crates/vm/src/stdlib/builtins.rs (1)
135-135: LGTM!The configuration access path updates from
vm.state.settings.optimizetovm.state.config.settings.optimizeare consistent with the broader PyConfig migration in this PR. The logic remains unchanged.Also applies to: 1083-1083
crates/vm/src/vm/setting.rs (1)
4-21: Well-structured path configuration.The
Pathsstruct cleanly encapsulates all runtime path configuration fields that mirror CPython'sPyConfigpath outputs. The documentation comments for each field clearly indicate their purpose (e.g.,sys.executable,sys.prefix).crates/vm/src/vm/interpreter.rs (1)
50-57: Clean initialization flow for path configuration.The initialization sequence properly separates concerns:
- Compute path configuration from settings
- Create
PyConfigcombining settings and paths- Pass the unified config to
VirtualMachine::newThis integrates well with the new
getpathmodule and maintains a clear data flow.crates/vm/src/vm/mod.rs (4)
53-53: Public exports correctly expanded.Adding
PathsandPyConfigto the public exports alongside existingCheckHashPycsModeandSettingsproperly exposes the new configuration types for downstream users.
89-90: State structure properly updated.
PyGlobalStatenow holdsconfig: PyConfiginstead of rawsettings, centralizing configuration access through the new unified structure.
117-174: VM construction cleanly migrated to PyConfig.The
VirtualMachine::newsignature change and internal initialization logic properly consume the newPyConfigstructure. All config field accesses (hash_seed, int_max_str_digits) correctly useconfig.settings.*.
230-230: Consistent config access pattern throughout.All internal usages have been systematically updated to use
self.state.config.settings.*instead ofself.state.settings.*, maintaining consistency across the codebase.Also applies to: 306-306, 367-367, 378-378, 508-508
crates/vm/src/getpath.rs (5)
1-11: Well-documented module following CPython's getpath.py pattern.The module documentation clearly explains the purpose and references the CPython equivalent. The landmark-based search approach mirrors Python's path calculation logic.
72-89: Robust upward search implementation.The
search_upfunction correctly handles the base case by checkingcurrent.pop()return value, preventing infinite loops when reaching the filesystem root.
108-171: Solid path configuration initialization.The
init_path_configfunction follows a logical flow:
- Get executable path
- Check environment variables
- Detect venv configuration
- Detect build directory
- Calculate prefix/base_prefix
- Calculate exec_prefix
- Calculate base_executable
- Build module search paths
The step-by-step comments align with the implementation, making the code maintainable.
364-388: Good test coverage for basic scenarios.The tests verify non-panicking initialization, upward search behavior, and default prefix generation. Consider adding tests for venv detection and build directory detection in future iterations.
336-347: Thewhichcrate dependency is properly configured.The
whichcrate is listed incrates/vm/Cargo.tomlaswhich = "8"under[target.'cfg(not(target_arch = "wasm32"))'.dependencies], which correctly aligns with the code's conditional compilation. The edge case handling via the?operator is correct—env::args_os().next()?properly returnsNoneif no arguments exist, and the function's return typeOption<PathBuf>propagates this appropriately.crates/vm/src/stdlib/sys.rs (9)
98-112: Systematic migration tovm.state.config.pathslooks correct.The prefix-related functions now correctly source their values from the centralized
PyConfig.pathsstructure. The return type change from&'static strtoStringis consistent with the PR's objective to compute paths dynamically.
121-129: LGTM:argvcorrectly migrated tovm.state.config.settings.
160-162: LGTM: Simplified return type fromPyObjectReftoString.
165-167: LGTM:dont_write_bytecodecorrectly migrated.
170-172: LGTM: Simplified return type fromPyObjectReftoString.
209-217: LGTM:pathcorrectly sources fromvm.state.config.paths.module_search_paths.
247-258: LGTM:_xoptionscorrectly migrated tovm.state.config.settings.
261-269: LGTM:warnoptionscorrectly migrated tovm.state.config.settings.
407-409: LGTM:flagscorrectly constructs fromvm.state.config.settings.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/vm/src/getpath.rs (1)
188-191: Consider removing unused parameter or documenting future use.The
_executableparameter is prefixed with underscore indicating intentional non-use, but it may be worth a brief comment explaining if this is for future compatibility with Python's behavior.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.cspell.dict/cpython.txt(1 hunks)crates/stdlib/src/syslog.rs(1 hunks)crates/vm/src/getpath.rs(1 hunks)crates/vm/src/import.rs(1 hunks)crates/vm/src/stdlib/builtins.rs(2 hunks)crates/vm/src/stdlib/imp.rs(1 hunks)crates/vm/src/stdlib/io.rs(1 hunks)crates/vm/src/stdlib/signal.rs(1 hunks)crates/vm/src/stdlib/sys.rs(7 hunks)crates/vm/src/vm/compile.rs(1 hunks)crates/vm/src/vm/interpreter.rs(2 hunks)crates/vm/src/vm/mod.rs(13 hunks)crates/vm/src/vm/setting.rs(1 hunks)src/interpreter.rs(1 hunks)src/lib.rs(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- .cspell.dict/cpython.txt
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/vm/src/stdlib/signal.rs
- crates/vm/src/stdlib/imp.rs
- src/interpreter.rs
- crates/stdlib/src/syslog.rs
- crates/vm/src/vm/compile.rs
- crates/vm/src/stdlib/io.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:
crates/vm/src/import.rscrates/vm/src/stdlib/builtins.rscrates/vm/src/vm/interpreter.rscrates/vm/src/vm/setting.rscrates/vm/src/getpath.rscrates/vm/src/vm/mod.rscrates/vm/src/stdlib/sys.rssrc/lib.rs
🧠 Learnings (1)
📚 Learning: 2025-12-09T08:46:58.660Z
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6358
File: crates/vm/src/exception_group.rs:173-185
Timestamp: 2025-12-09T08:46:58.660Z
Learning: In crates/vm/src/exception_group.rs, the derive() method intentionally always creates a BaseExceptionGroup instance rather than preserving the original exception class type. This is a deliberate design decision that differs from CPython's behavior.
Applied to files:
crates/vm/src/stdlib/builtins.rs
🧬 Code graph analysis (3)
crates/vm/src/vm/interpreter.rs (3)
crates/vm/src/getpath.rs (1)
init_path_config(108-171)crates/vm/src/vm/setting.rs (1)
new(32-34)crates/vm/src/vm/mod.rs (1)
new(117-220)
crates/vm/src/vm/setting.rs (1)
crates/vm/src/vm/mod.rs (1)
new(117-220)
crates/vm/src/vm/mod.rs (1)
crates/vm/src/vm/setting.rs (1)
new(32-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Check Rust code with clippy
🔇 Additional comments (26)
crates/vm/src/getpath.rs (7)
1-11: LGTM! Well-structured module with clear documentation.The module header clearly references Python's getpath.py and explains the purpose. The imports are clean and appropriate for the functionality.
14-66: LGTM! Platform-specific landmarks are well-designed.The platform modules provide a clean abstraction with consistent interfaces across POSIX and Windows. Version-based path construction correctly uses the crate's version constants.
72-89: LGTM! Clean upward directory search implementation.The generic
search_upfunction is well-designed with a test closure pattern. The loop correctly terminates whenpop()returns false at the filesystem root.
108-171: LGTM! Well-structured path configuration workflow.The function follows Python's getpath.py workflow with clear step-by-step comments. Each step has appropriate fallbacks, ensuring the function never fails.
284-298: LGTM! Correct base_executable derivation logic.The function properly handles venv scenarios by constructing the path from the home directory, with a sensible fallback to the current executable.
335-347: LGTM! Platform-appropriate executable path resolution.Using
which::whichfor proper PATH resolution on native platforms and falling back to direct path on wasm32 is appropriate.
364-388: Tests provide basic coverage.The tests verify non-panicking behavior and basic functionality. Consider adding tests for venv detection and build directory detection in follow-up work.
crates/vm/src/import.rs (1)
209-209: LGTM! Configuration access path correctly updated.The change from
vm.state.settings.verbosetovm.state.config.settings.verboseis consistent with the PR's migration to PyConfig-based configuration.crates/vm/src/stdlib/builtins.rs (2)
134-135: LGTM! Optimize setting access correctly migrated.The change to
vm.state.config.settings.optimizeis consistent with the configuration migration.
1083-1083: LGTM! Debug mode derivation correctly updated.The
debug_modecalculation now uses the migrated config path consistently with other changes in this PR.src/lib.rs (5)
172-180: LGTM! Safe path check correctly migrated.The configuration access is properly updated. The inline Python code for path insertion is appropriate for this use case.
192-198: LGTM! Faulthandler check correctly migrated.The faulthandler initialization logic correctly uses the new config path.
201-202: LGTM! Quiet and verbose checks correctly migrated.Both settings are now accessed through the unified
vm.state.config.settingspath.
235-235: LGTM! Inspect check correctly migrated.The inspect flag access is consistent with other configuration migrations.
244-244: LGTM! Profile settings access correctly migrated.The flame profiler settings are now accessed through the new config structure.
crates/vm/src/vm/interpreter.rs (2)
1-2: LGTM! Imports correctly updated for new configuration system.The imports properly bring in
PyConfigfrom the parent module andgetpathfrom the crate for path configuration initialization.
50-57: LGTM! Core integration of path configuration into VM initialization.The initialization flow correctly:
- Computes paths from settings via
getpath::init_path_config- Wraps settings and paths into
PyConfig- Passes the unified config to
VirtualMachine::newThis cleanly separates path computation from VM construction while maintaining a single configuration object.
crates/vm/src/vm/setting.rs (1)
4-35: LGTM! Clean introduction of PyConfig and Paths.The new
PathsandPyConfigstructs cleanly separate computed runtime paths from user-configurable settings. The design maintains good ownership discipline by keeping them as separate structs within a combined configuration container.crates/vm/src/vm/mod.rs (4)
53-53: LGTM! Public API updated to expose PyConfig and Paths.The export list correctly includes the new
PathsandPyConfigtypes alongside the existingSettingsexport.
230-232: LGTM! Path accesses updated to use module_search_paths.All path-related logic correctly uses
config.paths.module_search_pathsinstead of the deprecatedsettings.path_list. Error messages have also been updated to reference the new structure.Also applies to: 241-241, 378-378, 392-393
306-306: LGTM! Settings accesses updated consistently.All settings accesses correctly use
config.settings.*for buffered_stdio, allow_external_library, and optimize.Also applies to: 367-367, 508-508
90-90: LGTM! VirtualMachine refactored to use PyConfig consistently.The refactoring correctly updates the configuration approach:
PyGlobalStatenow usesconfig: PyConfig(line 90)VirtualMachine::newsignature updated to acceptPyConfig(line 117)- All internal accesses properly use
config.settingsto retrieve values (lines 144, 154, 174)- External callsite in
Interpreter::with_initcorrectly constructsPyConfigfromSettingsbefore passing it toVirtualMachine::new(interpreter.rs:53-57)The pattern is clean and consistent throughout.
crates/vm/src/stdlib/sys.rs (4)
98-112: LGTM! Prefix functions updated to use PyConfig paths.The
prefix,base_prefix,exec_prefix, andbase_exec_prefixfunctions correctly returnStringvalues fromvm.state.config.paths.*instead of static strings. The cloning is appropriate for these configuration getters.
160-162: LGTM! Executable paths correctly sourced from PyConfig.Both
_base_executableandexecutablenow returnStringvalues fromvm.state.config.paths.*, consistent with the path configuration refactoring.Also applies to: 170-172
121-129: LGTM! argv and path correctly use PyConfig.
argv(lines 121-129) correctly accessesvm.state.config.settings.argvpath(lines 209-217) correctly accessesvm.state.config.paths.module_search_pathsThe refactoring properly separates user-provided arguments from computed search paths.
Also applies to: 209-217
165-167: LGTM! Remaining sys attributes updated consistently.All remaining functions (
dont_write_bytecode,_xoptions,warnoptions,flags) correctly access configuration throughvm.state.config.settings.*. The refactoring is complete and consistent.Also applies to: 247-258, 261-269, 407-409
1 participant
Summary by CodeRabbit
New Features
Improvements
Other
✏️ Tip: You can customize this high-level summary in your review settings.