Expand StdioProvider with terminal types and nonblocking stdin#757
Expand StdioProvider with terminal types and nonblocking stdin#757
Conversation
Add StdioReadError::WouldBlock, StdioIoctlError, TerminalAttributes, WindowSize, SetTermiosWhen, and HostTtyDeviceInfo types. Add read_from_stdin_nonblocking, get/set_terminal_attributes, get/set_window_size, get_terminal_input_bytes, poll_stdin_readable, cancel_stdin, and host_stdin_tty_device_info methods to StdioProvider with sensible defaults. Add mock implementations for get_terminal_input_bytes and poll_stdin_readable. Add test for nonblocking stdin reads.
|
🤖 SemverChecks 🤖 No breaking API changes detected Note: this does not mean API is unchanged, or even that there are no breaking changes; simply, none of the detections triggered. |
jaybosamiya-ms
added
the
expmt:shadow-kiln
jaybosamiya-ms
left a comment
jaybosamiya-ms
left a comment
There was a problem hiding this comment.
I think this PR needs the specific platform side implementations to actually figure out whether the interface defined here is good enough or not. It is currently drastically expanding out the platform interface with a very Linux-y view, which makes me worry that the cross-platform nature becomes harder. At minimum, looking at the relative sizes of implementations of these interfaces for the Linux platform and the Windows platform should help.
I am also not convinced that merging this in directly with StdioProvider is the right move. One possibility is to have StdioProvider have an associated type called Terminal which implements a Terminal trait that has all these bits inside it; this trait would not have any defaults set, but litebox::platform::trivial_providers or such can provide a NotATerminal: Terminal type that platforms can just pick up if they don't want to support terminals. That would also improve long term stability, since we would not be accidentally regressing a fully terminal-supported platform to a partial-supported one if/when the interface evolves.
| /// Errors from terminal operations on [`StdioProvider`]. | ||
| #[derive(Error, Debug)] | ||
| #[non_exhaustive] | ||
| pub enum StdioIoctlError { |
There was a problem hiding this comment.
Nit: Consider StdioTerminalError instead
| /// The operation failed with an OS error code (errno on Linux, mapped | ||
| /// equivalent on other platforms). | ||
| #[error("ioctl failed: {0}")] | ||
| OsError(i32), |
There was a problem hiding this comment.
This i32 overfits to Linux, needs a better generalized error mapping
| /// Platform-agnostic terminal attributes, mirroring the fields of Linux | ||
| /// `struct termios`. | ||
| /// | ||
| /// The shim layer translates these fields to and from the guest ABI. | ||
| /// Platform implementations fill this struct using their native APIs (e.g., | ||
| /// direct ioctl forwarding on Linux, `GetConsoleMode`/`SetConsoleMode` on | ||
| /// Windows). | ||
| #[derive(Debug, Clone)] | ||
| pub struct TerminalAttributes { | ||
| /// Input mode flags. | ||
| pub c_iflag: u32, | ||
| /// Output mode flags. | ||
| pub c_oflag: u32, | ||
| /// Control mode flags. | ||
| pub c_cflag: u32, | ||
| /// Local mode flags. | ||
| pub c_lflag: u32, | ||
| /// Line discipline (typically `0` for `N_TTY`). | ||
| pub c_line: u8, | ||
| /// Control characters. | ||
| pub c_cc: [u8; 19], | ||
| } |
There was a problem hiding this comment.
I'm not totally against this, but I need to see the Windows userland platform implementation of this to be convinced that this is actually fine.
As it currently stands, it is extremely Linux centric (e.g., the flags literally hardcode the Linux ABI).
Also, "typically" begs the question of "when not"? Also, what is N_TTY in the context of a platform-agnostic thing?
| // Terminal attribute flag constants. | ||
| const TERMATTR_ECHO: u32 = 0x0008; | ||
| const TERMATTR_ICRNL: u32 = 0x0100; | ||
| const TERMATTR_OPOST: u32 = 0x0001; | ||
| const TERMATTR_ONLCR: u32 = 0x0004; | ||
|
|
||
| impl TerminalAttributes { | ||
| /// Default terminal attributes matching a freshly opened Linux PTY. | ||
| /// | ||
| /// These are realistic values that satisfy terminal detection in programs | ||
| /// such as Node.js Ink. **All-zero termios causes such programs to reject | ||
| /// the terminal silently.** | ||
| pub fn new_default() -> Self { | ||
| Self { | ||
| c_iflag: 0x6d02, // ICRNL | IXON | IXANY | IMAXBEL | IUTF8 | ||
| c_oflag: 0x0005, // OPOST | ONLCR | ||
| c_cflag: 0x04bf, // CS8 | CREAD | CLOCAL | B38400 | ||
| c_lflag: 0x8a3b, // ECHO | ECHOE | ECHOK | ISIG | ICANON | IEXTEN | ECHOCTL | ECHOKE | ||
| c_line: 0, // N_TTY | ||
| c_cc: [ | ||
| 0x03, 0x1c, 0x7f, 0x15, 0x04, 0x00, 0x01, 0x00, 0x11, 0x13, 0x1a, 0xff, 0x12, 0x0f, | ||
| 0x17, 0x16, 0xff, 0x00, 0x00, | ||
| ], | ||
| } | ||
| } |
There was a problem hiding this comment.
All this is hardcoding a lot of Linux-ism.
| /// Returns `true` if the `ECHO` local flag is set. | ||
| pub fn echo_enabled(&self) -> bool { | ||
| self.c_lflag & TERMATTR_ECHO != 0 | ||
| } | ||
|
|
||
| /// Returns `true` if the `ICRNL` input flag is set. | ||
| pub fn icrnl_enabled(&self) -> bool { | ||
| self.c_iflag & TERMATTR_ICRNL != 0 | ||
| } | ||
|
|
||
| /// Returns `true` if output post-processing with newline translation | ||
| /// (`OPOST | ONLCR`) is enabled. | ||
| pub fn onlcr_enabled(&self) -> bool { | ||
| (self.c_oflag & TERMATTR_OPOST != 0) && (self.c_oflag & TERMATTR_ONLCR != 0) | ||
| } |
There was a problem hiding this comment.
Why are these functions necessary? Can't a shim just look at the public fields?
| } | ||
| } | ||
|
|
||
| /// Platform-agnostic terminal window size. |
There was a problem hiding this comment.
Why is this mentioning "Platform-agnostic terminal window size" (emphasis mine). This seems like the agent was told to remove "Linux" and make it platform agnostic, and it literally did that. Unnecessary/superflous text should just be removed entirely from documentation.
| /// Horizontal size in pixels (informational, often zero). | ||
| pub xpixel: u16, | ||
| /// Vertical size in pixels (informational, often zero). | ||
| pub ypixel: u16, |
There was a problem hiding this comment.
If it is informational and often zero, why not just remove it?
2 participants
Summary
StdioReadError::WouldBlockvariant for nonblocking stdin reads.StdioIoctlErrorenum for terminal operation failures.TerminalAttributesandWindowSizestructs.HostTtyDeviceInfostruct for host terminal device metadata.StdioProviderwith methods:get_terminal_attributes,set_terminal_attributes,get_window_size,set_window_size,read_stdin_nonblocking,cancel_stdin_read,host_tty_device_info.fs/devices.rsto handleStdioReadError::WouldBlock.Split from #743.