Move pylib -> crates/pylib
#6225
Conversation
WalkthroughThe pull request moves the pylib crate into a Changes
Sequence Diagram(s)sequenceDiagram
participant Build as build.rs
participant FS as Filesystem
Note over Build,FS `#dfefff`: CRATE_ROOT-based path resolution
Build->>Build: set CRATE_ROOT = "../.."
alt Non-Windows
Build->>FS: process_python_libs("./Lib/**/*")
else Windows
Build->>FS: process_python_libs("{CRATE_ROOT}/Lib/**/*")
end
Build->>FS: process_python_libs("{CRATE_ROOT}/vm/Lib/python_builtins/*")
Build->>FS: process_python_libs("{CRATE_ROOT}/vm/Lib/core_modules/*")
Note over FS: files read / assets bundled
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-09-28T22:22:55.921ZApplied to files:
⏰ 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). (10)
🔇 Additional comments (1)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/rustpython_pylib/build.rs (1)
7-12: Consider unifying the path handling across platforms.There's an inconsistency between Windows and non-Windows platforms:
- Line 9: Windows uses
"{CRATE_ROOT}/Lib/**/*"- Line 11: Non-Windows uses
"./Lib/**/*"The non-Windows path relies on the
./Libfile (which contains"../../Lib/") to resolve correctly, while Windows usesCRATE_ROOTdirectly. This difference in approach could be confusing and may lead to maintenance issues.Consider using
CRATE_ROOTconsistently across both platforms for clarity, unless there's a specific technical reason for the distinction.Apply this diff to unify the path handling:
#[cfg(feature = "freeze-stdlib")] if cfg!(windows) { process_python_libs(format!("{CRATE_ROOT}/Lib/**/*").as_str()); } else { - process_python_libs("./Lib/**/*"); + process_python_libs(format!("{CRATE_ROOT}/Lib/**/*").as_str()); }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Cargo.toml(2 hunks)crates/rustpython_pylib/Lib(1 hunks)crates/rustpython_pylib/build.rs(1 hunks)pylib/Lib(0 hunks)
💤 Files with no reviewable changes (1)
- pylib/Lib
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Format Rust code with the default rustfmt style (runcargo fmt)
Run clippy and fix any warnings or lints introduced by your changes
Follow Rust best practices for error handling and memory management
Files:
crates/rustpython_pylib/build.rs
🧠 Learnings (2)
📚 Learning: 2025-09-11T05:06:59.971Z
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-11T05:06:59.971Z
Learning: Applies to Lib/**/*.py : Minimize modifications to CPython standard library files; only make minimal changes to work around RustPython limitations
Applied to files:
crates/rustpython_pylib/build.rscrates/rustpython_pylib/Lib
📚 Learning: 2025-09-11T05:06:59.971Z
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-11T05:06:59.971Z
Learning: Applies to Lib/** : Do not edit the Lib/ directory directly; only copy files from CPython
Applied to files:
crates/rustpython_pylib/Lib
⏰ 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 on wasm-wasi
- GitHub Check: Run tests under miri
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
🔇 Additional comments (4)
Cargo.toml (1)
134-134: LGTM!The use of the glob pattern
"crates/*"to include all crates in thecrates/directory is a standard Rust workspace convention and simplifies the workspace configuration.crates/rustpython_pylib/build.rs (2)
1-2: LGTM!The
CRATE_ROOTconstant correctly represents the path from the new crate location (crates/rustpython_pylib/) to the repository root. This is a good refactoring that makes the paths more maintainable.
4-5: LGTM!The path updates using
CRATE_ROOTcorrectly resolve tovm/Lib/python_builtins/*andvm/Lib/core_modules/*from the new crate location.crates/rustpython_pylib/Lib (1)
1-1: I need to examine the full build.rs context to understand the complete picture:Now I need to check the actual build.rs file to see the full context and whether there's documentation:Let me examine the full build.rs file to see the complete implementation and any existing documentation:Let me search the codebase directly for the complete build.rs context:
The symlink mechanism is correctly implemented and functions as designed.
Based on the verification:
RustPython uses symlinks to manage python libraries in Lib/.
How it works across platforms: On non-Windows systems, the symlink at
crates/rustpython_pylib/Lib→../../Lib/resolves directly. On Windows, when packaging a crate with symlink, the file is not followed but packed as a file with relative path as content—so the build.rs script reads this "file" and canonicalizes the path.Documentation: The mechanism is documented in the RustPython README. For Windows users, the README instructs to set the RUSTPYTHONPATH environment variable as the Lib path in the project directory, and build.rs automatically resolves this for the binary at build time.
No issues detected. The file functions correctly, the build process handles both platforms as intended, and sufficient documentation exists in the project README.
youknowone
left a comment
youknowone
left a comment
There was a problem hiding this comment.
Based on the discussion on #6214, I approve it.
But this is more about the decision making.
This patch must be also approved by @coolreader18 before merge.
youknowone
left a comment
youknowone
left a comment
There was a problem hiding this comment.
by #6214, crates/pyilb please
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/pylib/build.rs (1)
9-11: Consider using CRATE_ROOT consistently across platforms.The Windows branch uses
CRATE_ROOTfor an absolute-like path, while non-Windows uses a relative"./Lib/**/*". This inconsistency could be confusing.For better consistency, consider:
#[cfg(feature = "freeze-stdlib")] if cfg!(windows) { process_python_libs(format!("{CRATE_ROOT}/Lib/**/*").as_str()); } else { - process_python_libs("./Lib/**/*"); + process_python_libs(format!("{CRATE_ROOT}/Lib/**/*").as_str()); }Alternatively, if the local symlink approach is intentional, consider adding a comment explaining why non-Windows uses a different path construction strategy.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Cargo.toml(2 hunks)crates/pylib/Lib(1 hunks)crates/pylib/build.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/pylib/Lib
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-28T22:22:55.921Z
Learnt from: reactive-firewall
Repo: RustPython/RustPython PR: 6176
File: .github/workflows/Check_Tests.yml:133-141
Timestamp: 2025-09-28T22:22:55.921Z
Learning: In the RustPython project's CI-5974-Test-RustPython-Integration action, the override-rustpython-path input is marked as required but has runtime fallback logic that defaults to RUSTPYTHONPATH environment variable or "Lib" if neither is provided, making explicit specification unnecessary in most cases.
Applied to files:
Cargo.toml
⏰ 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). (12)
- 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 snippets and cpython tests (ubuntu-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run tests under miri
- GitHub Check: auto_format
🔇 Additional comments (5)
Cargo.toml (2)
134-134: LGTM! Glob pattern aligns with crates directory structure.The change from explicit
"pylib"to the glob pattern"crates/*"is appropriate for the new directory layout and follows common Cargo workspace conventions.
155-155: No issues found with the path reference.The verification confirms that
crates/pylibis the actual directory and exists in the repository. The path reference in Cargo.toml line 155 (path = "crates/pylib") correctly points to this location. The code change is correct as-is.Likely an incorrect or invalid review comment.
crates/pylib/build.rs (3)
1-1: LGTM! CRATE_ROOT correctly references workspace root.The constant
"../.."appropriately navigates fromcrates/pylib/to the workspace root, improving path reference maintainability.
4-5: LGTM! Path references correctly updated for new crate location.Both paths appropriately use
CRATE_ROOTto reference the VM library directories from the newcrates/pylib/location.
14-23: LGTM! Windows symlink resolution logic is correct.The code appropriately handles Windows' symlink limitations by reading and canonicalizing the path from the
Libfile reference.
@youknowone is there a good reason for the difference really? why not remove the symlink and make all platforms to use the same path? (I'll do that in a separate PR if needed) |
ShaharNaveh
changed the title
pylib -> crates/rustpython_pylibpylib -> crates/pylib
|
When we publish this crates, a crate must be self-contained. Otherwise installing pylib via crates.io will be broken |
youknowone
left a comment
youknowone
left a comment
There was a problem hiding this comment.
tbh no idea what exactly was happened. I had to leave well-describing comments...
2 participants
related to #6214
Summary by CodeRabbit