10000 impl preexec_fn by youknowone · Pull Request #6479 · RustPython/RustPython · GitHub
[go: up one dir, main page]
Skip to content

impl preexec_fn #6479

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
youknowone merged 1 commit into from
Dec 24, 2025
Merged

impl preexec_fn #6479

youknowone merged 1 commit into from
Dec 24, 2025

Conversation

@youknowone
Copy link
Member
@youknowone youknowone commented Dec 24, 2025 •
edited by coderabbitai bot
Loading

Summary by CodeRabbit

Bug Fixes

  • preexec_fn is now supported in subprocess execution (previously blocked)
  • Enhanced error reporting for preexec_fn failures with clearer, more descriptive error messages distinguishing subprocess-level errors from OS-level errors

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

@coderabbitai
Copy link
Contributor
coderabbitai bot commented Dec 24, 2025 •
edited
Loading

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

The changes enable preexec_fn support in subprocess execution by removing a not-implemented guard, threading the VM reference through function signatures from fork_exec to exec_inner, adding a new PreExec variant to ExecErrorContext for error distinction, and implementing preexec_fn invocation in the child process after setup but before file descriptor closing.

Changes

Cohort / File(s) Summary
preexec_fn invocation and error context
crates/stdlib/src/posixsubprocess.rs		 Removed preexec_fn guard allowing its execution; added PreExec variant to ExecErrorContext enum with corresponding message "Exception occurred in preexec_fn."; enhanced error reporting to distinguish PreExec errors (SubprocessError:0:) from OS errors (OSError:<errno_hex>:)
Function signature updates for VM threading
crates/stdlib/src/posixsubprocess.rs		 Updated exec() signature to accept vm: &VirtualMachine parameter; updated exec_inner() signature to accept vm: &VirtualMachine parameter; updated call site in fork_exec() to pass vm to exec() and all exec_inner() call sites to forward the VM reference
preexec_fn execution logic
crates/stdlib/src/posixsubprocess.rs		 Implemented preexec_fn invocation in child process after setup steps but before file descriptor closing; on failure, sets error context to PreExec and returns UnknownErrno; on success, execution continues to execve/execv

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A preexec hop through code we go,
With VMs passed where children flow,
Callbacks bloom before execve's call,
Error messages catch one and all—
Subprocess magic, clean and bright!

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📜 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 309b2ad and c7fcd56.

⛔ Files ignored due to path filters (1)
  • Lib/test/test_subprocess.py is excluded by !Lib/**
📒 Files selected for processing (1)
  • crates/stdlib/src/posixsubprocess.rs

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.

@youknowone youknowone marked this pull request as ready for review December 24, 2025 07:22
@youknowone youknowone merged commit 4f0b940 into RustPython:main Dec 24, 2025
12 of 13 checks passed
@youknowone youknowone deleted the preexec_fn branch December 24, 2025 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@youknowone
0