Implement termios.tcgetwinsize/tcsetwinsize - #8347
Conversation
Adds ioctl(TIOCGWINSZ/TIOCSWINSZ) wrappers in host_env and the corresponding Python-facing functions in the termios stdlib module, removing the associated expectedFailure markers in test_termios.py. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01RbzcYLnX5tMfTM9BP7FDTT
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds terminal window-size retrieval and update functions to the host termios layer and exposes them through Python’s ChangesTerminal window sizing
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant PythonTermios
participant HostTermios
participant TerminalFD
PythonTermios->>HostTermios: Request or set terminal window size
HostTermios->>TerminalFD: Issue TIOCGWINSZ or TIOCSWINSZ ioctl
TerminalFD-->>HostTermios: Return size or OS error
HostTermios-->>PythonTermios: Return tuple, None, or termios.error
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/pty.py dependencies:
dependent tests: (4 tests)
[x] test: cpython/Lib/test/test_termios.py (TODO: 1) dependencies: dependent tests: (8 tests)
Legend:
|
There was a problem hiding this comment.
Pull request overview
This PR implements the missing termios.tcgetwinsize() / termios.tcsetwinsize() APIs in RustPython so Python code can query and set terminal window size, and it unblocks upstream Lib/test/test_termios.py tests that were previously marked as expected failures due to AttributeError.
Changes:
- Added low-level
ioctl(TIOCGWINSZ/TIOCSWINSZ)wrappers incrates/host_env. - Exposed
tcgetwinsize/tcsetwinsizeas#[pyfunction]s in thetermiosstdlib module. - Removed
@unittest.expectedFailuremarkers for the newly-working termios winsize tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| Lib/test/test_termios.py | Removes expectedFailure decorators now that winsize APIs exist and tests pass. |
| crates/stdlib/src/termios.rs | Adds Python-facing tcgetwinsize/tcsetwinsize bindings on top of host termios support. |
| crates/host_env/src/termios.rs | Adds platform-level ioctl wrappers to get/set terminal window size. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](/web-api/iframe-proxy?url=https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/stdlib/src/termios.rs`:
- Around line 281-283: Update the size extraction in tcsetwinsize to use a
tuple/list-only conversion instead of extract_elements_with, rejecting
dictionaries and other iterables while preserving the existing two-element
validation and error behavior.
- Around line 285-292: Update the row and col conversion logic in the
tcsetwinsize implementation to use the VM’s index-protocol conversion rather
than direct PyInt downcasts, accepting integer-like objects implementing
__index__. Normalize every conversion failure, including negative or otherwise
out-of-range values, to OverflowError while preserving the existing winsize
validation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 5975d213-b9d3-41d1-8919-28c91cbd4bf7
⛔ Files ignored due to path filters (1)
Lib/test/test_termios.pyis excluded by!Lib/**
📒 Files selected for processing (2)
crates/host_env/src/termios.rscrates/stdlib/src/termios.rs
POSIX ioctl only guarantees -1 on failure, not exactly 0 on success; matches the existing check_libc_neg convention used elsewhere in host_env (e.g. fcntl.rs, posix.rs::get_terminal_size). Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01RbzcYLnX5tMfTM9BP7FDTT
extract_elements_with also accepted dicts (treating keys as elements). Switch to try_sequence(), matching CPython's PySequence_Check, so only real sequences (tuple/list/etc.) are accepted. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01RbzcYLnX5tMfTM9BP7FDTT
`std::mem` just re-exports `core::mem`, but clippy prefers importing from core when there's no OS dependency. This was breaking the wasm CI build. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01RbzcYLnX5tMfTM9BP7FDTT
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
crates/stdlib/src/termios.rs:292
- tcsetwinsize currently requires both winsize items to be actual PyInt objects via downcast_ref::(), which rejects valid int-like inputs (e.g. objects implementing index, and bool). CPython uses PyLong_AsLong for each item, which accepts index and bool, so this is a behavior incompatibility.
let row: u16 = row
.downcast_ref::<PyInt>()
.ok_or_else(|| vm.new_type_error("tcsetwinsize: winsize values must be integers"))?
.try_to_primitive(vm)?;
let col: u16 = col
CPython's PyLong_AsLong calls __index__ on non-int objects before converting, so downcast_ref::<PyInt> was stricter than CPython. try_index matches that behavior and is more concise. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01RbzcYLnX5tMfTM9BP7FDTT
Both were hidden by a skipIf on tty.tcgetwinsize, which now exists. Root cause: pty.fork() calls os.login_tty(), which isn't implemented, so the forked child crashes before the test body runs. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01RbzcYLnX5tMfTM9BP7FDTT
expectedFailure still runs the test body, so pty.fork()'s real fork()
still happens and the child crashes on missing os.login_tty inside the
parallel test runner's worker process, corrupting its JSON reporting
channel ("worker bug"). skip prevents the method from running at all,
avoiding that. Verified locally with -j 2.
Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01RbzcYLnX5tMfTM9BP7FDTT
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
Lib/test/test_pty.py:301
- Same as
test_fork: the PR description mentionsexpectedFailure, but this change usesskip. If this is a known failing assertion (rather than a hang/crash),expectedFailureis preferable so the test still runs and will surface as an unexpected pass once fixed.
@unittest.skip("TODO: RUSTPYTHON; pty.fork() child is not made a session leader")
def test_spawn_doesnt_hang(self):
| pub fn tcsetwinsize(fd: i32, row: u16, col: u16) -> std::io::Result<()> { | ||
| let mut size: libc::winsize = unsafe { core::mem::zeroed() }; | ||
| let ret = unsafe { libc::ioctl(fd, TIOCGWINSZ as _, &mut size) }; | ||
| if ret < 0 { | ||
| return Err(std::io::Error::last_os_error()); | ||
| } | ||
| size.ws_row = row; | ||
| size.ws_col = col; | ||
| let ret = unsafe { libc::ioctl(fd, TIOCSWINSZ as _, &size) }; | ||
| if ret < 0 { | ||
| return Err(std::io::Error::last_os_error()); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
CPython also calls ioctl twice
https://github.com/python/cpython/blob/b35c37910a4595f22458cf0291a9b0252b3c6c70/Modules/termios.c#L529-L543
| s2 = _readline(master_fd) | ||
| self.assertEqual(b'For my pet fish, Eric.\n', normalize_output(s2)) | ||
|
|
||
| @unittest.skip("TODO: RUSTPYTHON; pty.fork() child is not made a session leader") |
There was a problem hiding this comment.
We avoid to skip tests without good reasons.
Is this test always failing? Then please use @unittest.expectedFailure
Is this test specifically failing in certain platforms? Then please use @unittest.expectedFailureIf
Otherwise please add a comment why this test must be skipped rather than expectedFailure marked.
There was a problem hiding this comment.
Thanks for the review!
Implementing termios.tcgetwinsize/tcsetwinsize un-skips test_pty.py's PtyTest class.
Two of its tests, test_fork and test_spawn_doesnt_hang, then fail because pty.fork() calls os.login_tty(), which isn't implemented:
pty.fork()
└─ tries os.forkpty() → not implemented → fails, falls through
└─ calls os.fork() → succeeds, real fork happens
└─ (in the child) calls os.login_tty() → not implemented → AttributeError
I used unittest.skip instead of expectedFailure because expectedFailure still runs the test body, so the real fork() still happens. The forked child crashes inside the parallel test runner's worker process, corrupting its JSON reporting channel back to the main runner -> this shows up as worker bug, not a normal test failure, and is what broke CI.
[reproduce the bug locally, when used expectedFailure]
Without parallelism (plain):
./target/release/rustpython -m test test_pty -v
With parallelism (what CI uses, reproduces the worker bug):
./target/release/rustpython -u -m test --slow-ci -j 2 test_pty
I'd like to hear your thoughts on whether it'd be better to keep it as expectedFailure.
There was a problem hiding this comment.
please use expectedFailure as long as test passes with expectedFailure.
please keep it skip when it cause hang, panic or any kind of problem entirely stop the test process. but leave a comment what made you decide to skip the test
| fn tcsetwinsize(Fildes(fd): Fildes, size: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> { | ||
| let seq = size.try_sequence(vm)?; | ||
| if seq.length(vm)? != 2 { | ||
| return Err(vm.new_type_error("tcsetwinsize: size must be a 2 element sequence")); | ||
| } |
There was a problem hiding this comment.
size also is looking like a pair of number, always.
| fn tcsetwinsize(Fildes(fd): Fildes, size: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> { | |
| let seq = size.try_sequence(vm)?; | |
| if seq.length(vm)? != 2 { | |
| return Err(vm.new_type_error("tcsetwinsize: size must be a 2 element sequence")); | |
| } | |
| fn tcsetwinsize(Fildes(fd): Fildes, size: (u16, u16), vm: &VirtualMachine) -> PyResult<()> { | |
| let (row, col) = size; |
There was a problem hiding this comment.
Tested it locally, and it behaves differently via FromArgs impl than a tuple or list argument would.
There was a problem hiding this comment.
ah, you are right. I am sorry.
* Implement termios.tcgetwinsize/tcsetwinsize Adds ioctl(TIOCGWINSZ/TIOCSWINSZ) wrappers in host_env and the corresponding Python-facing functions in the termios stdlib module, removing the associated expectedFailure markers in test_termios.py. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01RbzcYLnX5tMfTM9BP7FDTT * Use ret < 0 convention for ioctl error checks in termios winsize POSIX ioctl only guarantees -1 on failure, not exactly 0 on success; matches the existing check_libc_neg convention used elsewhere in host_env (e.g. fcntl.rs, posix.rs::get_terminal_size). Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01RbzcYLnX5tMfTM9BP7FDTT * Reject non-sequence size in tcsetwinsize extract_elements_with also accepted dicts (treating keys as elements). Switch to try_sequence(), matching CPython's PySequence_Check, so only real sequences (tuple/list/etc.) are accepted. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01RbzcYLnX5tMfTM9BP7FDTT * Use `core::mem::zeroed` instead of `std::mem::zeroed` `std::mem` just re-exports `core::mem`, but clippy prefers importing from core when there's no OS dependency. This was breaking the wasm CI build. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01RbzcYLnX5tMfTM9BP7FDTT * Use try_index for tcsetwinsize row/col conversion CPython's PyLong_AsLong calls __index__ on non-int objects before converting, so downcast_ref::<PyInt> was stricter than CPython. try_index matches that behavior and is more concise. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01RbzcYLnX5tMfTM9BP7FDTT * Mark test_fork/test_spawn_doesnt_hang as expected failures Both were hidden by a skipIf on tty.tcgetwinsize, which now exists. Root cause: pty.fork() calls os.login_tty(), which isn't implemented, so the forked child crashes before the test body runs. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01RbzcYLnX5tMfTM9BP7FDTT * Use unittest.skip instead of expectedFailure for pty.fork() tests expectedFailure still runs the test body, so pty.fork()'s real fork() still happens and the child crashes on missing os.login_tty inside the parallel test runner's worker process, corrupting its JSON reporting channel ("worker bug"). skip prevents the method from running at all, avoiding that. Verified locally with -j 2. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01RbzcYLnX5tMfTM9BP7FDTT * Clarify skip reason for pty.fork() tests * Simplify tcgetwinsize/tcsetwinsize fd argument with Fildes destructuring * Explain why pty.fork() tests use skip instead of expectedFailure --------- Co-authored-by: Claude Sonnet 5 <[email protected]>
Summary
The termios module was missing
tcgetwinsize/tcsetwinsize, so callingtermios.tcgetwinsize(fd)raised AttributeError. This PR adds both functions, following CPython's implementation Modules/termios.c#L417-L444:Summary by CodeRabbit
Summary by CodeRabbit
termios.tcgetwinsize()to retrieve terminal window dimensions.termios.tcsetwinsize()to update terminal window dimensions.termioserrors.u16.