Skip to content

Fix redox compilation in stdlib #6298

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
Nov 27, 2025
Merged

Fix redox compilation in stdlib #6298

youknowone merged 1 commit into from
Nov 27, 2025
+2 −3

Conversation

@willnode
Copy link
Contributor

@willnode willnode commented Nov 27, 2025 •
edited by coderabbitai bot
Loading

This fixes compilation on Redox due to cfg filter in this code, and fcntl::open here returns OwnedFd already.

This upstream version can't be compiled to Redox due to other reasons, so as a workaround this patch fix is verified in Redox build system using a backported version.

Summary by CodeRabbit

  • Refactor
    • Improved internal file descriptor handling for enhanced code maintainability. No end-user facing changes.

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 27, 2025 •
edited
Loading

Walkthrough

The close_filetable_fds function in the redox target was refactored to use OwnedFd instead of raw file descriptors. The opened filetable is now stored as an owned wrapper, with the read operation updated to pass the reference directly rather than extracting the raw fd via as_raw_fd().

Changes

Cohort / File(s) Summary
File descriptor ownership refactoring
crates/stdlib/src/posixsubprocess.rs		 Updated close_filetable_fds to use OwnedFd wrapper instead of raw fd for filetable operations; read call modified to accept OwnedFd reference directly; fd comparisons updated to call as_raw_fd() as needed

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • Single file change with straightforward refactoring
  • Ownership pattern improvement from raw to RAII-based wrapper
  • No logic flow alterations, only resource management improvements

Suggested reviewers

  • moreal
  • youknowone

Poem

🐰 A file descriptor now safely owned,
No longer bare, but wrapped and grown!
From raw to RAII, the pattern's clear,
Resource safety brings us cheer! ✨

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 'Fix redox compilation in stdlib' directly relates to the main change: adapting code to fix Redox compilation issues by properly handling OwnedFd from fcntl::open.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% 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
📜 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 1e7a490 and 6f88fac.

📒 Files selected for processing (1)
  • crates/stdlib/src/posixsubprocess.rs (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style in Rust code (cargo fmt to format)
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by your changes
Follow Rust best practices for error handling and memory management

Files:

  • crates/stdlib/src/posixsubprocess.rs
**/src/**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust

Files:

  • crates/stdlib/src/posixsubprocess.rs
🧬 Code graph analysis (1)
crates/stdlib/src/posixsubprocess.rs (1)
crates/stdlib/src/fcntl.rs (1)
  • fcntl (59-90)
⏰ 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). (11)
  • GitHub Check: Run snippets and cpython tests (windows-latest)
  • GitHub Check: Run tests under miri
  • GitHub Check: Run snippets and cpython tests (macos-latest)
  • GitHub Check: Run snippets and cpython tests (ubuntu-latest)
  • GitHub Check: Check Rust code with clippy
  • GitHub Check: Check the WASM package and demo
  • GitHub Check: Run snippets and cpython tests on wasm-wasi
  • GitHub Check: Run rust tests (macos-latest)
  • GitHub Check: Run rust tests (windows-latest)
  • GitHub Check: Run rust tests (ubuntu-latest)
  • GitHub Check: Ensure compilation on various targets
🔇 Additional comments (1)
crates/stdlib/src/posixsubprocess.rs (1)

444-452: OwnedFd-based filetable handling and read call look correct

Binding filetable directly to the result of fcntl::open and using RAII for closing, plus calling nix::unistd::read(&filetable, ...), is consistent with the newer nix APIs where open returns an OwnedFd and read accepts an AsFd-based argument. The later comparison against filetable.as_raw_fd() still prevents accidentally closing the filetable descriptor itself, so behavior is preserved while fixing the Redox compilation issue.

You may want to rely on cargo clippy to confirm there are no new lints (e.g., unused imports in this block) on the updated nix/Redox toolchain. As per coding guidelines, please ensure clippy is clean for this change.

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

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.

ShaharNaveh
ShaharNaveh approved these changes Nov 27, 2025
View reviewed changes
Copy link
Collaborator

@ShaharNaveh ShaharNaveh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM.
cc @youknowone

youknowone
youknowone approved these changes Nov 27, 2025
View reviewed changes
Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@youknowone youknowone merged commit 0e6e256 into RustPython:main Nov 27, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@youknowone youknowone youknowone approved these changes

+1 more reviewer

@ShaharNaveh ShaharNaveh ShaharNaveh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@willnode @youknowone @ShaharNaveh