GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||
GeezerVison is showing github.com — nt junction by youknowone · Pull Request #6407 · RustPython/RustPython Conversation
|
WalkthroughThe PR enhances Windows path handling in RustPython. It strips extended-length path prefixes in the build script, adds Windows system feature flags to the cargo manifest, and introduces Windows-specific file-type classification utilities, path existence checks, and readlink support with OsPathOrFd-based operations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/pylib/build.rs (1)
19-22: Consider handling potential UTF-8 conversion failure.The
unwrap()on Line 20 assumes the canonicalized path is valid UTF-8, which may not always be true on Windows with certain file names. Consider usingexpect()with a descriptive message or handling the error more gracefully.Apply this diff to provide a better error message:
- let path_str = canonicalized_path.to_str().unwrap(); + let path_str = canonicalized_path.to_str() + .expect("Failed to convert canonicalized path to UTF-8");crates/vm/src/stdlib/nt.rs (1)
285-338: Consider documenting thedisk_onlyparameter.The
disk_onlyparameter's purpose and behavior are not immediately clear from the code. Adding a doc comment explaining when it should be true/false would improve maintainability.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Lib/test/test_ntpath.pyis excluded by!Lib/**Lib/test/test_os.pyis excluded by!Lib/**
📒 Files selected for processing (3)
crates/pylib/build.rs(1 hunks)crates/vm/Cargo.toml(1 hunks)crates/vm/src/stdlib/nt.rs(8 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/pylib/build.rscrates/vm/src/stdlib/nt.rs
🧠 Learnings (5)
📓 Common learnings
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.
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: 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.
📚 Learning: 2025-06-28T16:31:03.991Z
Learnt from: arihant2math
Repo: RustPython/RustPython PR: 5790
File: build.rs:2-2
Timestamp: 2025-06-28T16:31:03.991Z
Learning: In Cargo build scripts (build.rs), the environment variable CARGO_CFG_TARGET_OS is guaranteed to exist and is automatically set by Cargo during the build process, making unwrap() safe to use when accessing this variable.
Applied to files:
crates/pylib/build.rscrates/vm/Cargo.toml
📚 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:
crates/pylib/build.rs
📚 Learning: 2025-06-27T14:47:28.810Z
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.
Applied to files:
crates/vm/src/stdlib/nt.rs
📚 Learning: 2025-06-27T14:47:28.810Z
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.
Applied to files:
crates/vm/src/stdlib/nt.rs
🔇 Additional comments (8)
crates/vm/Cargo.toml (1)
127-129: LGTM!The new Windows-specific features
Win32_System_IOandWin32_System_Kernelare correctly added to support the enhanced Windows path handling functionality in nt.rs, including DeviceIoControl for reparse point operations and PROCESS_BASIC_INFORMATION for process queries.crates/vm/src/stdlib/nt.rs (7)
240-250: LGTM!The
PathArghelper struct provides a clean abstraction for converting Python path-like objects to either paths or file descriptors, following the established pattern in the codebase.
252-283: LGTM!The internal file type constants and
_test_infofunction correctly classify Windows file types using file attributes and reparse tags. The logic properly distinguishes between regular files, directories, symlinks, and mount points (junctions).
340-414: LGTM, but note the complex fallback logic.The function correctly handles multiple fallback scenarios when opening files fails, including using Rust's standard library metadata as a fallback. This complexity is necessary for robust Windows file type checking across different file types and permissions.
416-517: LGTM!The existence checking functions provide comprehensive handling of Windows-specific edge cases, including console devices like
\\.\CON. The wrapper functions cleanly abstract the differences between file descriptor and path-based operations.
519-565: LGTM!The Python-exposed path checking functions provide a clean, consistent interface for Windows-specific file type and existence checks, including the Windows-specific junction detection.
1478-1528: The field nameInheritedFromUniqueProcessIdis correct forwindows-sys0.61.2 and matches thePROCESS_BASIC_INFORMATIONstructure definition. The field is properly defined as a pointer-sized integer (usizein Rust), and the code correctly casts it tou32when returning the parent process ID. No changes are needed.
1569-1693: Revert the suggested refactor—the current implementation is correct.The manual byte parsing in the
readlinkfunction correctly implements the Windows REPARSE_DATA_BUFFER structure per official documentation. The byte offsets (8, 10, 16 for mount points; 20 for symlinks) are not "magic numbers"—they represent the documented Windows API structure layout. Usingrepr(C)struct definitions, as originally suggested, would be unsafe and incorrect because the REPARSE_DATA_BUFFER contains a union-like payload that varies by reparse tag type. The current approach of manually parsing from&[u8]is the recommended pattern for this Windows API and avoids undefined behavior. The existing comments accurately document the structure layout. No changes needed.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
1 participant
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.