Stream: git-wasmtime

Topic: wasmtime / PR #3696 Fix `isatty` in WASI.

Wasmtime GitHub notifications bot (Jan 18 2022 at 23:19):

sunfishcode opened PR #3696 from sunfishcode/isatty to main:

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.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Jan 19 2022 at 00:09):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Jan 19 2022 at 07:58):

bjorn3 created PR review comment:

Can this be cfg(unix) and cfg(not(unix)) instead?

Wasmtime GitHub notifications bot (Jan 19 2022 at 07:58):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jan 19 2022 at 14:22):

sunfishcode updated PR #3696 from sunfishcode/isatty to main.

Wasmtime GitHub notifications bot (Jan 19 2022 at 15:01):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 19 2022 at 15:01):

alexcrichton created PR review comment:

Could atty be used on all platforms? (on Unix does atty differ from rustix?)

Wasmtime GitHub notifications bot (Jan 19 2022 at 15:40):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jan 19 2022 at 15:40):

bjorn3 created PR review comment:

On Linux rustix can use inline asm, while atty always uses libc.

Wasmtime GitHub notifications bot (Jan 19 2022 at 15:51):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Jan 19 2022 at 15:51):

sunfishcode created PR review 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.

Wasmtime GitHub notifications bot (Jan 19 2022 at 15:58):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 19 2022 at 15:58):

alexcrichton created PR review 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.

Wasmtime GitHub notifications bot (Jan 19 2022 at 16:01):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Jan 19 2022 at 16:01):

sunfishcode created PR review comment:

The actual change uses cfg(unix), so it isn't Linux-specific.

Wasmtime GitHub notifications bot (Jan 19 2022 at 16:07):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 19 2022 at 16:07):

alexcrichton created PR review 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.

Wasmtime GitHub notifications bot (Jan 21 2022 at 20:16):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Jan 21 2022 at 20:16):

sunfishcode created PR review 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.

Wasmtime GitHub notifications bot (Jan 21 2022 at 20:16):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Jan 21 2022 at 20:55):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jan 21 2022 at 20:55):

bjorn3 created PR review comment:

The isatty crate would figure that out, right?

Wasmtime GitHub notifications bot (Jan 21 2022 at 20:55):

bjorn3 edited PR review comment.

Wasmtime GitHub notifications bot (Jan 21 2022 at 21:13):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Jan 21 2022 at 21:13):

sunfishcode created PR review 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.

Wasmtime GitHub notifications bot (Jan 21 2022 at 21:51):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jan 21 2022 at 21:51):

bjorn3 created PR review 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.

Wasmtime GitHub notifications bot (Jan 21 2022 at 21:56):

sunfishcode updated PR #3696 from sunfishcode/isatty to main.

Wasmtime GitHub notifications bot (Jan 21 2022 at 21:56):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Jan 21 2022 at 21:56):

sunfishcode created PR review comment:

Ah, ok. I've now made this change.

Wasmtime GitHub notifications bot (Jan 22 2022 at 02:54):

sunfishcode updated PR #3696 from sunfishcode/isatty to main.

Wasmtime GitHub notifications bot (Jan 22 2022 at 02:54):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Jan 22 2022 at 02:54):

sunfishcode created PR review comment:

I've now factored this to use a platform-agnostic crate, so that Wasmtime's own code avoids the cfgs here.

Wasmtime GitHub notifications bot (Jan 22 2022 at 03:12):

sunfishcode updated PR #3696 from sunfishcode/isatty to main.

Wasmtime GitHub notifications bot (Jan 24 2022 at 15:20):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 24 2022 at 15:20):

alexcrichton created PR review 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)

Wasmtime GitHub notifications bot (Jan 24 2022 at 15:31):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jan 24 2022 at 15:31):

bjorn3 created PR review 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.

Wasmtime GitHub notifications bot (Jan 24 2022 at 19:43):

sunfishcode created PR review 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.

Wasmtime GitHub notifications bot (Jan 24 2022 at 19:43):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Jan 24 2022 at 19:45):

sunfishcode merged PR #3696.

Last updated: Dec 23 2024 at 13:07 UTC