io/os: Implement IsTerminal trait on Stdio #81807

ghost · 2021-02-05T22:12:12Z

Simple change to use the 'atty' crate in the cli.rs file instead of a custom function. Fixes #80937

However, I couldn't get this to build locally while running ./x.py build --stage 0 and I don't know why. It gives me this error (edited out unrelated parts):

Compiling atty v0.2.14
error[E0463]: can't find crate for core
error: aborting due to previous error
error: could not compile atty

I think this is all that needs to be done here but I'm fairly new to Rust and completely new to contributing to the actual compiler so I'm not sure. Would appreciate any help!

rust-highfive · 2021-02-05T22:12:15Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

jyn514 · 2021-02-06T23:01:33Z

     Checking atty v0.2.14
error[E0463]: can't find crate for `core`

error: aborting due to previous error

atty needs to be built after core, not before. You may want to see how this is done for other crates like libc.

ghost · 2021-02-07T19:57:10Z

     Checking atty v0.2.14
error[E0463]: can't find crate for `core`

error: aborting due to previous error
atty needs to be built after core, not before. You may want to see how this is done for other crates like libc.

Hey @jyn514! Thanks for the help. I've just made a PR to the atty repo: softprops/atty#45 to add the 'rustc-dep-of-std' feature to 'atty', using the same setup as 'cfg-if': https://github.com/alexcrichton/cfg-if/blob/main/Cargo.toml. Once this is added then it should compile just fine, seems to be working locally for me.

Mark-Simulacrum · 2021-02-12T12:36:32Z

Hm, so I actually am not sure we want to do this, or at least, I'm a bit worried about adding a dependency to libtest which seems to have significantly more complicated logic without a better understanding of that logic and the reasons it exists (probably good ones!); I would also prefer to keep std/test dependencies to ones controlled by rust-lang so that we don't need to fork things or otherwise have trouble editing them. It is also my recollection that there are a number of different crates which do this, in which case it seems reasonable to ask why this one was chosen :)

cc @rust-lang/libs - can you give your opinion here?

m-ou-se · 2021-02-12T13:26:05Z

I'd prefer to not add dependencies to the std/alloc/core/test/.. crates from outside the rust-lang. I wouldn't be opposed to adding a isatty() (is_terminal()? is_console()?) function to the standard library though.

BurntSushi · 2021-02-12T16:32:41Z

Hm, so I actually am not sure we want to do this, or at least, I'm a bit worried about adding a dependency to libtest which seems to have significantly more complicated logic without a better understanding of that logic and the reasons it exists (probably good ones!);

I'm the one who wrote most of that more complicated logic and contributed it to the atty crate. I got it from how git does the same thing. The comments in the code of the atty crate explain most of it I think. The high level bit is that on Windows, detecting a tty differs depending on whether you're in a standard Windows console (such as cmd.exe) or whether you're in an MSYS terminal.

I would also prefer to keep std/test dependencies to ones controlled by rust-lang so that we don't need to fork things or otherwise have trouble editing them.

I don't have any strong opinions on this, but would tend to agree with you. Certainly, I employ this strategy in some cases with my own crates, so it makes sense to me.

It is also my recollection that there are a number of different crates which do this, in which case it seems reasonable to ask why this one was chosen :)

The isatty crate is deprecated in favor of atty. Maybe there are other tty detection crates that have atty's logic now, but atty was definitely the first. Given the simplicity of the API, I've never done a survey of the ecosystem since atty did what I wanted. But my sense is that atty is the de facto standard solution to the problem in the ecosystem today.

Other thoughts:

It might be okay to just copy the logic from atty instead of bringing in the crate itself. The logic has been rather fixed and unchanged for a few years at this point. While some of the details are complex, the amount of code is very small.
I would also be in favor of adding an isatty routine to std as well, although, the way in which the atty crate detects a tty on Windows is pretty non-standard. In particular, its cygwin/MSYS handling is mostly a heuristic. So whether that would be appropriate for a std API isn't quite clear to me. It depends on whether we can get away with documenting it as a platform specific heuristic that can't be relied on 100%.

Amanieu · 2021-02-12T16:59:37Z

I feel that this logic should probably be moved into libstd since we want Stdout to only be line buffered if we are printing to a tty.

rust/library/std/src/io/stdio.rs

Lines 491 to 494 in a118ee2

    
           // FIXME: this should be LineWriter or BufWriter depending on the state of 
        
           //        stdout (tty or not). Note that if this is not line buffered it 
        
           //        should also flush-on-panic or some form of flush-on-abort. 
        
           inner: Pin<&'static ReentrantMutex<RefCell<LineWriter<StdoutRaw>>>>,

ghost · 2021-04-01T04:07:33Z

Coming back to this after getting some more Rust experience.

So basically we'd want to just take the logic that is currently in the atty crate and put it into std/io? If so, should that go in the stdio.rs file, in a new file, or somewhere else?

Would appreciate any help on this!

Amanieu · 2021-04-01T06:42:49Z

I feel that the best place for the logic is in library/std/src/sys/$TARGET/stdio.rs.

This then needs to be exported as an unstable feature in std's public API so that test can import it. Perhaps this could be a method on the Stdin/Stdout/Stderr types?

ghost · 2021-04-23T05:56:36Z

I feel that the best place for the logic is in library/std/src/sys/$TARGET/stdio.rs.

This then needs to be exported as an unstable feature in std's public API so that test can import it. Perhaps this could be a method on the Stdin/Stdout/Stderr types?

Hey @Amanieu , Had a first go at implementing the methods on all of the platforms that the atty crate supported (unix, windows, hermit) . I added an IsAtty trait to std::io that I just repeated on each of the three streams for each of the platforms.

I marked it all with an unstable feature is_atty and also had to mark everything that wasn't part of the feature as stable so the rest of the project still complied. Not sure if that was correct though, especially because I just put it as:
#[stable(feature = "rust1", since = "1.0.0")]
because I don't actually know when these were stabilized. I'm guessing it was pre-1.0 though.

Let me know what you think, I'm sure I did something wrong, lol.

bjorn3 · 2021-04-23T07:53:29Z

library/std/src/lib.rs

@@ -516,6 +516,9 @@ pub mod task {
 mod sys_common;
 mod sys;

+#[unstable(feature = "is_atty", issue = "80937")]
+pub use sys::stdio::{Stderr, Stdin, Stdout};


None of the items inside sys should be exported. This is solely an implementation detail. Instead is_atty should be on the Std* types inside std::io. You could add an IsAtty trait to each of the relevant platform modules inside the os module and implement it there. This should likely forward to a method on sys::stdio::Std* though. The os module is where platform specific extensions should be.

bjorn3 · 2021-04-23T07:54:10Z

library/test/src/lib.rs

+                next_timeout - now
+            } else {
+                Duration::new(0, 0)
+            }


I think you have been formatting with a stable rustfmt that doesn't support some of the options specified in rustfmt.toml. Please use ./x.py fmt instead.

bjorn3 · 2021-04-23T07:55:05Z

library/test/src/cli.rs

@@ -30,7 +31,7 @@ pub struct TestOpts {
 impl TestOpts {
    pub fn use_color(&self) -> bool {
        match self.color {
-            ColorConfig::AutoColor => !self.nocapture && isatty::stdout_isatty(),
+            ColorConfig::AutoColor => !self.nocapture && Stdout::is_atty(),


As written this will break all platforms for which there is no IsAtty implementation yet.

update

jyn514 · 2021-04-23T16:48:54Z

@mattwilkinsonn did you mean to close this?

ghost · 2021-04-23T17:01:22Z

@mattwilkinsonn did you mean to close this?

No was reverting my commits so I can get back on the latest version of the repo. Will reopen once i have another revision

ghost · 2021-04-23T18:38:05Z

@bjorn3
Looking in std::os, seems like the std::os::windows crate is actually std::sys::windows::ext, just re-exported. I assume I should implement the trait in there? Same for unix & hermit.

bjorn3 · 2021-04-23T19:41:05Z

Ah, yes. I looked at the linux specific mod which isn't a re-export of ...::ext.

ghost · 2021-04-24T01:51:13Z

@bjorn3

Ok, I implemented the trait inside the ext modules for each platform on the raw Stdout, Stdin, and Stderr, and then wrapped around those in io, using a cfg_if to prevent issues for the other platforms. Let me know what you think

Amanieu · 2021-04-24T17:01:56Z

library/std/src/io/stdio.rs

+
+/// Trait to determine if stdio stream (Stdin, Stdout, Stderr) is a tty.
+#[unstable(feature = "is_atty", issue = "80937")]
+pub trait IsAtty {


Slight nit: this is actually 3 words: is a TTY

So the proper name for the trait should be IsATty. But that looks pretty ugly so perhaps IsTerminal might be a better name.

Renamed to IsTerminal and renamed the function and feature accordingly

bors · 2021-05-05T16:57:31Z

☔ The latest upstream changes (presumably #84200) made this pull request unmergeable. Please resolve the merge conflicts.

m-ou-se · 2021-07-05T12:09:01Z

I see this is still marked as draft. What work is there left to do before this is ready for review?

bjorn3 · 2021-07-05T12:11:51Z

There are compile error it seems.

wooster0

I would really like to see this move forward. I think it'd be great to have this in the std.

wooster0 · 2021-11-01T18:00:35Z

library/std/src/sys/windows/ext/io.rs

+
+        if unsafe { console_on_any(&[fd]) } {
+            // False positives aren't possible. If we got a console then
+            // we definitely have a tty on stdin.


Suggested change

// we definitely have a tty on stdin.

// we definitely have a tty on stdout.

wooster0 · 2021-11-01T18:00:45Z

library/std/src/sys/windows/ext/io.rs

+
+        if unsafe { console_on_any(&[fd]) } {
+            // False positives aren't possible. If we got a console then
+            // we definitely have a tty on stdin.


Suggested change

// we definitely have a tty on stdin.

// we definitely have a tty on stderr.

wooster0 · 2021-11-01T18:05:56Z

library/std/src/sys/windows/ext/io.rs

+        use c::{
+            STD_ERROR_HANDLE as STD_ERROR, STD_INPUT_HANDLE as STD_INPUT,
+            STD_OUTPUT_HANDLE as STD_OUTPUT,
+        };


Is it a possibility to just remove these 3 uses and use them directly like c::STD_ERROR_HANDLE etc. like it's done in other parts of this file? See e.g.

rust/library/std/src/sys/windows/ext/io.rs

Line 70 in 9213d20

unsafe { c::GetStdHandle(c::STD_INPUT_HANDLE) as RawHandle }

ghost · 2021-11-10T01:58:17Z

Sorry, got distracted with other stuff for the past few months. I'll take a look at this again this week/next week. Thanks for the feedback!

ghost · 2021-11-21T23:27:35Z

I've been looking into making these updates, however it looks like my branch is far enough behind that I can't get the submodule artifacts in order to run any x.py commands.

In addition rebasing onto thousands of commits sounds a bit daunting. What would be the best approach for this? Just revert back to the HEAD and manually rewrite the changes into a new commit?

Amanieu · 2021-11-22T00:05:00Z

Considering the changes aren't too big (only ~250 lines changed) it's reasonable to attempt a manual rewrite if you can't get rebasing to work.

ghost · 2021-11-22T02:51:22Z

Closing as I've reimplemented the changes in #91121.

rust-highfive assigned Mark-Simulacrum Feb 5, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 5, 2021

This comment has been minimized.

Sign in to view

jyn514 added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Feb 6, 2021

ghost mentioned this pull request Feb 7, 2021

feat: Add optional dep-of-std feature softprops/atty#45

Closed

Mark-Simulacrum 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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 12, 2021

bjorn3 reviewed Apr 23, 2021

View reviewed changes

ghost closed this Apr 23, 2021

ghost force-pushed the master branch from 48f50e5 to 5605b5d Compare April 23, 2021 16:31

Merge pull request #1 from rust-lang/master

9b90f9e

update

ghost reopened this Apr 23, 2021

reimplement IsAtty in os module and wrap with io module

f9f2c8f

ghost changed the title ~~libtest: Use 'atty' crate~~ io/os: Implement IsAtty trait on Stdin, Stdout, and Stderr Apr 24, 2021

ghost changed the title ~~io/os: Implement IsAtty trait on Stdin, Stdout, and Stderr~~ io/os: Implement IsAtty trait on Stdio Apr 24, 2021

Amanieu reviewed Apr 24, 2021

View reviewed changes

Matt Wilkinson added 2 commits April 24, 2021 13:48

rename IsAtty to IsTerminal

1d2ea75

fmt

9213d20

ghost changed the title ~~io/os: Implement IsAtty trait on Stdio~~ io/os: Implement IsTerminal trait on Stdio Apr 24, 2021

wooster0 suggested changes Nov 1, 2021

View reviewed changes

ghost mentioned this pull request Nov 22, 2021

io/os: Implement IsTerminal trait on Stdio #91121

Closed

ghost closed this Nov 22, 2021

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

io/os: Implement IsTerminal trait on Stdio #81807

io/os: Implement IsTerminal trait on Stdio #81807

ghost commented Feb 5, 2021

rust-highfive commented Feb 5, 2021

This comment has been minimized.

jyn514 commented Feb 6, 2021

ghost commented Feb 7, 2021 •

edited by ghost

Loading

Mark-Simulacrum commented Feb 12, 2021

m-ou-se commented Feb 12, 2021 •

edited

Loading

BurntSushi commented Feb 12, 2021

Amanieu commented Feb 12, 2021

ghost commented Apr 1, 2021

Amanieu commented Apr 1, 2021

ghost commented Apr 23, 2021

bjorn3 Apr 23, 2021

bjorn3 Apr 23, 2021

bjorn3 Apr 23, 2021

jyn514 commented Apr 23, 2021

ghost commented Apr 23, 2021

ghost commented Apr 23, 2021 •

edited by ghost

Loading

bjorn3 commented Apr 23, 2021

ghost commented Apr 24, 2021

Amanieu Apr 24, 2021

ghost Apr 24, 2021

bors commented May 5, 2021

m-ou-se commented Jul 5, 2021

bjorn3 commented Jul 5, 2021

wooster0 left a comment

wooster0 Nov 1, 2021

wooster0 Nov 1, 2021

wooster0 Nov 1, 2021

ghost commented Nov 10, 2021 •

edited by ghost

Loading

ghost commented Nov 21, 2021

Amanieu commented Nov 22, 2021

ghost commented Nov 22, 2021

	// we definitely have a tty on stdin.
	// we definitely have a tty on stdout.

	// we definitely have a tty on stdin.
	// we definitely have a tty on stderr.

io/os: Implement IsTerminal trait on Stdio #81807

io/os: Implement IsTerminal trait on Stdio #81807

Conversation

ghost commented Feb 5, 2021

rust-highfive commented Feb 5, 2021

This comment has been minimized.

jyn514 commented Feb 6, 2021

ghost commented Feb 7, 2021 • edited by ghost Loading

Mark-Simulacrum commented Feb 12, 2021

m-ou-se commented Feb 12, 2021 • edited Loading

BurntSushi commented Feb 12, 2021

Amanieu commented Feb 12, 2021

ghost commented Apr 1, 2021

Amanieu commented Apr 1, 2021

ghost commented Apr 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyn514 commented Apr 23, 2021

ghost commented Apr 23, 2021

ghost commented Apr 23, 2021 • edited by ghost Loading

bjorn3 commented Apr 23, 2021

ghost commented Apr 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented May 5, 2021

m-ou-se commented Jul 5, 2021

bjorn3 commented Jul 5, 2021

wooster0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Nov 10, 2021 • edited by ghost Loading

ghost commented Nov 21, 2021

Amanieu commented Nov 22, 2021

ghost commented Nov 22, 2021

ghost commented Feb 7, 2021 •

edited by ghost

Loading

m-ou-se commented Feb 12, 2021 •

edited

Loading

ghost commented Apr 23, 2021 •

edited by ghost

Loading

ghost commented Nov 10, 2021 •

edited by ghost

Loading