-
Notifications
You must be signed in to change notification settings - Fork 1.4k
get_inheritable, dup for windows #6343
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.
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
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds Windows CRT-backed file-descriptor operations to the NT stdlib: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/vm/src/stdlib/nt.rs (1)
485-489: FFI for_dup/_dup2looks correct but confirm target coverageThe signatures for
_dupand_dup2match the MSVC CRT prototypes and fit how you use them below (return value checked for< 0). Just make sure these symbols are available for all Windows targets you support (e.g.windows-gnuvswindows-msvc), or gate them withcfgif necessary.If needed, please double‑check your supported Windows targets’ CRTs expose
_dup/_dup2under these names.
📜 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 (1)
crates/vm/src/stdlib/nt.rs(3 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/vm/src/stdlib/nt.rs
🧬 Code graph analysis (1)
crates/vm/src/stdlib/nt.rs (3)
crates/vm/src/stdlib/posix.rs (4)
get_inheritable(947-953)dup(1769-1774)dup2(1787-1795)args(1632-1639)crates/common/src/crt_fd.rs (1)
as_handle(341-345)crates/vm/src/stdlib/os.rs (1)
errno_err(39-41)
⏰ 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). (9)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run tests under miri
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
🔇 Additional comments (1)
crates/vm/src/stdlib/nt.rs (1)
394-399: FD-basedget_inheritablewiring looks goodThis cleanly reuses
crt_fd::as_handleand the existingget_handle_inheritablehelper, with correct error propagation to Python. No issues from a correctness or API-compat standpoint.
b936588 to
334ef6c
Compare
334ef6c to
388bf2b
Compare
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.