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

Allow to configure colors using an environment variable #3873

Closed
wants to merge 2 commits into from

Conversation

weiznich
Copy link
Contributor

This commit add support for configuring colors used by cargo through a
environment variable (CARGO_COLORS). The used syntax for the environment
variable is similar to that one used by gcc (GCC_COLORS).

Example: CARGO_COLORS="status=01;2:warn=01;3:error=01;1:default:01;231:blocked=01;4"

The current state should be seen as a proof of concept and demo to see if this functionality is wanted. I've only tested the code on linux so far.
For a final implementation the interpretation of the escape codes may be improved.

This commit add support for configuring colors used by cargo through a
environment variable (CARGO_COLORS). The used syntax for the environment
variable is similar to that one used by gcc (GCC_COLORS).

Example:
CARGO_COLORS="status=01;2:warn=01;3:error=01;1:default:01;231:blocked=01;4"
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (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.

@bors
Copy link
Contributor

bors commented Mar 31, 2017

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

@alexcrichton
Copy link
Member

Hm this seems like it'd be very unix/linux specific rather than being general to all platforms? This also seems like something that I think that .cargo/config should support perhaps? (IIRC it may already?)

@weiznich
Copy link
Contributor Author

weiznich commented Apr 1, 2017

This should work on all platforms that uses ANSI escape codes. Which should basic mean everything except cmd.exe. (As noted I only tested it using zsh on linux, so not entirely sure about this.)
There is an open issue about configuring this through .cargo/config. There was also an pull request for this, that got closed.
Also in my opinion configuring the colours through .cargo/config is not perfect, because sometimes I'm working with different terminals on the same project. Those terminals use different color configurations. Configuring those colors through .cargo/config would mean that I could only adjust the output for only one terminal, but not for both at the same time. Then I remember that gcc implemented this through GCC_COLORS that let adjust you the output colors on base of the environment.

@alexcrichton
Copy link
Member

Ah ok, thanks for the clarification and investigation! Seems like a reasonable idea to configure this for multiple kinds of terminals.

I'm somewhat wary though of adopting this specific syntax because it seems quite alien on the surface (I can't parse the string above). At the very least we'll need to document this environment variable as well as the syntax associated with it before landing. I wonder if there are perhaps other options for configuring this?

Oh I'll also note that all .cargo/config configuration values should be specify-able through the environment, so if we push the .cargo/config style of configuration it'd still have per-terminal configs available.

@weiznich
Copy link
Contributor Author

weiznich commented Apr 3, 2017

I'm somewhat wary though of adopting this specific syntax because it seems quite alien on the surface (I can't parse the string above). At the very least we'll need to document this environment variable as well as the syntax associated with it before landing. I wonder if there are perhaps other options for configuring this?

I'm rather uncreative with such stuff. So if anyone knows a better syntax for this I'm happy to change it. Using color names instead of the underlying ANSI escape codes does not really work, because the color name would be hard coded to an specific escape code, but the color of those escape codes is configurable by the terminal. (For example one could configure the color named yellow in this schema to a green color.) So this would be rather misleading.
On the other hand: To configure the colors of the escape codes away from the standard one must know about those codes. So this person should also be able to use the rather cryptic syntax above.

Oh I'll also note that all .cargo/config configuration values should be specify-able through the environment, so if we push the .cargo/config style of configuration it'd still have per-terminal configs available.

Great. Should I open a issue about this?

@alexcrichton
Copy link
Member

Hm ok, that makes sense. Is there documentation for what all these codes are?

As for the syntax and whatnot, I think I'd be in favor of supporting something like this in cargo configuration:

[term]
status = 'green'
error = 'red'
# ...

which could then be configured with

CARGO_TERM_STATUS=green
CARGO_TERM_ERROR=red
# ...

Does that make sense?

@weiznich
Copy link
Contributor Author

weiznich commented Apr 4, 2017

Hm ok, that makes sense. Is there documentation for what all these codes are?

I've used the wikipedia entry (See the table about sgr parameters) and a shell script to check those codes.

#!/bin/bash

echo
echo -e "$(tput bold) reg  bld  und   tput-command-colors$(tput sgr0)"

for i in $(seq 1 255); do
   echo " $(tput setaf $i)Text$(tput sgr0) $(tput bold)$(tput setaf $i)Text$(tput sgr0) $(tput sgr 0 1)$(tput setaf $i)Text$(tput sgr0)  \$(tput setaf $i)"
done

echo ' Bold            $(tput bold)'
echo ' Underline       $(tput sgr 0 1)'
echo ' Reset           $(tput sgr0)'
echo

As for the syntax and whatnot, I think I'd be in favor of supporting something like this in cargo configuration:

[term]
status = 'green'
error = 'red'
# ...

which could then be configured with

CARGO_TERM_STATUS=green
CARGO_TERM_ERROR=red
# ...

Does that make sense?

As already noted: This only worked fine as long as the terminal does render those escape codes in the default way. But terminals (or better users) could change this.
See the following two Examples:
Alacritty using some default configuration:
alacritty
Gnome terminal with an (manually) adjusted color profile to show the problem
gnome_terminal

As far as I understand there seems no way to tell the terminal/shell: "This should be green". One could only tell: "This should be styled using the style number x". Even with this we could theoretically introduce names for those styles as proposed by you (Meaning green maps to style 2 or so). In practice I think it would be rather surprising for user if the he configures the status messages to be green and the resulting output only contains blue status messages.
I'm not saying that using the raw style codes is a better way. I think this is actually a pretty bad solution, but I do not now any better one. At least this will not hide the underlying details from anybody. So there will be surprising for somebody.

@weiznich
Copy link
Contributor Author

weiznich commented Apr 8, 2017

I'm a few weeks abroad, so I'm not able to work in this time on this. I will continue the development as soon as I returned. (Feel free to close the pull request in the mean time.)

@alexcrichton
Copy link
Member

Ok, no worries!

One thing I also remembered last night is that we probably don't want to only do this in Cargo if we do it. Lots of Rust tools do color: rustc, cargo, rustup, etc. We'd want to be sure to update all of them, not just one.

@alexcrichton
Copy link
Member

I'm going to close this due to inactivity for now, but feel free to resubmit with a rebase!

@weiznich
Copy link
Contributor Author

weiznich commented Jun 7, 2017

The feature branch is now rebased. Should I submit a new pull request or do you want to reopen this one to save the comment history?

One thing I also remembered last night is that we probably don't want to only do this in Cargo if we do it. Lots of Rust tools do color: rustc, cargo, rustup, etc. We'd want to be sure to update all of them, not just one.

Right, but we must start at one place. Maybe we could reuse the code somewhere else?

@alexcrichton
Copy link
Member

Sure yeah, want to open a new PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants