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

Breaking color issues in terminal #7658

Closed
memsharded opened this issue Sep 4, 2020 · 9 comments
Closed

Breaking color issues in terminal #7658

memsharded opened this issue Sep 4, 2020 · 9 comments
Milestone

Comments

@memsharded
Copy link
Member

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 running conan --version and now it no longer works.

Originally posted by @ytimenkov in #7154 (comment)

@memsharded
Copy link
Member Author

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.

We explicitly set CONAN_COLOR_DISPLAY=0 when running conan --version and now it no longer works.

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 required_conan_version in conan.conf to enforce it.

@ytimenkov
Copy link
Contributor

@memsharded Thanks for taking my rant seriously 🙇

It's easy to reproduce:

env CLICOLOR_FORCE=1 CONAN_COLOR_DISPLAY=0 conan --version | cat

Before it was raw text, now it's ANSI-colored. Thing is those variables were set for different purposes: CLICOLOR_FORCE was set to get color output from the build tool in general while CONAN_FORCE_COLOR=0 was added only when querying the version to get machine-readable text.

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 CONAN_FORCE_ANSI_COLORS which should behave exactly as CLICOLOR_FORCE=1 now).

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 required_conan_version in conan.conf to enforce it.

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 NO_COLOR), but I've fixed this on our side and I like new behavior more, so I suggest keeping things as they are now.

@ytimenkov
Copy link
Contributor

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.

@johan-boule
Copy link

johan-boule commented Oct 21, 2023

Current situation:

This is the list of variables that influence Conan, in order: NO_COLOR CLICOLOR_FORCE CLICOLOR CONAN_COLOR_DISPLAY PYCHARM_HOSTED

  • if NO_COLOR is set to whatever value, colors are disabled.
  • if CLICOLOR_FORCE is nonblank/nonzero, then colors are enabled regardless of whether stdout/err is a TTY, and also it disables any potential VT100=>Pre-MSWin10 conversion.
  • if CLICOLOR is 0, then colors are disabled.
  • if CLICOLOR is nonblank/nonzero and stdout is not a TTY, then colors are disabled.
  • if CLICOLOR is nonblank/nonzero and stdout is a TTY, then colors are enabled.
  • if CONAN_COLOR_DISPLAY is 0, then colors are disabled.
  • if CONAN_COLOR_DISPLAY is nonblank/nonzero, then colors are enabled regardless of whether stdout/err is a TTY (and if PYCHARM_HOSTED is set to whatever value, it disables any potential VT100=>Pre-MSWin10 conversion).
  • if stdout is a TTY, then colors are enabled (and if PYCHARM_HOSTED is set to whatever value, it disables any potential VT100=>Pre-MSWin10 conversion).

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:

  • CLICOLOR_FORCE=1 or CLICOLOR=1 takes precedence over CONAN_COLOR_DISPLAY=0.
  • CLICOLOR_FORCE=1 breaks Pre-MSWin10 terminal.
  • CLICOLOR=1 breaks JetBrain's PyCharm terminal.

Proposed fix:

I think the rules should be changed so a veto wins (i.e. a No wins over a Yes).

First the Noes:

  • if NO_COLOR is nonblank, colors are disabled.
  • if CLICOLOR is 0, then colors are disabled.
  • if CONAN_COLOR_DISPLAY is 0, then colors are disabled.

Then the Ayes:

  • if CLICOLOR is nonblank/nonzero and stdout is a TTY, then colors are enabled.
  • if CLICOLOR_FORCE is nonblank/nonzero, then colors are enabled regardless of whether stdout/err is a TTY.
  • if CONAN_COLOR_DISPLAY is nonblank/nonzero, then colors are enabled regardless of whether stdout/err is a TTY.
  • if stdout is a TTY, then colors are enabled.

Notice the following are blank votes, they have no effect whatsoever:

  • CLICOLOR=1 means "I'm not opposed to colors, no daltonism."
  • CLICOLOR_FORCE=0 means "I'm not forcing anybody."

With the rules above, NO_COLOR=1, CLICOLOR=0, or CONAN_COLOR_DISPLAY=0 would all take precedence over CLICOLOR_FORCE=1.

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 PYCHARM_HOSTED which JetBrain invented has a dirty kluge, rather than TERM, why ? Doesn't PyCharm set TERM ? It seems to me colorama.init(strip=False) should force colors without TTY, while still retaining minimal TERM detection to decide when doing convert=False. If not, that's a colorama bug IMHO. I can't see a situation where you have say TERM=xterm and you'd want to use Win32 console API!

Other known bugs:

When launching Conan through winpty on Windows within a mintty terminal, the background becomes pink. It's atrocious on the eyes.

So, when I don't need an interactive prompt, I don't run it through winpty, but, then Conan can't detect any TTY, so it doesn't do color.

So, I force it to use colors with CONAN_COLOR_DISPLAY=1, but then it does the VT100=>Pre-MSWin10 conversion which doesn't make sense on the mintty VT100 terminal.

So, to force it to NOT do the VT100=>Pre-MSWin10 conversion, I have to set PYCHARM_HOSTED to whatever value.

Now comes a bug: you can't really disable color anymore with CONAN_COLOR_DISPLAY=0 or CLICOLOR=0 or NO_COLOR.

$ PYCHARM_HOSTED=1 CONAN_COLOR_DISPLAY=0 conan config home | cat -A
C:\Users\jboule\.conan^M$
^[[0m

The VT100 escape sequence SGR reset ESC + [0m should not be present. In fact, the same command on a tmux terminal in linux does not print it.

This badly breaks the parsing of the output for commands like conan --version, conan config home, conan config get, etc.

The current workaround for mintty is to not set PYCHARM_HOSTED and CONAN_COLOR_DISPLAY=1, and instead set CLICOLOR_FORCE=1, this will bypass the VT100=>Pre-MSWin10 conversion.
But this means to make parsing like home=$(conan config home) work, only NO_COLOR=1 will correctly disable colors, and neither CLICOLOR=0 nor CONAN_COLOR_DISPLAY=0 will work anymore (due to bad precedence rules described above).

In the end, if you have a script that does home=$(conan config home),
for robustness, it has to be replaced with home=$(env -u PYCHARM_HOSTED -u CLICOLOR_FORCE NO_COLOR=1 conan config home).

@memsharded
Copy link
Member Author

This badly breaks the parsing of the output for commands like conan --version, conan config home, conan config get, etc.

  • conan config home already prints to stdout without colors in Conan 2.0
  • conan config get has been removed in Conan 2.0
  • There is a new way to automatically get the Conan version without parsing in Conan 2.0 conan version --format=json (and getting the version item)

So there are 2 different issues in this thread: output that shouldn't be using colors at all, like conan --version (lets change that), and then using different env-vars to activate/deactivate/force colors.

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 --format=json output. So while we are also willing to improve the definition of activation of colors, it should be noted that some output printing strange color characters should never be a reason for breaking user CI or automated scripts (and if it is, like the exception of conan --version, we will change it and remove colors).

@memsharded
Copy link
Member Author

memsharded commented Oct 23, 2023

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:

  • If CLICOLOR_FORCE is defined to anything else than "0", colors will be activated and forced (highest priority)
  • If NO_COLOR is defined, that colors will be disabled

I think this should be simpler and easier to understand logic.

@memsharded memsharded added this to the 2.0.14 milestone Oct 23, 2023
@memsharded
Copy link
Member Author

#15002 has been merged.

I think that Conan 2.0 CLICOLOR_FORCE and NO_COLOR should be enough to define the desired behavior, so this issue could be closed.

@memsharded
Copy link
Member Author

I will add these variables description to the docs and close this ticket as solved in 2.0.
Thanks for the feedback!

@memsharded
Copy link
Member Author

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

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

No branches or pull requests

3 participants