Fix subprocess and Update subprocess from Python 3.13.11 by youknowone · Pull Request #6583 · RustPython/RustPython

youknowone · 2025-12-29T15:42:29Z

Summary by CodeRabbit

New Features
- Added runtime warning when line buffering is requested in binary mode.
- Added new Windows process feedback control constants for subprocess management.
Improvements
- Enhanced subprocess signal handling and umask configuration for more robust process creation.
- Improved environment variable handling in POSIX spawn operations with fallback to current process environment.

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

coderabbitai · 2025-12-29T15:42:34Z

📝 Walkthrough

Walkthrough

This PR implements missing functionality in subprocess pre-exec setup (umask and signal restoration), makes POSIX spawn environment optional with automatic inheritance, adds a runtime buffering warning for edge cases, and expands Windows threading constants.

Changes

Cohort / File(s)	Summary
Pre-exec subprocess setup `crates/stdlib/src/posixsubprocess.rs`	Replaces TODOs with runtime actions: sets process umask via `libc::umask` when `child_umask >= 0`; restores SIGPIPE and SIGXFSZ to default handlers via `libc::signal` when `restore_signals` is true.
POSIX spawn environment `crates/vm/src/stdlib/posix.rs`	Changes `PosixSpawnArgs.env` from mandatory to `Option<ArgMapping>`. When env is provided, uses existing `envp_from_dict` logic. When None, derives environment from current process via `env::vars_os` with null-byte validation, falling back to automatic inheritance.
I/O buffering validation `crates/vm/src/stdlib/io.rs`	Adds runtime warning when line buffering (buffering=1) is requested in binary mode, emitted during `io_open` after `isatty` check. No control-flow impact on subsequent buffering behavior.
Windows threading constants `crates/vm/src/stdlib/winapi.rs`	Adds public constants `STARTF_FORCEOFFFEEDBACK` and `STARTF_FORCEONFEEDBACK` to Windows Threading imports, repositioning existing STARTF constants.

Sequence Diagram(s)

sequenceDiagram
    participant Child as Child Process Setup
    participant Umask as libc::umask
    participant Signal as libc::signal
    participant Exec as exec/setsid/pgid
    
    Child->>Child: Check child_umask
    alt child_umask >= 0
        Child->>Umask: Set process umask
        Umask-->>Child: umask applied
    end
    
    Child->>Child: Check restore_signals
    alt restore_signals == true
        Child->>Signal: Restore SIGPIPE to default
        Signal-->>Child: Handler set
        Child->>Signal: Restore SIGXFSZ to default
        Signal-->>Child: Handler set
    end
    
    Child->>Exec: Proceed with exec/setsid/pgid

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Fix posix tests #5811: Modifies POSIX spawn environment handling in posix.rs, including env field construction and envp conversion logic.
impl preexec_fn #6479: Implements pre-exec setup enhancements in posixsubprocess.rs, including preexec_fn handling alongside umask and signal restoration.
windows umask, win32_xstat_slow_impl, fake EXT_SUFFIX #6340: Adds umask-related runtime behavior (Windows _umask wrapper), complementing the libc umask calls in this PR.

Suggested reviewers

ShaharNaveh

Poem

🐰 Whiskers twitch with POSIX glee,
Umasks set and signals free,
Environments now born anew—
From parent's breath, the child's debut.
Windows constants dance in place,
A buffering warning, just in case! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title references subprocess fixes and updates from Python 3.13.11, which aligns with the actual changes affecting subprocess functionality across multiple files (posixsubprocess.rs, posix.rs, and updates to Windows constants).

✨ Finishing touches

📝 Generate docstrings

📜 Recent 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 2f4ca50 and 5c5cacc.

⛔ Files ignored due to path filters (3)

Lib/subprocess.py is excluded by !Lib/**
Lib/test/test_file.py is excluded by !Lib/**
Lib/test/test_subprocess.py is excluded by !Lib/**

📒 Files selected for processing (4)

crates/stdlib/src/posixsubprocess.rs
crates/vm/src/stdlib/io.rs
crates/vm/src/stdlib/posix.rs
crates/vm/src/stdlib/winapi.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 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/io.rs
crates/vm/src/stdlib/posix.rs
crates/stdlib/src/posixsubprocess.rs
crates/vm/src/stdlib/winapi.rs

🧬 Code graph analysis (1)

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

crates/vm/src/warn.rs (1)

warn (83-94)

🔇 Additional comments (5)

crates/stdlib/src/posixsubprocess.rs (1)

323-331: Child umask and signal restoration logic looks correct

Applying libc::umask only when child_umask >= 0 and resetting SIGPIPE/SIGXFSZ to SIG_DFL under restore_signals in the child pre-exec path matches expected POSIX subprocess semantics and is safe to do here; ignoring the return values is acceptable for these calls.

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

4492-4503: RuntimeWarning on buffering=1 in binary mode is well-placed

The added warning for buffering == 1 with EncodeMode::Bytes correctly highlights that line buffering isn’t meaningful in binary mode, while leaving the existing behavior (fallback to DEFAULT_BUFFER_SIZE and unchanged line_buffering semantics) intact.

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

62-68: Additional STARTF_ threading constants are correctly exposed*

Importing STARTF_FORCEOFFFEEDBACK and STARTF_FORCEONFEEDBACK alongside the existing Threading constants under #[pyattr] cleanly extends the _winapi surface without affecting existing behavior.

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

1518-1518: LGTM: Optional environment parameter.

Making the env parameter optional is the correct approach for allowing subprocess spawning to inherit the current environment when None is passed, which aligns with Python's subprocess module behavior.

1681-1696: Well-implemented environment inheritance.

The logic correctly handles env=None by inheriting the current process environment via env::vars_os(), which matches Python's subprocess module behavior. The null-byte validation and error handling are appropriate.

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.}

github-actions · 2025-12-29T16:09:14Z

Code has been automatically formatted

The code in this PR has been formatted using cargo fmt --all.
Please pull the latest changes before pushing again:

git pull origin subprocess

youknowone added 4 commits December 30, 2025 00:39

fix test_subprocess

88e7760

fix posixsubprocess

6f52a0b

fix io warning

b339768

Update subprocess from Python 3.13.11

0988633

fix

ffcda01

youknowone force-pushed the subprocess branch from b5f2c99 to ffcda01 Compare December 29, 2025 16:43

github-actions bot and others added 2 commits December 29, 2025 16:43

Auto-format: cargo fmt --all

2b2236c

fix macos

5c5cacc

youknowone marked this pull request as ready for review December 30, 2025 03:09

youknowone merged commit 489289f into RustPython:main Dec 30, 2025
13 checks passed

youknowone deleted the subprocess branch December 30, 2025 03:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix subprocess and Update subprocess from Python 3.13.11#6583

Fix subprocess and Update subprocess from Python 3.13.11#6583
youknowone merged 7 commits intoRustPython:mainfrom
youknowone:subprocess

youknowone commented Dec 29, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Dec 29, 2025 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Uh oh!

github-actions bot commented Dec 29, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

youknowone commented Dec 29, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Dec 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

github-actions bot commented Dec 29, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

youknowone commented Dec 29, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 29, 2025 •

edited

Loading