GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — os.waitstatus_to_exitcode for windows by youknowone · Pull Request #6355 · RustPython/RustPython

Conversation

youknowone commented Dec 8, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

Summary by CodeRabbit

Refactor
- Reworked OS process-exit handling to use distinct, platform-specific implementations for Unix and Windows, improving cross-platform correctness and maintainability.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          fix waitstatus_to_exitcode

c5b2b41

coderabbitai Bot commented Dec 8, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Caution

Review failed

The pull request is closed.

Walkthrough

Refactors waitstatus_to_exitcode into platform-specific implementations: a Unix variant under #[cfg(unix)] using i32 and WIFEXITED/WEXITSTATUS/WIFSIGNALED, and a new Windows variant accepting u64 that extracts the exit code via bit-shifting and validates it as u32. (47 words)

Changes

Cohort / File(s) Change Summary

Platform-specific waitstatus_to_exitcode refactor
crates/vm/src/stdlib/os.rs Split the previous cross-platform function into two platform-guarded implementations: #[cfg(unix)] function uses status: i32 with WIFEXITED/WIFEXITSTATUS/WIFSIGNALED logic returning PyResult<i32>; a Windows-only function uses status: u64, extracts the exit code via status >> 8, validates it fits in u32, and returns PyResult<u32>. Removed prior combined cfg_if Windows handling from the Unix-guarded path.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Single-file, focused platform split with small type/signature changes.
Pay attention to:
- Call sites expecting the old signature/type.
- Correct cfg attributes and exported visibility.
- Consistent error handling and return types between platforms.

Possibly related PRs

os.waitstatus_to_exitcode for windows #6355: Modifies the same waitstatus_to_exitcode function and splits Unix/Windows implementations — directly related.
Windows execv, spawnv, wait #6350: Changes wait/waitpid behavior to return u64, aligning with the new Windows signature that accepts u64.

Poem

🐰
Hops through code on nimble feet,
Unix paths and Windows meet,
Bits shift, macros softly sing,
Each platform gets its proper wing,
A carrot cheer for builds complete 🥕

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.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check	✅ Passed	The title 'os.waitstatus_to_exitcode for windows' accurately describes the main change: adding Windows support to the os.waitstatus_to_exitcode function by implementing a Windows-specific variant.

📜 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 c5b2b41 and f22bac7.

📒 Files selected for processing (1)

crates/vm/src/stdlib/os.rs (1 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.}

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

youknowone marked this pull request as ready for review

December 8, 2025 17:37

coderabbitai Bot reviewed

Dec 8, 2025

View reviewed changes

coderabbitai Bot left a comment

Copy link

Copy Markdown

Contributor

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: 0

🧹 Nitpick comments (1)

crates/vm/src/stdlib/os.rs (1)
1442-1450: LGTM! Windows implementation correctly extracts exit code.

The >> 8 shift and u32 validation match CPython's Windows behavior. The distinct signature (u64 input, u32 output) appropriately reflects Windows semantics where exit codes are always non-negative.

Nitpick: Minor inconsistency in error message capitalization—line 1439 uses "Invalid" while line 1449 uses "invalid". Consider aligning for consistency.
-        .map_err(|_| vm.new_value_error(format!("invalid exit code: {exitcode}")))
+        .map_err(|_| vm.new_value_error(format!("Invalid exit code: {exitcode}")))

📜 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 a3d638a and c5b2b41.

⛔ 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/os.rs (1 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/os.rs

🧠 Learnings (2)

📓 Common learnings

Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.

Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.

📚 Learning: 2025-06-27T14:47:28.810Z

Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.

Applied to files:

crates/vm/src/stdlib/os.rs

🔇 Additional comments (1)

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

1424-1440: LGTM! Unix implementation correctly handles wait status.

The implementation properly uses libc macros (WIFEXITED/WEXITSTATUS/WIFSIGNALED/WTERMSIG) and correctly returns negative values for signal-terminated processes. The validation flow (i32 → u32 → c_int) ensures non-negative input while maintaining compatibility with libc macro expectations.

Based on learnings, this follows the recommended pattern of platform-specific conditional compilation.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

youknowone changed the title ~~fix waitstatus_to_exitcode~~ os.waitstatus_to_exitcode for windows

Dec 8, 2025

msg

f22bac7

youknowone merged commit abc5c22 into RustPython:main

Dec 8, 2025

12 of 13 checks passed

Uh oh!

There was an error while loading. Please reload this page.

youknowone deleted the waitstatus_to_exitcode branch

December 8, 2025 17:46

coderabbitai Bot mentioned this pull request

Dec 12, 2025

Fix test_runpy #6409

Merged

Reviewers

coderabbitai[bot]

coderabbitai[bot] left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

1 participant

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

youknowone commented Dec 8, 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 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

youknowone commented Dec 8, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

coderabbitai Bot commented Dec 8, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.