impl os functions #6484
impl os functions #6484
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant PyObj as Python Object
participant PathConv as PathConverter
participant FDCheck as FD Parser
participant FsPath as fspath Resolver
participant StrMatch as String/Bytes
participant Output as OutputMode
PyObj->>PathConv: try_path_or_fd(obj)
alt FD Available
PathConv->>FDCheck: Check if int
FDCheck-->>PathConv: FD parsed
PathConv-->>Output: Return FD result
else No FD
PathConv->>FsPath: Try __fspath__()
alt fspath Success
FsPath-->>PathConv: Return path object
PathConv->>Output: Determine mode from origin
else fspath Failed
PathConv->>StrMatch: Match string/bytes
alt Match Success
StrMatch-->>PathConv: Path extracted
PathConv->>Output: Determine mode (String/Bytes)
else No Match
PathConv-->>Output: Error with context
end
end
end
rect rgb(200, 220, 240)
note over PathConv,Output: PathConverter configurable with<br/>function_name, argument_name, non_strict
end
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/vm/src/stdlib/nt.rs (1)
1591-1612: Consider cloning path only once for error handling.The
path.clone()is called twice in error paths (lines 1610 and 1638). Sincepathis moved at line 1592, consider storing the mode and a cloned path for errors upfront to avoid redundant clones.🔎 Suggested optimization
fn readlink(path: OsPath, vm: &VirtualMachine) -> PyResult { ... let mode = path.mode(); - let wide_path = path.as_ref().to_wide_with_nul(); + let wide_path = path.as_ref().to_wide_with_nul(); + // path is still available for error reporting after this pointNote: The current code is correct since
as_ref()borrows, but keepingpathalive for error messages is the intended pattern here.crates/vm/src/stdlib/posix.rs (1)
1259-1265: Redundant import shadows outer scope import.The
use std::os::fd::BorrowedFd;at line 1260 shadows the import already present at line 4 (use std::os::fd::BorrowedFd;). The same applies totcsetpgrpat line 1270. While functionally correct, this is unnecessary.🔎 Suggested cleanup
#[cfg(not(any(target_os = "wasi", target_os = "redox")))] #[pyfunction] fn tcgetpgrp(fd: i32, vm: &VirtualMachine) -> PyResult<libc::pid_t> { - use std::os::fd::BorrowedFd; let fd = unsafe { BorrowedFd::borrow_raw(fd) }; unistd::tcgetpgrp(fd) .map(|pid| pid.as_raw()) .map_err(|err| err.into_pyexception(vm)) } #[cfg(not(any(target_os = "wasi", target_os = "redox")))] #[pyfunction] fn tcsetpgrp(fd: i32, pgid: libc::pid_t, vm: &VirtualMachine) -> PyResult<()> { - use std::os::fd::BorrowedFd; let fd = unsafe { BorrowedFd::borrow_raw(fd) }; unistd::tcsetpgrp(fd, Pid::from_raw(pgid)).map_err(|err| err.into_pyexception(vm)) }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Lib/test/test_os.pyis excluded by!Lib/**
📒 Files selected for processing (5)
crates/vm/src/function/fspath.rscrates/vm/src/ospath.rscrates/vm/src/stdlib/nt.rscrates/vm/src/stdlib/os.rscrates/vm/src/stdlib/posix.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/stdlib/nt.rscrates/vm/src/function/fspath.rscrates/vm/src/stdlib/posix.rscrates/vm/src/ospath.rscrates/vm/src/stdlib/os.rs
🧠 Learnings (3)
📓 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.
📚 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/posix.rscrates/vm/src/stdlib/os.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/os.rs
🧬 Code graph analysis (3)
crates/vm/src/stdlib/nt.rs (2)
crates/vm/src/ospath.rs (2)
with_filename(306-316)mode(256-261)crates/vm/src/stdlib/os.rs (1)
path(584-586)
crates/vm/src/stdlib/posix.rs (1)
crates/common/src/crt_fd.rs (2)
borrow_raw(101-106)borrow_raw(248-251)
crates/vm/src/stdlib/os.rs (1)
crates/vm/src/ospath.rs (3)
new_str(200-203)mode(256-261)new(23-29)
⏰ 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 (13)
crates/vm/src/function/fspath.rs (1)
10-11: LGTM - Documentation improvement.The added doc comment clarifies the purpose of
FsPathas a helper for implementingos.fspath().crates/vm/src/stdlib/nt.rs (2)
197-202: LGTM - Improved error handling with filename context.Using
OSErrorBuilder::with_filenameprovides better error messages by including the path in OS errors, consistent with CPython behavior.
926-931: LGTM - Consistent use of mode() method.The change from field access to method call
path.mode()aligns with the newOsPathAPI that derives output mode from the origin type.crates/vm/src/stdlib/posix.rs (1)
1860-1861: LGTM - Support functions registry updated.Adding
fpathconfandfchdirto the support functions list properly advertises the newly available functionality.crates/vm/src/ospath.rs (4)
11-44: Well-designed builder pattern for path conversion.The
PathConvertertype with its fluent API (function(),argument(),non_strict()) provides clean, CPython-compatible error message formatting. Usingconst fnfor the builder methods is a nice touch for compile-time construction.
246-261: LGTM - Origin-based mode detection.The
filename()andmode()methods correctly derive behavior from the stored origin, enabling accurate round-tripping (bytes input → bytes output, string input → string output). This matches CPython's behavior.
304-316: Helper simplifies OSError construction with filename.The
OSErrorBuilder::with_filenamehelper provides a convenient way to construct OS errors with filename context, reducing boilerplate in calling code.
76-90: Consider adding fd validation before callingtry_borrow_raw.The fd is currently passed directly to
crt_fd::Borrowed::try_borrow_raw(fd), which only validates against-1. However, other modules likeio.rsexplicitly reject all negative file descriptors (if fd < 0) before processing. While the current implementation is functional, adding an early validation check would be more defensive, especially since special negative values likeAT_FDCWD(-100) exist in the codebase and need explicit handling. This would improve consistency with other parts of the codebase.crates/vm/src/stdlib/os.rs (5)
383-386: LGTM - Output mode captured once for efficiency.Capturing
path.mode()once before the iterator loop avoids repeated method calls during iteration. The mode is then used consistently for each directory entry.
732-736: LGTM - Pickle prevention matches CPython.Adding
__reduce__that raisesTypeErrorwith "cannot pickle 'DirEntry' object" matches CPython's behavior foros.DirEntryobjects, which are not picklable.
1136-1151: Good use of PathConverter for function-specific error messages.Using
PathConverterwith.function("rename")and.argument("src"/"dst")ensures error messages match CPython's format (e.g., "rename: src should be string, bytes or os.PathLike, not int").
1729-1730: LGTM - Support functions registry updated.Adding
ftruncateandfsyncto the support functions list properly advertises fd support for these operations.
368-374:listdircorrectly handlesNoneto default to current directory, matching CPython behavior.The implementation properly handles both missing and
Nonearguments throughOptionalArg<Option<OsPathOrFd<'_>>>, defaulting to"."which matches CPython whereos.listdir()andos.listdir(None)both list the current directory.
| #[pyfunction] | ||
| fn setpgrp(vm: &VirtualMachine) -> PyResult<()> { | ||
| // setpgrp() is equivalent to setpgid(0, 0) | ||
| unistd::setpgid(Pid::from_raw(0), Pid::from_raw(0)).map_err(|err| err.into_pyexception(vm)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the setpgid function and its platform guards
rg -n "fn setpgid" crates/vm/src/stdlib/posix.rs -A5 -B5Repository: RustPython/RustPython
Length of output: 511
🏁 Script executed:
# Also check for any #[cfg] guards on setpgrp function
rg -n "fn setpgrp" crates/vm/src/stdlib/posix.rs -A5 -B5Repository: RustPython/RustPython
Length of output: 551
🏁 Script executed:
# Verify tcgetpgrp/tcsetpgrp guards as mentioned in review
rg -n "fn tcgetpgrp\|fn tcsetpgrp" crates/vm/src/stdlib/posix.rs -A3 -B3Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Search for tcgetpgrp and tcsetpgrp in the file
rg -n "tcgetpgrp\|tcsetpgrp" crates/vm/src/stdlib/posix.rs -B3 -A5Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Get more context around setpgrp to see the cfg guards applied
sed -n '1240,1260p' crates/vm/src/stdlib/posix.rsRepository: RustPython/RustPython
Length of output: 791
🏁 Script executed:
# Check if setpgid has any cfg guards elsewhere or if it's conditionally available in nix crate
rg -n "#\[cfg" crates/vm/src/stdlib/posix.rs | grep -A2 -B2 "setpgid\|setpgrp" || rg -n "#\[cfg" crates/vm/src/stdlib/posix.rs | head -20Repository: RustPython/RustPython
Length of output: 806
🏁 Script executed:
# Check all cfg guards around setpgid function
sed -n '1230,1250p' crates/vm/src/stdlib/posix.rsRepository: RustPython/RustPython
Length of output: 853
🏁 Script executed:
# Check if setpgid is available on wasi/redox by searching nix crate availability
# First, let's look at what the nix crate's setpgid function definition supports
rg -n "setpgid\|setpgrp" crates/vm/src/stdlib/posix.rs -B1Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Search for nix::unistd imports to understand platform support
rg -n "use nix::unistd" crates/vm/src/stdlib/posix.rsRepository: RustPython/RustPython
Length of output: 153
🏁 Script executed:
# Check if setpgid has any cfg guards that might be missing
rg -n "#\[pyfunction\]" crates/vm/src/stdlib/posix.rs | grep -B1 "setpgid\|setpgrp"Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Look at the nix crate's Cargo.toml to understand platform support
cat Cargo.lock | grep -A5 "name = \"nix\""Repository: RustPython/RustPython
Length of output: 491
🏁 Script executed:
# Search for any test or conditional compilation related to setpgid/setpgrp
rg -n "setpgid|setpgrp" crates/vm/src/stdlib/posix.rsRepository: RustPython/RustPython
Length of output: 634
🏁 Script executed:
# Check the full function signatures around setpgid to understand platform support
sed -n '1235,1275p' crates/vm/src/stdlib/posix.rsRepository: RustPython/RustPython
Length of output: 1601
🏁 Script executed:
# Check the nix crate's source to see if setpgid is conditionally compiled
# First let's search online documentation or check if there's a Cargo.toml with features
cat Cargo.toml | head -50Repository: RustPython/RustPython
Length of output: 1778
🏁 Script executed:
# Check if setpgid is available on all platforms or if nix crate provides it conditionally
# by looking at other platform-specific functions
rg -n "#\[cfg.*target_os" crates/vm/src/stdlib/posix.rs | head -30Repository: RustPython/RustPython
Length of output: 1653
🏁 Script executed:
# Let's check if there are any imports or re-exports that might indicate setpgid availability
rg -n "use nix" crates/vm/src/stdlib/posix.rsRepository: RustPython/RustPython
Length of output: 479
🏁 Script executed:
# Search web for nix crate setpgid platform supportRepository: RustPython/RustPython
Length of output: 47
🌐 Web query:
nix crate setpgid platform support wasi redox
💡 Result:
Short answer
- nix provides nix::unistd::setpgid (process feature) for Unix targets. [1][2]
- WASI is not a supported Unix target for the nix crate, so setpgid is not available on WASI. [2]
- Redox (x86_64-unknown-redox) has appeared as a Tier‑3 target in the nix supported-targets list, but tier‑3 support is best‑effort and many Unix process APIs (like traditional setpgid semantics) may be missing or unimplemented on Redox. Test/verify on Redox if you need setpgid behavior. [3][1]
Sources
- setpgid docs (nix::unistd::setpgid). [1]
- nix crate overview / "Available on Unix only" and crate features. [2]
- nix README / supported-targets showing x86_64-unknown-redox as Tier 3. [3]
Add platform guard to setpgrp function.
The setpgrp function uses unistd::setpgid, which is not available on WASI or Redox. The related functions setsid, tcgetpgrp, and tcsetpgrp are properly guarded with #[cfg(not(any(target_os = "wasi", target_os = "redox")))], but setpgrp lacks this guard and will cause compilation failures on unsupported platforms.
+#[cfg(not(any(target_os = "wasi", target_os = "redox")))]
#[pyfunction]
fn setpgrp(vm: &VirtualMachine) -> PyResult<()> {
// setpgrp() is equivalent to setpgid(0, 0)
unistd::setpgid(Pid::from_raw(0), Pid::from_raw(0)).map_err(|err| err.into_pyexception(vm))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[pyfunction] | |
| fn setpgrp(vm: &VirtualMachine) -> PyResult<()> { | |
| // setpgrp() is equivalent to setpgid(0, 0) | |
| unistd::setpgid(Pid::from_raw(0), Pid::from_raw(0)).map_err(|err| err.into_pyexception(vm)) | |
| } | |
| #[cfg(not(any(target_os = "wasi", target_os = "redox")))] | |
| #[pyfunction] | |
| fn setpgrp(vm: &VirtualMachine) -> PyResult<()> { | |
| // setpgrp() is equivalent to setpgid(0, 0) | |
| unistd::setpgid(Pid::from_raw(0), Pid::from_raw(0)).map_err(|err| err.into_pyexception(vm)) | |
| } |
🤖 Prompt for AI Agents
In crates/vm/src/stdlib/posix.rs around lines 1243-1247, the setpgrp function is
missing a platform guard and calls unistd::setpgid which is unavailable on WASI
and Redox; add the same compile-time attribute used by related functions
(#[cfg(not(any(target_os = "wasi", target_os = "redox")))]) immediately above
the fn setpgrp declaration so the function is excluded on those platforms,
matching the guards used for setsid, tcgetpgrp, and tcsetpgrp.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.