-
Notifications
You must be signed in to change notification settings - Fork 990
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
Breaking color issues in terminal #7658
Comments
Hi @ytimenkov If this is a regression, lets have a look and fix it. We merged this because we thought it had very low risk of breaking, all the changes seemed opt-in, and at the end of the day it is just changing the UX, it was unlikely that it would break a build.
Could you please clarify? I have tried: $ conan --version
Conan version 1.30.0-dev # in green color
$ CONAN_COLOR_DISPLAY=0
$ conan --version
Conan version 1.30.0-dev # no color
$ conan --version > version.txt # file without escape sequence Please let us know the steps to reproduce the regression and we will try to fix it asap. Hint: if you are trying to automate in your CI or somewhere and force a specific version, you don't need to parse this output, you can use the |
@memsharded Thanks for taking my rant seriously 🙇 It's easy to reproduce:
Before it was raw text, now it's ANSI-colored. Thing is those variables were set for different purposes: As you can see the problem is when there are multiple conflicting variables and the order in which Conan processes them. There were like 2 before and PR introduces 3 more with Well, they didn't conflict before, and there is always risk when you use something non-namespaced (that's why somewhere I suggested introducing a completely new one, like
Yes, I automate this on CI, but I do version detection to use new features, so same script works with multiple versions, so just minimum doesn't solve all the problems. Probably it would not break if new logic was applied after the original one (well, maybe except for |
Can't close the issue... Another thought is that documentation could be be improved. Something like adding a section "controlling output colorst" describing all variables and their interaction in one place. Also links in the commit message could be referred, they're useful. |
Current situation:This is the list of variables that influence Conan, in order:
Note that Conan doesn't color stdout and stderr separately and only tests whether stdout is a TTY, so, stderr will be colored iif stdout is colored. Problems:
Proposed fix:I think the rules should be changed so a veto wins (i.e. a No wins over a Yes). First the Noes:
Then the Ayes:
Notice the following are blank votes, they have no effect whatsoever:
With the rules above, When enabling colors, we should always test whether the environment asks for the VT100=>Pre-MSWin10 conversion to be disabled. Currently, the test is with Other known bugs:When launching Conan through So, when I don't need an interactive prompt, I don't run it through So, I force it to use colors with So, to force it to NOT do the VT100=>Pre-MSWin10 conversion, I have to set Now comes a bug: you can't really disable color anymore with
The VT100 escape sequence SGR reset ESC + This badly breaks the parsing of the output for commands like The current workaround for mintty is to not set In the end, if you have a script that does |
So there are 2 different issues in this thread: output that shouldn't be using colors at all, like It is very important to highlight that in Conan 2.0, except very limited cases, all the terminal regular text output is not intended to be parsed, and it can change at any time. Most commands are providing now structured |
Regarding the variables, this should be reviewed for Conan 2.0, that has greatly simplified the logic: def color_enabled(stream):
"""
NO_COLOR: No colors
https://no-color.org/
Command-line software which adds ANSI color to its output by default should check for the
presence of a NO_COLOR environment variable that, when present (**regardless of its value**),
prevents the addition of ANSI color.
CLICOLOR_FORCE: Force color
https://bixense.com/clicolors/
"""
if os.getenv("CLICOLOR_FORCE", "0") != "0":
# CLICOLOR_FORCE != 0, ANSI colors should be enabled no matter what.
return True
if os.getenv("NO_COLOR") is not None:
return False
return is_terminal(stream) Only 2 variables are used now:
I think this should be simpler and easier to understand logic. |
#15002 has been merged. I think that Conan 2.0 |
I will add these variables description to the docs and close this ticket as solved in 2.0. |
The Conan 2.0 env-vars meaning and precedence is already documented in https://docs.conan.io/2/reference/environment.html#terminal-color-variables, closing the ticket |
Funny thing: you were so afraid to break customers with my PRs so you finally broke us :( By introducing a lot of new variables with extremely complicated interacton and overriding...
As PR mentiones those variables are used by other tools used by Conan so we set
CLICOLOR_FORCE
to 1 and this breaks parsing conan output, its--version
as it becomes populated with ANSI-sequences.We explicitly set
CONAN_COLOR_DISPLAY=0
when runningconan --version
and now it no longer works.Originally posted by @ytimenkov in #7154 (comment)
The text was updated successfully, but these errors were encountered: