Skip to content

Conversation

@youknowone
Copy link
Member

@youknowone youknowone commented Dec 8, 2025

Summary by CodeRabbit

  • New Features
    • Add file-descriptor duplication, including duplicating onto a target descriptor.
    • Allow callers to query whether a descriptor is inheritable.
    • Provide an option to control/clear the inheritable flag on newly duplicated descriptors (default remains inheritable).
    • Improve error handling and cleanup when duplication or inheritable setting fails.

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 8, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

Adds Windows CRT-backed file-descriptor operations to the NT stdlib: get_inheritable(fd), dup(fd), and dup2(fd, fd2, inheritable), plus FFI declarations for _dup and _dup2, a Dup2Args struct, and helper logic to set/clear descriptor inheritable state and map errors to Python exceptions.

Changes

Cohort / File(s) Summary
NT module — FD duplication & inheritance
crates/vm/src/stdlib/nt.rs
Added get_inheritable(fd: i32, vm: &VirtualMachine) -> PyResult<bool>; declared extern C _dup(fd: i32) -> i32 and _dup2(fd: i32, fd2: i32) -> i32; added dup(fd) that duplicates an FD and marks the new descriptor non-inheritable; added Dup2Args struct and dup2(args: Dup2Args) to duplicate onto a target FD with optional inheritable control; implemented close_fd_and_raise helper and used raw_set_handle_inheritable for setting inheritability; exported new functions and struct.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Single-file change with cohesive additions.
  • Review focus:
    • FFI signatures for _dup / _dup2
    • Correct handle conversion and raw_set_handle_inheritable usage
    • Error handling and resource cleanup in close_fd_and_raise
    • Dup2Args pyarg defaults and parsing

Possibly related PRs

Poem

🐰 I hopped through bytes with a whiskered grin,

I cloned a handle on a midnight spin.
dup and dup2, a neat little trick,
inheritable flags set — click by click.
Carrots for code, and a floppy FD tin.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and concisely identifies the main additions: get_inheritable and dup functions for Windows platform support.
Docstring Coverage ✅ Passed Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%.

📜 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 334ef6c and 388bf2b.

⛔ Files ignored due to path filters (2)
  • Lib/test/test_builtin.py is excluded by !Lib/**
  • Lib/test/test_os.py is excluded by !Lib/**
📒 Files selected for processing (1)
  • crates/vm/src/stdlib/nt.rs (4 hunks)

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 8, 2025 13:00
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 / _dup2 looks correct but confirm target coverage

The signatures for _dup and _dup2 match 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-gnu vs windows-msvc), or gate them with cfg if necessary.

If needed, please double‑check your supported Windows targets’ CRTs expose _dup / _dup2 under these names.

📜 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 7250b1f and b936588.

⛔ Files ignored due to path filters (1)
  • Lib/test/test_os.py is 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 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
🧬 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-based get_inheritable wiring looks good

This cleanly reuses crt_fd::as_handle and the existing get_handle_inheritable helper, with correct error propagation to Python. No issues from a correctness or API-compat standpoint.

@youknowone youknowone merged commit 4592787 into RustPython:main Dec 8, 2025
12 of 13 checks passed
@youknowone youknowone deleted the windows-dup branch December 8, 2025 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant