Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add IsTerminal trait to determine if a descriptor or handle is a terminal #98033

Merged
merged 4 commits into from
Oct 15, 2022

Conversation

joshtriplett
Copy link
Member

@joshtriplett joshtriplett commented Jun 12, 2022

The UNIX implementation uses isatty. The Windows implementation uses
the same logic the atty crate uses, including the hack needed to
detect msys terminals.

Implement this trait for Stdin/Stdout/Stderr/File on all
platforms. On Unix, implement it for BorrowedFd/OwnedFd. On Windows,
implement it for BorrowedHandle/OwnedHandle.

Based on #91121

Co-authored-by: Matt Wilkinson mattwilki17@gmail.com

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jun 12, 2022
@rust-highfive
Copy link
Collaborator

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 12, 2022
@joshtriplett joshtriplett added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 12, 2022
@joshtriplett joshtriplett force-pushed the is-terminal-fd-handle branch 2 times, most recently from 2202b56 to 321f20c Compare June 12, 2022 21:39
@rust-log-analyzer

This comment has been minimized.

@joshtriplett joshtriplett force-pushed the is-terminal-fd-handle branch 2 times, most recently from 857c920 to 08f4705 Compare June 12, 2022 21:58
@rust-log-analyzer

This comment has been minimized.

@joshtriplett joshtriplett force-pushed the is-terminal-fd-handle branch 2 times, most recently from a0faee9 to 920f59b Compare June 12, 2022 22:58
@joshtriplett joshtriplett changed the title Add IsTerminal trait with implementations for AsFd and AsHandle Add IsTerminal trait with implementations for AsFd and AsHandle Jun 12, 2022
@the8472
Copy link
Member

the8472 commented Jun 12, 2022

If we had the view-from-borrowed thing then we don't need the sealing complications and instead one would do file.as_view::<IsTerminal>().is_terminal() where IsTerminal is an fd-wrapping struct like File.

@joshtriplett
Copy link
Member Author

sealing complications

I just finished implementing sealing after all, which turned out to be possible.

@joshtriplett joshtriplett removed the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Jun 13, 2022
@the8472
Copy link
Member

the8472 commented Jun 13, 2022

This could also be a portability hazard. On unix you can now do tcp_socket.is_terminal() but it would fail to compile on windows because sockets and handles are separate.

I think it would be better if we added File::is_terminal (since any file could be a terminal anyway. /dev/tty, conin$) and then people would have to acknowledge the platform-dependence themselves by doing.

let is_terminal = false;
#[cfg(windows)]
let is_terminal = stdin.as_view::<File>().is_terminal()?;
#[cfg(unix)]
let is_terminal = stdin.as_view::<File>().is_terminal()?;

This isn't too terrible since one doesn't need cfg_if, only shadowing.

@joshtriplett
Copy link
Member Author

@the8472 I continue to be quite skeptical of the "File view" mechanism, as opposed to BorrowedFd and OwnedFd and similar.

However, we could change this to implement IsTerminal for Stdin/Stdout/Stderr/File on every platform, and then implement it for BorrowedFd and OwnedFd on Unix, and BorrowedHandle and OwnedHandle on Windows. That would then mean you can unconditionally and portably call it on Stdin/Stdout/Stderr/File, while if you want to call it on an AsFd you have to call .as_fd().is_terminal() and import AsFd with associated target-specific code.

@the8472
Copy link
Member

the8472 commented Jun 13, 2022

I continue to be quite skeptical of the "File view" mechanism, as opposed to BorrowedFd and OwnedFd and similar.

I have shifted my position a bit, wrappers should have to explicitly opt-in to this kind of conversion but I see File as a prime candidate for that because it does really basic operations that should either be valid or harmlessly error on any file descriptor (read, write, stat, fsync, seek, chmod). At least on unix is_terminal is just another flavor of stat. pipes, sockets and ttys can show up in the filesystem anyway.

I think the conversion is better because we have those structs and useful methods implemented on them anyway. Similarly something might pass a socket as std-in and if you want to do socket-ops on it you need to borrow it as a TcpStream for example.

But maybe you're right and this is a mistake of the past and the basic operations should have been implemented on BorrowedFd and only stateful operations such as iterating a directory via getdents would live on wrapper types

However, we could change this to implement IsTerminal for Stdin/Stdout/Stderr/File on every platform, and then implement it for BorrowedFd and OwnedFd on Unix, and BorrowedHandle and OwnedHandle on Windows.

Not how I'd do it, but that works too.

@joshtriplett joshtriplett changed the title Add IsTerminal trait with implementations for AsFd and AsHandle Add IsTerminal trait to determine if a descriptor or handle is a terminal Jun 13, 2022
@rust-log-analyzer

This comment has been minimized.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 8, 2022
… r=thomcc

Add `IsTerminal` trait to determine if a descriptor or handle is a terminal

The UNIX implementation uses `isatty`. The Windows implementation uses
the same logic the `atty` crate uses, including the hack needed to
detect msys terminals.

Implement this trait for `Stdin`/`Stdout`/`Stderr`/`File` on all
platforms. On Unix, implement it for `BorrowedFd`/`OwnedFd`. On Windows,
implement it for `BorrowedHandle`/`OwnedHandle`.

Based on rust-lang#91121

Co-authored-by: Matt Wilkinson <mattwilki17@gmail.com>
@matthiaskrgr
Copy link
Member

@bors r- rollup=iffy failed in a rollup
#102814 (comment)

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 8, 2022
joshtriplett and others added 4 commits October 15, 2022 00:35
…rminal

The UNIX and WASI implementations use `isatty`. The Windows
implementation uses the same logic the `atty` crate uses, including the
hack needed to detect msys terminals.

Implement this trait for `File` and for `Stdin`/`Stdout`/`Stderr` and
their locked counterparts on all platforms. On UNIX and WASI, implement
it for `BorrowedFd`/`OwnedFd`. On Windows, implement it for
`BorrowedHandle`/`OwnedHandle`.

Based on rust-lang#91121

Co-authored-by: Matt Wilkinson <mattwilki17@gmail.com>
If a process has no console, it'll have NULL in place of a console
handle, so return early with `false` in that case without making any OS
calls.
Rather than referencing a slice's pointer and then creating a new slice
with a longer length, offset from the base structure pointer instead.
This makes some choices of Rust semantics happier.
@joshtriplett
Copy link
Member Author

Rebased on top of #102847 .

@bors r=thomcc

@bors
Copy link
Contributor

bors commented Oct 14, 2022

📌 Commit 97d438c has been approved by thomcc

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 14, 2022
@bors
Copy link
Contributor

bors commented Oct 15, 2022

⌛ Testing commit 97d438c with merge 8154955...

@bors
Copy link
Contributor

bors commented Oct 15, 2022

☀️ Test successful - checks-actions
Approved by: thomcc
Pushing 8154955 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 15, 2022
@bors bors merged commit 8154955 into rust-lang:master Oct 15, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 15, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8154955): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
2.4% [1.5%, 3.4%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-3.2% [-3.9%, -2.6%] 3
Improvements ✅
(secondary)
-3.1% [-5.3%, -2.1%] 29
All ❌✅ (primary) -1.0% [-3.9%, 3.4%] 5

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.5% [-2.8%, -2.1%] 2
Improvements ✅
(secondary)
-4.0% [-5.9%, -2.1%] 2
All ❌✅ (primary) -2.5% [-2.8%, -2.1%] 2

Footnotes

  1. the arithmetic mean of the percent change 2

  2. number of relevant changes 2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.