Fix isatty in WASI.#3696
Conversation
Subscribe to Label Actioncc @kubkon DetailsThis issue or pull request has been labeled: "wasi"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
| #[cfg(windows)] | ||
| { | ||
| false | ||
| } |
There was a problem hiding this comment.
Can this be cfg(unix) and cfg(not(unix)) instead?
There was a problem hiding this comment.
I changed the first to cfg(unix), but I think cfg(windows) makes sense for the second. If we port to a non-Unix non-Windows platform in the future, we should figure out what isatty means for that platform.
There was a problem hiding this comment.
The atty crate would figure that out, right?
There was a problem hiding this comment.
Not in its current form, because the atty crate currently has no interface for passing in a handle. It just supports literal stdin, stdout, and stderr. Possibly that will change in the future, and then we could simplify code like this.
There was a problem hiding this comment.
In this case yes, but in stdio.rs you are using atty on stdin only for windows. There the #[cfg(windows)] can be turned into #[cfg(not(unix))] without issues.
There was a problem hiding this comment.
Ah, ok. I've now made this change.
| #[cfg(windows)] | ||
| { | ||
| atty::is(atty::Stream::Stdin) | ||
| } |
There was a problem hiding this comment.
Could atty be used on all platforms? (on Unix does atty differ from rustix?)
There was a problem hiding this comment.
On Linux rustix can use inline asm, while atty always uses libc.
There was a problem hiding this comment.
My reason for using rustix here was that cap-std-sync already depends on rustix for other things, so this avoids adding a new dependency on atty on unix-family platforms.
Rustix's isatty uses the same ioctl as musl, which is a different ioctl from what glibc uses, but the difference shouldn't matter in practice.
There was a problem hiding this comment.
Hm ok, personally I'm not a fan of having a lot of "if linux else everyone else this" because it seems like it'll just cause tons of Linux-specific or not-Linux-specific issues. This is pretty minor though.
There was a problem hiding this comment.
The actual change uses cfg(unix), so it isn't Linux-specific.
There was a problem hiding this comment.
Oh sure but it's the same thing with Windows. Crates like atty which provide a platform-agnostic interface at least theoretically deal with the portability within them (and afaik atty is widely used and well vetted). If we specifically don't use that then we're subject to all the bugs that atty would get except we'll get them instead (theoretically) at some later point.
But again this is personal preference and this is minor. I don't like to pervasively use platform-specific crates, instead using platform-specific crates in very specific situations and otherwise favoring platform-agnostic crates for heavy usage.
There was a problem hiding this comment.
I've now factored this to use a platform-agnostic crate, so that Wasmtime's own code avoids the cfgs here.
There was a problem hiding this comment.
To clarify I'm fine either way, mostly just stating my own personal preference. But also, this looks the same as before, so unsure if this is intended to stay as-is or whether a commit was forgotten? (this PR is fine to merge whenever imo, I don't feel any need to debate fine detatils like this)
There was a problem hiding this comment.
This one now has #[cfg(unix)] and #[cfg(not(unix))]. The other is left as #[cfg(unix)] and #[cfg(windows)] without fallback for non-unix, non-windows platforms.
There was a problem hiding this comment.
That's consistent with impl AsFd for File under cfg(unix) which is already there. Ideally we should support non-windows-non-unix, but that's a bigger change.
And, I'm working on a change which will help; I plan to replace all these isatty implementations with a new is-terminal crate which supports windows, unix, and (to the extent possible) non-windows-non-unix with a single API. I'll do it in a separate PR because I need to make a few other changes to avoid cargo deny flagging it for duplicate dependency versions.
WASI doesn't have an `isatty` ioctl or syscall, so wasi-libc's `isatty` implementation uses the file descriptor type and rights to determine if the file descriptor is likely to be a tty. The real fix here will be to add an `isatty` call to WASI. But for now, have Wasmtime set the filetype and rights for file descriptors so that wasi-libc's `isatty` works as expected.
sunfishcode
force-pushed
the
sunfishcode/isatty
branch
2 times, most recently
from
7cb5f06 to
a0aaf9f
Compare
Following up on bytecodealliance#3696, use the new is-terminal crate to test for a tty rather than having platform-specific logic in Wasmtime. The is-terminal crate has a platform-independent API which takes a handle. This also updates the tree to cap-std 0.24 etc., to avoid depending on multiple versions of io-lifetimes at once, as enforced by the cargo deny check.
Following up on bytecodealliance#3696, use the new is-terminal crate to test for a tty rather than having platform-specific logic in Wasmtime. The is-terminal crate has a platform-independent API which takes a handle. This also updates the tree to cap-std 0.24 etc., to avoid depending on multiple versions of io-lifetimes at once, as enforced by the cargo deny check.
Following up on #3696, use the new is-terminal crate to test for a tty rather than having platform-specific logic in Wasmtime. The is-terminal crate has a platform-independent API which takes a handle. This also updates the tree to cap-std 0.24 etc., to avoid depending on multiple versions of io-lifetimes at once, as enforced by the cargo deny check.
WASI doesn't have an `isatty` ioctl or syscall, so wasi-libc's `isatty` implementation uses the file descriptor type and rights to determine if the file descriptor is likely to be a tty. The real fix here will be to add an `isatty` call to WASI. But for now, have Wasmtime set the filetype and rights for file descriptors so that wasi-libc's `isatty` works as expected.
Following up on bytecodealliance#3696, use the new is-terminal crate to test for a tty rather than having platform-specific logic in Wasmtime. The is-terminal crate has a platform-independent API which takes a handle. This also updates the tree to cap-std 0.24 etc., to avoid depending on multiple versions of io-lifetimes at once, as enforced by the cargo deny check.
Following up on bytecodealliance#3696, use the new is-terminal crate to test for a tty rather than having platform-specific logic in Wasmtime. The is-terminal crate has a platform-independent API which takes a handle. This also updates the tree to cap-std 0.24 etc., to avoid depending on multiple versions of io-lifetimes at once, as enforced by the cargo deny check.
4 participants
WASI doesn't have an
isattyioctl or syscall, so wasi-libc'sisattyimplementation uses the file descriptor type and rights to determine if
the file descriptor is likely to be a tty. The real fix here will be to
add an
isattycall to WASI. But for now, have Wasmtime set thefiletype and rights for file descriptors so that wasi-libc's
isattyworks as expected.