Enable or disable colored output according to CLICOLOR(_FORCE)

[jhasse]

Updated version of #27867.

Also related: https://users.rust-lang.org/t/enable-or-disable-colored-output-according-to-clicolor--force/4445

[rust-highfive]

r? @shepmaster

(rust_highfive has picked a reviewer for you, use r? to override)

[rust-highfive]

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-12-09T16:56:00.3916826Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-12-09T16:56:00.3931401Z ##[command]git config gc.auto 0
2019-12-09T16:56:00.3936569Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-12-09T16:56:00.3940324Z ##[command]git config --get-all http.proxy
2019-12-09T16:56:00.3946115Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67177/merge:refs/remotes/pull/67177/merge
---
2019-12-09T17:01:47.3960484Z    Compiling serde_json v1.0.40
2019-12-09T17:01:49.0755779Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-12-09T17:01:59.5935292Z     Finished release [optimized] target(s) in 1m 24s
2019-12-09T17:01:59.6039858Z tidy check
2019-12-09T17:02:00.7671648Z tidy error: /checkout/src/librustc_errors/emitter.rs:462: line longer than 100 chars
2019-12-09T17:02:02.3753914Z Found 485 error codes
2019-12-09T17:02:02.3754092Z Found 0 error codes with no tests
2019-12-09T17:02:02.3754141Z Done!
2019-12-09T17:02:02.3813002Z some tidy checks failed
2019-12-09T17:02:02.3813002Z some tidy checks failed
2019-12-09T17:02:02.3814070Z 
2019-12-09T17:02:02.3814261Z 
2019-12-09T17:02:02.3815930Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-12-09T17:02:02.3816329Z 
2019-12-09T17:02:02.3816471Z 
2019-12-09T17:02:02.3816689Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-12-09T17:02:02.3816846Z Build completed unsuccessfully in 0:01:28
2019-12-09T17:02:02.3816846Z Build completed unsuccessfully in 0:01:28
2019-12-09T17:02:02.3821242Z == clock drift check ==
2019-12-09T17:02:02.3821449Z   local time: Mon Dec  9 17:02:02 UTC 2019
2019-12-09T17:02:02.6712912Z   network time: Mon, 09 Dec 2019 17:02:02 GMT
2019-12-09T17:02:02.6715475Z == end clock drift check ==
2019-12-09T17:02:04.1828538Z 
2019-12-09T17:02:04.1948083Z ##[error]Bash exited with code '1'.
2019-12-09T17:02:04.1993454Z ##[section]Starting: Checkout
2019-12-09T17:02:04.1995847Z ==============================================================================
2019-12-09T17:02:04.1995902Z Task         : Get sources
2019-12-09T17:02:04.1995986Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

[rust-highfive]

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-12-10T12:23:49.9762269Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-12-10T12:23:49.9775543Z ##[command]git config gc.auto 0
2019-12-10T12:23:49.9791473Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-12-10T12:23:49.9796219Z ##[command]git config --get-all http.proxy
2019-12-10T12:23:49.9809882Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67177/merge:refs/remotes/pull/67177/merge
---
2019-12-10T12:29:16.6894693Z     Checking term v0.0.0 (/checkout/src/libterm)
2019-12-10T12:29:16.8341870Z error[E0658]: use of unstable library feature 'result_map_or'
2019-12-10T12:29:16.8343108Z   --> src/libterm/terminfo/mod.rs:80:50
2019-12-10T12:29:16.8344361Z    |
2019-12-10T12:29:16.8345130Z 80 |         if term.is_err() && (env::var("MSYSCON").map_or(false, |s| "mintty.exe" == s) ||
2019-12-10T12:29:16.8346396Z    |
2019-12-10T12:29:16.8346396Z    |
2019-12-10T12:29:16.8347121Z    = note: for more information, see ***/issues/66293
2019-12-10T12:29:16.8347862Z    = help: add `#![feature(result_map_or)]` to the crate attributes to enable
2019-12-10T12:29:16.9239554Z error: aborting due to previous error
2019-12-10T12:29:16.9240452Z 
2019-12-10T12:29:16.9241555Z For more information about this error, try `rustc --explain E0658`.
2019-12-10T12:29:16.9317973Z error: could not compile `term`.
---
2019-12-10T12:29:19.6323376Z   local time: Tue Dec 10 12:29:19 UTC 2019
2019-12-10T12:29:19.6926983Z   network time: Tue, 10 Dec 2019 12:29:19 GMT
2019-12-10T12:29:19.6930900Z == end clock drift check ==
2019-12-10T12:29:26.0562927Z 
2019-12-10T12:29:26.0664145Z ##[error]Bash exited with code '1'.
2019-12-10T12:29:26.0694473Z ##[section]Starting: Checkout
2019-12-10T12:29:26.0697040Z ==============================================================================
2019-12-10T12:29:26.0699211Z Task         : Get sources
2019-12-10T12:29:26.0699325Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

[shepmaster]

I don't know much about this area of the code.... so I'll pick someone at random

r? @BurntSushi

[joshtriplett]

Nothing in this pull request seems to negate the extensive discussion already on #27867 that made it clear there was no consensus to make such a change. I don't think this should be merged.

As before, I think it would be perfectly reasonable to respect CLICOLOR, if the spec didn't also suggest the highly-breakage-prone CLICOLOR_FORCE that applications should not support (and without even any documentation of what could potentially go wrong). Having a global way to tell all applications "I don't want color" or "I do want color even if you default to no color" seems perfectly reasonable. A global flag that force-enables color for arbitrary subprocesses seems highly likely to cause breakage.

People will (incorrectly) set this in their shell or project configuration because they want color when piping to their pager, or other similar reasons. And then much later they'll run into strange issues caused by programs generating color codes where not expected.

(As a side note, one potential solution that would work in place of CLICOLOR_FORCE: an environment variable specifying enough information about the pipe or log file that stdout pointed to, such that if and only if an application could confirm that its stdout still pointed to the same place, it would force color when writing to that place. That way, stdout would get colorized when written to the expected place, but not colorized when redirected/piped somewhere else that may not expect color. That approach would avoid errors, but may not be possible on all systems, only those systems where you can uniquely identify a file or pipe based only on an open fd/handle to it in the form of stdout.)

[bors]

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

[bjorn3]

This unnecessarily allocates a string. Something like env::var(...).as_ref().map(|s| &**s).unwrap_or("0") should work. Also I think it would be nicer to use ColorConfig::Auto => { if { ... } else { ... } } instead.

[bjorn3]

It is confusing what the condition is. It is easy to overlook the start and end of (...).

[joshtriplett]

👎 See extensive discussion on #27867.

[Dylan-DPC-zz]

Doesn't seem to be much interest in having this. Closing it. Thanks for taking the time to contribute :)

[jhasse]

Nothing in this pull request seems to negate the extensive discussion already on #27867 that made it clear there was no consensus to make such a change.

That's because the points you made are not something which can be negated with code.

I've answered to your comment and IMHO addressed your concerns. Please answer there instead of rephrasing them here while ignoring my counter arguments.

---

I must say that the way this went down feels VERY frustrating to me. The first PR was closed with the following comment:

I'm going to close this due to inactivity for now. This may want to be discussed externally first (e.g. on the forums) about whether it's a desirable change to push through regardless.

I've done exactly that: #27867 (comment)
But then other people chime in and it doesn't look like discussing this externally first is really the solution.

Doesn't seem to be much interest in having this.

Well, most people just workaround the issue. Or do not care that much about colors to look at open PRs all the time. Maybe some kind of poll could shed light on this?

[Mark-Simulacrum]

I would recommend writing an RFC to add this; it can likely be relatively short, but it would be a good way to build consensus (doing so in PR comments for contentious changes doesn't work too well, in practice, as they do not have good visibility).

[jhasse]

Thanks, I will have a look :)

[joshtriplett]

@jhasse

That's because the points you made are not something which can be negated with code.

This problem cannot be solved solely with code, no. It could, however, be solved with a change to the specification, and corresponding code implementing the new specification.

One possible proposal: an environment variable that specifies the device/inode pair of the file to force sending color to. A program looking to emit color to stdout would call fstat on stdout, and if it's a TTY or if it matches that device/inode, it would emit color to it. (Likewise for stderr.) That would allow the top-level program that wants color output from programs it invokes to set up a file or pipe and set that environment variable accordingly, but then any later use pipes or redirection other than that would not match and would not produce color. I've confirmed that this would work on Linux, on OSX, on FreeBSD, on NetBSD, and I've done enough research to find that something similar should work on Windows (just with a different underlying concept of "fstat" and "device/inode pair").

Please answer there instead of rephrasing them here

I would have continued to answer there, but that issue was closed, and this one was still open without any record of those concerns. (I also don't believe a new issue should have been opened at all. If the problems had been addressed, that issue could have been reopened.) And no, I do not believe you have addressed these concerns.

I must say that the way this went down feels VERY frustrating to me.

I absolutely understand and appreciate that. I have been in that position before, and it can be frustrating to not have an obvious next step. I'm hoping that the above proposal helps. (If you don't like that proposal, please feel free to make an alternative one that solves the same problem.)

I think you received somewhat mixed messages, in this case. I don't believe that the other issue should have been closed due to "inactivity", for instance, as opposed to "lack of consensus for this change".

[jhasse]

Thanks for your answer :) Sorry for mine that was a bit harsh.

[...] but then any later use pipes or redirection other than that would not match and would not produce color.

That CLICOLOR_FORCE=1 gets inherited for later uses is a feature for me - otherwise a command line flag would do the trick already.

[..] I've done enough research to find that something similar should work on Windows (just with a different underlying concept of "fstat" and "device/inode pair").

I thought Windows doesn't use files for ttys? How would it work on Windows?

[joshtriplett]

@jhasse What I'm proposing would get inherited by subprocesses, but only if their output is directed to a specific place (as indicated in the environment variable). You'd be able to direct output to a pipe or file and get color, and any output going to that pipe or file would use color, but any output that a script redirects to a different pipe or file would not use color.

Suppose that grep added support for this. If you ran grep directly, it would produce color. If you run a script that runs grep, with grep's output directed to the script's stdout, it would produce color. If you run a script that runs grep with its output redirected by the script to a pipe or file, it would not produce color, and thus would not break the script.

I thought Windows doesn't use files for ttys? How would it work on Windows?

I'm talking about pipes, here.

This could work on Windows using NtQueryObject with ObjectNameInformation to match against the name of a named pipe, as one possible solution. The environment variable, on Windows, could contain the name of the named pipe.

[jhasse]

Okay, that case is already somewhat covered by isatty. Even if you want to redirect the top-most command to a file, that's already possible, too, with screen.

The currently unsolvable issue comes when scripts do something like this:

rustc foo > tmpfile
cat tmpfile

Doesn't have to be a file, most of the time it's a buffer, e.g. with make --output-sync or ninja.