impl os functions #6484

youknowone · 2025-12-24T10:54:51Z

Summary by CodeRabbit

New Features
- Added process group management functions for POSIX systems: setpgrp, tcgetpgrp, and tcsetpgrp for improved process control.
Improvements
- Enhanced error messages for filesystem operations with contextual information including function and argument names.
- Refined path handling and conversion logic for improved reliability.
- Directory and file iteration operations now properly detect and report serialization attempts.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-12-24T10:54:57Z

📝 Walkthrough

Walkthrough

This pull request introduces a new PathConverter type for flexible path argument handling with configurable error messaging, refactors OsPath to track the origin of path objects for accurate round-tripping, updates path resolution logic across the VM's filesystem operations, and adds POSIX process group management functions (setpgrp, tcgetpgrp, tcsetpgrp).

Changes

Cohort / File(s)	Summary
Path conversion infrastructure `crates/vm/src/function/fspath.rs`	Added documentation comment for `FsPath` as a helper for `os.fspath()` implementation and minor comment updates.
Core path handling refactor `crates/vm/src/ospath.rs`	Introduced public `PathConverter` type with fluent configuration API for function/argument names and strict mode. Refactored `OsPath` to carry an optional `origin` field (preserving original Python object) instead of simple mode tracking. Changed `OutputMode` from private to public enum. Updated path resolution to route through `PathConverter` for both FD and path-like handling.
Windows path operations `crates/vm/src/stdlib/nt.rs`	Replaced direct error handling with `OSErrorBuilder::with_filename()` for symlink metadata and permission changes. Updated method calls from field access `path.mode` to method call `path.mode()`. Standardized path processing across `_getfinalpathname`, `_getfullpathname`, and `_getvolumepathname`. Minor typo fix.
OS module updates `crates/vm/src/stdlib/os.rs`	Added `PathConverter` import. Changed `listdir` signature to accept `OptionalArg<Option<OsPathOrFd<'_>>>` with flattened defaults. Updated `rename` to accept `PyObjectRef` arguments with `PathConverter`-based conversion. Added `__reduce__` methods for `DirEntry`, `ScandirIterator`, and `ScandirIter` to prevent pickling. Standardized mode retrieval via `path.mode()` method across directory iteration. Extended module support surface to advertise `ftruncate` and `fsync`.
POSIX process group functions `crates/vm/src/stdlib/posix.rs`	Added three new exported functions: `setpgrp()`, `tcgetpgrp(fd)`, and `tcsetpgrp(fd, pgid)` for process group management (guarded by OS availability checks). Updated support functions list to include `fpathconf` and `fchdir`.

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

Rework crt_fd to be more aligned with io-safety #5789: Directly modifies ospath.rs with changes to OsPath/OsPathOrFd structure and error handling that establish the foundation for this PR's refactoring.
nt junction #6407: Updates Windows-specific path handling in nt.rs and modifies OsPath-related logic, overlapping with this PR's path operation standardization.
fix scandir/lstat for windows #6357: Modifies scandir/DirEntry and OS module path handling, directly intersecting with this PR's directory iteration mode retrieval changes.

Suggested reviewers

ShaharNaveh
coolreader18
arihant2math

Poem

🐰 Hops with glee through paths most fine,
PathConverter makes them align,
Origin preserved, modes so bright,
POSIX groups now set just right,
Process dancing in the night! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'impl os functions' is vague and generic, using non-descriptive terms that fail to convey meaningful information about the specific changes made across multiple files.	Provide a more descriptive title that highlights the primary changes, such as 'Add PathConverter for path handling and implement posix/os functions' or 'Refactor path conversion and add posix functions (setpgrp, tcgetpgrp, tcsetpgrp)'.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	Docstring coverage is 84.09% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

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). Since path is 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 point
Note: The current code is correct since as_ref() borrows, but keeping path alive 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 to tcsetpgrp at 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

📥 Commits

Reviewing files that changed from the base of the PR and between cbde5ce and fc63eef.

⛔ Files ignored due to path filters (1)

Lib/test/test_os.py is excluded by !Lib/**

📒 Files selected for processing (5)

crates/vm/src/function/fspath.rs
crates/vm/src/ospath.rs
crates/vm/src/stdlib/nt.rs
crates/vm/src/stdlib/os.rs
crates/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 running cargo fmt to 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.rs
crates/vm/src/function/fspath.rs
crates/vm/src/stdlib/posix.rs
crates/vm/src/ospath.rs
crates/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.rs
crates/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 FsPath as a helper for implementing os.fspath().

crates/vm/src/stdlib/nt.rs (2)

197-202: LGTM - Improved error handling with filename context.

Using OSErrorBuilder::with_filename provides 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 new OsPath API that derives output mode from the origin type.

crates/vm/src/stdlib/posix.rs (1)

1860-1861: LGTM - Support functions registry updated.

Adding fpathconf and fchdir to 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 PathConverter type with its fluent API (function(), argument(), non_strict()) provides clean, CPython-compatible error message formatting. Using const fn for the builder methods is a nice touch for compile-time construction.

246-261: LGTM - Origin-based mode detection.

The filename() and mode() 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_filename helper provides a convenient way to construct OS errors with filename context, reducing boilerplate in calling code.

76-90: Consider adding fd validation before calling try_borrow_raw.

The fd is currently passed directly to crt_fd::Borrowed::try_borrow_raw(fd), which only validates against -1. However, other modules like io.rs explicitly 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 like AT_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 raises TypeError with "cannot pickle 'DirEntry' object" matches CPython's behavior for os.DirEntry objects, which are not picklable.

1136-1151: Good use of PathConverter for function-specific error messages.

Using PathConverter with .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 ftruncate and fsync to the support functions list properly advertises fd support for these operations.

368-374: listdir correctly handles None to default to current directory, matching CPython behavior.

The implementation properly handles both missing and None arguments through OptionalArg<Option<OsPathOrFd<'_>>>, defaulting to "." which matches CPython where os.listdir() and os.listdir(None) both list the current directory.

coderabbitai · 2025-12-25T00:16:35Z

crates/vm/src/stdlib/posix.rs

+    #[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))
+    }


⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# Find the setpgid function and its platform guards rg -n "fn setpgid" crates/vm/src/stdlib/posix.rs -A5 -B5

Repository: 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 -B5

Repository: 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 -B3

Repository: 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 -A5

Repository: 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.rs

Repository: 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 -20

Repository: RustPython/RustPython

Length of output: 806

🏁 Script executed:

# Check all cfg guards around setpgid function sed -n '1230,1250p' crates/vm/src/stdlib/posix.rs

Repository: 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 -B1

Repository: 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.rs

Repository: 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.rs

Repository: 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.rs

Repository: 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 -50

Repository: 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 -30

Repository: 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.rs

Repository: RustPython/RustPython

Length of output: 479

🏁 Script executed:

# Search web for nix crate setpgid platform support

Repository: 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.

Suggested change

#[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.

youknowone force-pushed the os branch from 36bbf14 to 8e0fe5a Compare December 24, 2025 13:44

youknowone mentioned this pull request Dec 24, 2025

subprocess.SubprocessError: Exception occurred in preexec_fn. #6489

Closed

youknowone force-pushed the os branch from 8e0fe5a to 4c2ac70 Compare December 24, 2025 13:53

youknowone added 4 commits December 25, 2025 08:06

os.setpgrp

18f0f2b

tcgetpgrp

b74f657

impl more os functions

75774d1

impl PathConverter

fd18784

youknowone force-pushed the os branch from 4c2ac70 to fd18784 Compare December 24, 2025 23:06

fix windows error

fc63eef

youknowone marked this pull request as ready for review December 25, 2025 00:12

coderabbitai bot reviewed Dec 25, 2025

View reviewed changes

youknowone merged commit 7ebb0f0 into RustPython:main Dec 25, 2025
13 checks passed

youknowone deleted the os branch December 25, 2025 00:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

impl os functions #6484

impl os functions #6484

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Summary by CodeRabbit

Uh oh!

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Development

Uh oh!