GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — A few windows fix by youknowone · Pull Request #7458 · RustPython/RustPython Conversation
|
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [ ] test: cpython/Lib/test/test_mmap.py (TODO: 1) dependencies: dependent tests: (2 tests)
[x] lib: cpython/Lib/subprocess.py dependencies:
dependent tests: (81 tests)
[x] test: cpython/Lib/test/test_sys.py (TODO: 7) dependencies: dependent tests: (225 tests)
Legend:
|
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
coderabbitai
Bot
commented
Mar 19, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
📝 WalkthroughWalkthroughThis pull request refines Windows-specific standard library implementations across four files: adjusts exception types in memory mapping operations, fixes environment block serialization for empty mappings, removes deprecated WinSock constants, and prevents direct instantiation of the Windows version struct. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
aaf925a to
68a394f
Compare
Add slot_new to PyWindowsVersion that raises TypeError, matching sys.flags behavior.
WSAHOS was hardcoded as an alias for WSAHOST_NOT_FOUND, but CPython guards it with #ifdef WSAHOS which doesn't exist in modern Windows SDK headers.
Empty env dict produced a single null terminator, but CreateProcessW requires a double null for a valid empty environment block.
mmap resize raises SystemError (not OSError) when mremap is unavailable, matching CPython behavior. test_mmap catches SystemError to skip unsupported resize operations.
…Failure Named mmap resize on Windows should raise OSError (not SystemError). Remove expectedFailure mark from TimeEINTRTest.test_sleep as it now passes.
test_sleep passes on macOS but fails on Linux due to timing.
test_sleep now passes on all platforms.
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/vm/src/stdlib/_winapi.rs (1)
425-429:⚠️ Potential issue | 🔴 CriticalFix UTF-8 slicing on environment variable names to prevent panics.
Line 428 uses byte-level slicing
k[1..]on a UTF-8&str. When an environment variable name starts with a multi-byte character (e.g.,Ä=value), Rust panics because the slice index 1 lands in the middle of the multi-byte sequence rather than at a valid UTF-8 boundary. This crashes the VM instead of raisingValueError.Suggested fix
- if k.is_empty() || k[1..].contains('=') { + if k.is_empty() || k.chars().skip(1).any(|ch| ch == '=') { return Err(vm.new_value_error("illegal environment variable name")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/stdlib/_winapi.rs` around lines 425 - 429, The code currently slices the UTF-8 string k with k[1..], which can panic for multi-byte characters; replace the byte-slice check with a char-aware check such as using k.chars().skip(1).any(|c| c == '=') so the condition becomes if k.is_empty() || k.chars().skip(1).any(|c| c == '=') { return Err(vm.new_value_error("illegal environment variable name")); } to avoid UTF-8 boundary panics while keeping the same semantics for k and v validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/stdlib/errno.rs`:
- Around line 42-46: The errno module is missing the Windows constant
WSAHOST_NOT_FOUND in its exported WSA error list; update the export list in
crates/vm/src/stdlib/errno.rs by replacing the placeholder/TODO (WSAHOS) with
the full constant name WSAHOST_NOT_FOUND so the module exposes that constant
(used by crates/stdlib/src/socket.rs for EAI mapping).
---
Outside diff comments:
In `@crates/vm/src/stdlib/_winapi.rs`:
- Around line 425-429: The code currently slices the UTF-8 string k with k[1..],
which can panic for multi-byte characters; replace the byte-slice check with a
char-aware check such as using k.chars().skip(1).any(|c| c == '=') so the
condition becomes if k.is_empty() || k.chars().skip(1).any(|c| c == '=') {
return Err(vm.new_value_error("illegal environment variable name")); } to avoid
UTF-8 boundary panics while keeping the same semantics for k and v validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 6d320c3e-6392-41ac-9255-594c41d0ab3e
⛔ Files ignored due to path filters (3)
Lib/test/test_mmap.pyis excluded by!Lib/**Lib/test/test_subprocess.pyis excluded by!Lib/**Lib/test/test_sys.pyis excluded by!Lib/**
📒 Files selected for processing (4)
crates/stdlib/src/mmap.rscrates/vm/src/stdlib/_winapi.rscrates/vm/src/stdlib/errno.rscrates/vm/src/stdlib/sys.rs
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| WSAESTALE, WSAETIMEDOUT, WSAETOOMANYREFS, WSAEUSERS, WSAEWOULDBLOCK, WSAID_ACCEPTEX, | ||
| WSAID_CONNECTEX, WSAID_DISCONNECTEX, WSAID_GETACCEPTEXSOCKADDRS, WSAID_TRANSMITFILE, | ||
| WSAID_TRANSMITPACKETS, WSAID_WSAPOLL, WSAID_WSARECVMSG, WSANO_DATA, WSANO_RECOVERY, | ||
| WSANOTINITIALISED, WSAPROTOCOL_LEN, WSASERVICE_NOT_FOUND, WSASYS_STATUS_LEN, | ||
| WSASYSCALLFAILURE, WSASYSNOTREADY, WSATRY_AGAIN, WSATYPE_NOT_FOUND, WSAVERNOTSUPPORTED, |
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.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the errno.rs file at the specified lines to see the current state
wc -l crates/vm/src/stdlib/errno.rsRepository: RustPython/RustPython
Length of output: 98
🏁 Script executed:
# Check what's at lines 42-46 in errno.rs sed -n '40,50p' crates/vm/src/stdlib/errno.rs
Repository: RustPython/RustPython
Length of output: 785
🏁 Script executed:
# Check what's at line 566 in errno.rs sed -n '560,575p' crates/vm/src/stdlib/errno.rs
Repository: RustPython/RustPython
Length of output: 665
🏁 Script executed:
# Search for WSAHOST_NOT_FOUND in the codebase rg "WSAHOST_NOT_FOUND" --type rust
Repository: RustPython/RustPython
Length of output: 170
🏁 Script executed:
# Check socket.rs around lines 85-95 for the claimed usage sed -n '80,100p' crates/stdlib/src/socket.rs
Repository: RustPython/RustPython
Length of output: 1371
🏁 Script executed:
# Check if WSAHOS is defined or referenced elsewhere rg "WSAHOS" --type rust -B2 -A2
Repository: RustPython/RustPython
Length of output: 888
🏁 Script executed:
# Check broader context around the TODO at line 566 sed -n '560,575p' crates/vm/src/stdlib/errno.rs
Repository: RustPython/RustPython
Length of output: 665
🏁 Script executed:
# Look at the import statements at the top of errno.rs to understand the Windows socket setup sed -n '1,50p' crates/vm/src/stdlib/errno.rs | head -40
Repository: RustPython/RustPython
Length of output: 1695
🏁 Script executed:
# Check if there are any comments explaining why WSAHOST_NOT_FOUND was removed rg -B5 -A5 "WSAHOST_NOT_FOUND|WSAHOS" crates/vm/src/stdlib/errno.rs
Repository: RustPython/RustPython
Length of output: 468
Add WSAHOST_NOT_FOUND to the errno module's Windows error exports.
errno no longer exposes WSAHOST_NOT_FOUND at the module level, even though it's a standard Winsock error code used in crates/stdlib/src/socket.rs for EAI mapping. While socket.rs imports this constant directly from windows_sys, the omission leaves the errno module incomplete on Windows. Replace the WSAHOS TODO comment with the correct full constant name.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm/src/stdlib/errno.rs` around lines 42 - 46, The errno module is
missing the Windows constant WSAHOST_NOT_FOUND in its exported WSA error list;
update the export list in crates/vm/src/stdlib/errno.rs by replacing the
placeholder/TODO (WSAHOS) with the full constant name WSAHOST_NOT_FOUND so the
module exposes that constant (used by crates/stdlib/src/socket.rs for EAI
mapping).
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
* Disallow instantiation of sys.getwindowsversion type Add slot_new to PyWindowsVersion that raises TypeError, matching sys.flags behavior. * Remove incorrect WSAHOS errno constant WSAHOS was hardcoded as an alias for WSAHOST_NOT_FOUND, but CPython guards it with #ifdef WSAHOS which doesn't exist in modern Windows SDK headers. * Fix mmap resize to raise OSError instead of SystemError * Fix CreateProcess with empty environment on Windows Empty env dict produced a single null terminator, but CreateProcessW requires a double null for a valid empty environment block. * Revert mmap resize error to SystemError and fix errno.rs formatting mmap resize raises SystemError (not OSError) when mremap is unavailable, matching CPython behavior. test_mmap catches SystemError to skip unsupported resize operations. * Fix named mmap resize to raise OSError and unmark test_sleep expectedFailure Named mmap resize on Windows should raise OSError (not SystemError). Remove expectedFailure mark from TimeEINTRTest.test_sleep as it now passes. * Use expectedFailureIf for TimeEINTRTest.test_sleep on Linux test_sleep passes on macOS but fails on Linux due to timing. * Remove expectedFailure for TimeEINTRTest.test_sleep test_sleep now passes on all platforms.
* Disallow instantiation of sys.getwindowsversion type Add slot_new to PyWindowsVersion that raises TypeError, matching sys.flags behavior. * Remove incorrect WSAHOS errno constant WSAHOS was hardcoded as an alias for WSAHOST_NOT_FOUND, but CPython guards it with #ifdef WSAHOS which doesn't exist in modern Windows SDK headers. * Fix mmap resize to raise OSError instead of SystemError * Fix CreateProcess with empty environment on Windows Empty env dict produced a single null terminator, but CreateProcessW requires a double null for a valid empty environment block. * Revert mmap resize error to SystemError and fix errno.rs formatting mmap resize raises SystemError (not OSError) when mremap is unavailable, matching CPython behavior. test_mmap catches SystemError to skip unsupported resize operations. * Fix named mmap resize to raise OSError and unmark test_sleep expectedFailure Named mmap resize on Windows should raise OSError (not SystemError). Remove expectedFailure mark from TimeEINTRTest.test_sleep as it now passes. * Use expectedFailureIf for TimeEINTRTest.test_sleep on Linux test_sleep passes on macOS but fails on Linux due to timing. * Remove expectedFailure for TimeEINTRTest.test_sleep test_sleep now passes on all platforms.
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
1 participant
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.