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

Enable or disable colored output according to CLICOLOR(_FORCE) #27867

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
@jhasse
Copy link
Contributor

jhasse commented Aug 17, 2015

While #14040 already enabled a command line flag to enable or disable colored output, this uses the environment variables CLICOLOR and CLICOLOR_FORCE. CLICOLOR=0 would be --color=never and CLICOLOR_FORCE=1 --color=always.

Why environment variables when there is already a command line switch? When working with build systems, editors or continuous integration servers output almost always gets piped or buffered, so a switch to force colored output is needed. When we agree upon a standard for this, the only thing one would need to do in such a situation is to define CLICOLOR_FORCE=1. I've also created a page with a summary: http://bixense.com/clicolors/ Please tell me what you think :)

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Aug 17, 2015

r? @sfackler

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

@nagisa

This comment has been minimized.

Copy link
Contributor

nagisa commented Aug 17, 2015

Is such environment variable in any way standard-ish? I know to try --color, because that’s a convention (which spread to other tools) from GNU tools. I personally have a bunch of issues with the idea as it is outlined in the linked page, but currently haven’t enough time to list them out.

Moreover, this change only touches libsyntax, whereas we use coloured output everywhere from the compiler itself (probably covered by libsyntax) to test runner (most likely not covered by libsyntax).

@jhasse

This comment has been minimized.

Copy link
Contributor

jhasse commented Aug 18, 2015

Is such environment variable in any way standard-ish?

Depends on what you mean. OS X uses them (according to my google research,
I don't have a Mac to test it out).

I know to try --color, because that’s a convention (which spread to other
tools) from GNU tools.

The problem of a command line argument is, that it doesn't get propagated
to child processes. E. g. make --color=always doesn't color my gcc / rustc
output.

I personally have a bunch of issues with the idea as it is outlined in
the linked page, but currently haven’t enough time to list them out.

I would love to hear them as this is something I'm dedicated to fix :)

Moreover, this change only touches libsyntax, whereas we use coloured
output everywhere from the compiler itself (probably covered by libsyntax)
to test runner (most likely not covered by libsyntax).

I see. I will look into the test runner then (might take a couple of days).

@nagisa

This comment has been minimized.

Copy link
Contributor

nagisa commented Aug 18, 2015

The problem of a command line argument is, that it doesn't get propagated to child processes. E. g. make --color=always doesn't color my gcc / rustc output.

You’d usually set CC to gcc -fdiagnostics-color=always and RUSTC to rustc --color=always or something, no?

I would love to hear them as this is something I'm dedicated to fix :)

Forcing Windows to use ANSI colours in any way is not acceptable. Windows has its own colouring mechanism; why not specify this for arbitrary colouring mechanism? Since this “spec” is for ANSI colours, we technically already violate it in rustc by using proper colouring mechanisms on each platform.

This implementation makes CLICOLOR=0 an equivalent for --color=never. While I find this behaviour sensible, it is not even noted in the linked summary.

In general, while I’m fine with rustc implementing such things first, I’d feel much better about it if they were more throughout-ly specified.

@jhasse

This comment has been minimized.

Copy link
Contributor

jhasse commented Aug 18, 2015

You’d usually set CC to gcc -fdiagnostics-color=always and
RUSTC to rustc --color=always or something, no?

As you can see the --color option isn't standarized, so I have to
check how it's called in every program. Also some build scripts set
CC=clang to switch the compiler, which would deactivate colors again.
And you forgot about CXX. Furthermore some build systems (like SCons)
don't check these variables at all.

The advantage of CLICOLOR(_FORCE) would be, that programs like a text
editor or a ci server which pipe the build, could set them
automatically when they support parsing colors, no further
configuration necessary.

Forcing Windows to use ANSI colours in any way is not acceptable.
Windows has its own colouring mechanism; why not specify this for
arbitrary colouring mechanism?

This isn't forcing Windows users to use ANSI colors. Windows users who
don't want to use them aren't affected by this. But this actually
improves the situation for Windows users who are using MSYS2: The
MSYS2 terminal supports ANSI colors, but not terminfo. So rustc
currently doesn't automatically output in color, which could be fixed
with setting CLICOLOR=1.

Since this “spec” is for ANSI colours, we technically already violate it in rustc by using proper colouring mechanisms on each platform.

The spec only tries to standardize what should happen when CLICOLOR
and CLICOLOR_FORCE are set. If they aren't, nothing needs to change.

This implementation makes CLICOLOR=0 an equivalent for --color=never. While I find this behaviour sensible, it is not even noted in the linked summary.

Good point, I've added it in the spec. Thanks :)

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Aug 19, 2015

I'm also slightly uncomfortable starting to look at some relatively-arbitrary environment variables as well, especially when we have a number of tools to update (as @nagisa mentions, and there's also Cargo).

@Turbo87

This comment has been minimized.

Copy link
Member

Turbo87 commented Sep 24, 2015

FWIW the most popular tool in the Node.js community is using FORCE_COLOR instead (see https://github.com/chalk/supports-color/blob/master/index.js#L72 and chalk/supports-color#18)

@jhasse

This comment has been minimized.

Copy link
Contributor

jhasse commented Sep 24, 2015

Thanks for the hint! I've created an issue here: chalk/supports-color#32

Another name is also fine, but I really hope that it isn't Rust specific, so that Jenkins (with the ANSI plugin) and other tools (atom-build, ninja, ...) would display color out-of-the-box.

@jhasse jhasse force-pushed the jhasse:patch-clicolor branch from a98cb6a to 5fa7f52 Oct 22, 2015

@jhasse

This comment has been minimized.

Copy link
Contributor

jhasse commented Oct 22, 2015

Rebased and added a new commit which makes sure that only ANSI color sequences are emitted when CLICOLOR(_FORCE) is set and not what is returned by terminfo. This finally enables colored output in atom-build :)

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 23, 2015

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

@jhasse jhasse force-pushed the jhasse:patch-clicolor branch from 5fa7f52 to 093102b Nov 23, 2015

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 3, 2015

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

@jhasse jhasse force-pushed the jhasse:patch-clicolor branch from 093102b to c121250 Jan 6, 2016

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 27, 2016

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.

@jhasse

This comment has been minimized.

Copy link
Contributor

jhasse commented Jan 27, 2016

Please reopen, I've put work into it every time it became unmergeable.

Btw: CMake merged my patch a few months ago: https://cmake.org/Bug/view.php?id=15789

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 27, 2016

@jhasse I don't think it's clear here that there's consensus to do this, and at this point I don't think that we'll be able to get consensus at this location (this was inactive without comment for ~5 months). It sounds like at this point we'll want to discuss ahead of time, and the forums are currently the best place to do that.

@jhasse

This comment has been minimized.

Copy link
Contributor

jhasse commented Jan 27, 2016

@alexcrichton Can you open a thread in the forums? I don't know exactly how that thread should look like (also never used the forums).

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 27, 2016

Ah we currently have a discourse instance over at https://users.rust-lang.org/ and I think you should be able to use your github account to open a thread

@jhasse

This comment has been minimized.

@jhasse

This comment has been minimized.

Copy link
Contributor

jhasse commented Feb 15, 2016

There's been one (positive) response to the forum thread.

Can this PR be reopened and can you tell me what is missing for this to be merged? Thanks! :)

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 22, 2016

cc @rust-lang/tools, do others have opinions here?

@jhasse I personally feel that there's so little activity on this (and the thread on the forums) that it's not enough to push forward with this as a general convention, but others may feel differently.

@colin-kiegel

This comment has been minimized.

Copy link

colin-kiegel commented Feb 22, 2016

I would like it. :-)

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Feb 25, 2016

On first glance, I'm disinclined to do this. Can't RUSTFLAGS solve this?

@jhasse

This comment has been minimized.

Copy link
Contributor

jhasse commented Feb 25, 2016

The problem with RUSTFLAGS is, that it's Rust specific. This PR is actually solving two real world problems I'm having: The first one is rustc's colored output not working in atom-build and the second one is rustc's colored output not working in AppVeyor. Both could be fixed if this was merged. I could then just set CLICOLOR_FORCE=1 for atom-build and AppVeyor (yes, AppVeyor supports ANSI escape codes). The advantage of a generic name instead of using RUSTFLAGS is, that I don't need to look up how every single program does it. For example if I also wanted to add something build by CMake in my atom-build/AppVeyor script, colored output would work out of the box.
In the ideal future atom-build and AppVeyor would have already set the environment variables for me. I'll will open feature requests for that as soon as more programs support CLICOLOR_FORCE (it's kind of a chicken-egg problem atm).

@jhasse

This comment has been minimized.

Copy link
Contributor

jhasse commented May 23, 2016

Any news on this? If you like, we could also discuss this on IRC.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented May 26, 2016

One other potential issue with this (an issue that also applies to GNU make's MAKE_TERMOUT and MAKE_TERMERR environment variables, documented at https://www.gnu.org/software/make/manual/make.html#Terminal-Output): what happens if someone sets CLICOLOR_FORCE or MAKE_TERMOUT to indicate that output will (eventually) go to a terminal for output, but then some portion of the build system actually does want to redirect output to a file or pipe that won't eventually get printed to a terminal? Does that part of the build system then need to explicitly unset CLICOLOR_FORCE and MAKE_TERMOUT and any other variable the program might potentially respect, to make sure it won't include color in its output? That seems likely to produce breakage.

Consider if grep respected CLICOLOR_FORCE or MAKE_TERMOUT. What would happen if a build system set one of those, and then somewhere deep in the build system (or even in a shell script called from the build system), someone ran grep in a pipeline, along with various other text processing commands? If grep respected those environment variables, then that pipeline would break.

I do like the idea of having a standard way to tell a program "no, really, your output will wind up on the terminal, please print in color even though it doesn't look like a TTY". However, I don't think it makes sense to do so without a solution for this problem. Not showing color output is an annoyance; breaking a build by printing color escape sequences to a pipe would be far worse.

(This is also part of why programs like grep are removing environment variables like GREP_OPTIONS: they break scripts.)

@jhasse

This comment has been minimized.

Copy link
Contributor

jhasse commented May 31, 2016

The issue would be smaller if there was only one enivorment variable (CLICOLOR_FORCE) you'd have to think about. That's why I think a standard is needed, as other variables already exist (CLANG_FORCE_COLOR_DIAGNOSTICS, FORCE_COLOR, ...) and it's hard to keep track of all of them.

The Unix tools on OS X already respect CLICOLOR_FORCE, so you'll have to already keep that in mind in some cases.

Not showing color output is an annoyance; breaking a build by printing color escape sequences to a pipe would be far worse.

It isn't worse for me. Breaking a build is an annoyance which I can fix with a CLICOLOR_FORCE=0 (not atm because of all the variables, but in a perfect world). Not showing colored output on the other hand makes reading output messages unbearable for me.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented May 31, 2016

I'd certainly like to see a single variable like CLICOLOR that I can set to enable autodetection (to the extent that programs don't just default to it), and so that people who dislike color output can easily disable it. However, I think forcing color via an environment variable will cause too many problems once more programs start looking at that variable, because the variable will persist in the environment even when redirecting output to somewhere that will not end up on a terminal.

And while it's certainly possible that if you set CLICOLOR_FORCE=1 yourself you'd think to set it to 0, the possibility of such breakage would mean programs like make could never set it automatically (the way make currently sets MAKE_TERMOUT).

As an alternative solution, programs like make could collect output using a pty rather than a pipe or temp file; that would give the programs it spawns an actual try, so autodetection via isatty would Just Work.

@placrosse

This comment has been minimized.

Copy link

placrosse commented Aug 16, 2016

A decent percentage of prospective Rust users will come from "full featured" IDE environment. Atom is able to provide a fairly nice setup, but the loss of color output is enough to cause people to use a separate terminal window for running cargo, in order to get the colorized text output. This leads to a less integrated experience (which, admittedly, is a minus for some, and a plus for others). If there is a simple, universal means to turn the color on, such that it works in as many arenas Rust apps may be build as possible, it will be an overall win.

@flying-sheep

This comment has been minimized.

Copy link

flying-sheep commented Jan 11, 2017

“just pretend to be a TTY” won’t work, as some programs will then (sensibly) assume interactivity and maybe prompt for input.

as of now, clang and OSX tools support CLICOLOR[_FORCE], and the wealth of node tools the (less intuitive) FORCE_COLOR.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Jan 11, 2017

“just pretend to be a TTY” won’t work, as some programs will then (sensibly) assume interactivity and maybe prompt for input.

Tools used in an automated build shouldn't never prompt interactively; the build scripts should pass them options that answer any questions they might want to ask.

@jhasse

This comment has been minimized.

Copy link
Contributor

jhasse commented Jan 11, 2017

Tools used in an automated build shouldn't never prompt interactively; the build scripts should pass them options that answer any questions they might want to ask.

They use isatty to find out if it's okay to prompt.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Jan 11, 2017

@jhasse Many people run builds and don't expect those builds to prompt in the middle. Tools usable from automated scripts should always have corresponding command-line options to specify anything they might otherwise want to prompt for, and build scripts should always specify those options.

Also, tools that want to prompt should check isatty(0) (stdin), and automated builds can connect stdout/stderr to a pty while connecting stdin to /dev/null.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment