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

Implement a way to enable color in console even when the output is redirected #33980

Closed
ejsmith opened this issue Mar 23, 2020 · 65 comments · Fixed by #47935
Closed

Implement a way to enable color in console even when the output is redirected #33980

ejsmith opened this issue Mar 23, 2020 · 65 comments · Fixed by #47935

Comments

@ejsmith
Copy link

ejsmith commented Mar 23, 2020

Currently, console color is disabled by checking to see if the console output is redirected or not.

I understand why this was done because there are scenarios that don't support ANSI escape sequences and the output looks really ugly in those scenarios when a bunch of non-rendered ANSI escape sequences are mixed into the output of the CLI.

There is a standard that a lot of other CLI systems use to disable these ANSI escape sequences using an environment variable called NO_COLOR here: https://no-color.org/

This may seem trivial, but when all CI systems like Jenkins, AppVeyor, GitHub Actions and pretty much every single other CI system simply record the redirected console output of the build resulting in a big blob of monotone text and a human is expected to look at that output and quickly identify the issues then it becomes a big problem for usability with .NET Core as a whole.

I am proposing that .NET Core is changed to enable ANSI color sequences by default and that it checks for the NO_COLOR environment variable to disable them.

This issue is being created after a discussion about this topic here: microsoft/vstest#2370

@ejsmith
Copy link
Author

ejsmith commented Mar 23, 2020

Also, I think it is relevant to show what other CLI tools are doing to detect color support in the NodeJS world. Here is the supports-color NPM package: https://github.com/chalk/supports-color/blob/master/index.js

@ejsmith
Copy link
Author

ejsmith commented Mar 23, 2020

Was thinking that the most likely reason why this was introduced in the first place is that, until recently, Windows didn't even support ANSI escape sequences. So maybe this could be implemented as a smart default where it's automatically turned it off on Windows versions less than 10.10586 which is the first build of Windows that started supporting ANSI.

@NickCraver
Copy link
Member

I think there are a lot of problems flipped from "default off" to "default on" that would break a lot of systems. I suggest re-titling the issue to be a bit broader to not narrow into a specific fix, e.g. "Allow a way to enable dotnet colorized output, even if console output is redirected". That's really what we're trying to solve - and for .NET that generally needs to be in a non-breaking way.

This could be an environmental variable, or just like on the NO_COLOR site it could be dotnet * --color=true (or whatever is semantically "familiar") as a CLI option...or something else :)

@ethomson
Copy link

I think there are a lot of problems flipped from "default off" to "default on" that would break a lot of systems. I suggest re-titling the issue to be a bit broader to not narrow into a specific fix, e.g. "Allow a way to enable dotnet colorized output, even if console output is redirected". That's really what we're trying to solve - and for .NET that generally needs to be in a non-breaking way.

I agree. When the output is not a tty (or "when the console output is redirected") is often because you're writing to a file. You would almost never want inband control characters / ANSI color codes in that output. Running in a CI system is the exception here, not the rule.

You may want to honor NO_COLOR for interactive use - that would be a welcome improvement for people (like me) who started using text consoles before we had these high-falutin' colors and likes it that way. (Also, please get off my lawn.) But NO_COLOR is - as best I can understand - about interactive use, not redirected use / when isatty is false.

For non-interactive use, you really must default to no colors and then opt-in the CI systems explicitly. As a CI vendor, this is what I want and expect you to do for our customers in common.

Mentioned in microsoft/vstest#2370:

I think introducing heuristics similar to Chalk's is quite dangerous as they will become maintenance hell and will effectively turn dotnet into a bottleneck when new CIs are introduced/need to be added.

Yes, this is true. (Though I'd suggest abstracting it out into a separate package.) As a CI vendor, we want to make sure that our shared users have colors. We will take on this burden of contributing support.

@ethomson
Copy link

Speaking of a separate package - I'm like 99% sure that cake already has this logic to detect when it's running in CI. Whether that's already abstracted out into a separate package or could be, I don't know, but it may be worth looking there first.

@niemyjski
Copy link

niemyjski commented Mar 24, 2020

I'd think in 2020, we'd be smart enough to detect our environments and show color automatically for a really nice experience instead of opting into it. I don't opt into it in other programming languages, it just works.

Also, the cool kids have color and we need it too 💯

@am11
Copy link
Member

am11 commented Mar 24, 2020

For automatic CI detection, here is a list of environment variables from different services:

Variable Check value Service(s)
CI /TRUE/i or 1 TravisCI, AppVeyor, CirrusCI, CircleCI, GitLab CI
TF_BUILD /TRUE/i Azure DevOPS
DOCKERFILE_PATH non empty string Docker Hub CI
GITHUB_SHA non empty string GitHub Actions
JENKINS_URL non empty string Jenkins
bamboo.buildKey non empty string Bamboo
TEAMCITY_VERSION non empty string TeamCity

@ejsmith
Copy link
Author

ejsmith commented Mar 24, 2020

I think there are a lot of problems flipped from "default off" to "default on" that would break a lot of systems. I suggest re-titling the issue to be a bit broader to not narrow into a specific fix, e.g. "Allow a way to enable dotnet colorized output, even if console output is redirected". That's really what we're trying to solve - and for .NET that generally needs to be in a non-breaking way.

I understand that the bar is high for breaking changes in .NET, but not sure that this is actually a breaking change. It would end up being ugly output in the few (IMO) cases where ANSI is not supported. I think it's REALLY important for .NET to have a great experience out of the box for people. The CLI is for developers to use and for developers to look at the output and be able to easily interpret it. I would think that the cases where you wouldn't want ANSI colors are far far less than the cases you do want it. A normal developer is not going to know they need to opt into having color. So we would either need all CI services to agree on some environment variable or we would need the code to check for all of the known CI services which as someone pointed out in the other thread seems pretty hacky and not something that I think the .NET team is going to want in their code.

@ejsmith
Copy link
Author

ejsmith commented Mar 24, 2020

Yes, this is true. (Though I'd suggest abstracting it out into a separate package.) As a CI vendor, we want to make sure that our shared users have colors. We will take on this burden of contributing support.

I am pretty sure that the .NET team isn't going to add a nuget package for something in the core runtime code. So that leaves us with adding code that needs to be maintained to detect all of the known CI services which seems also unlikely that the .NET team would be OK with in the core runtime code.

That is why I think whatever we do it's going to have to be kept simple. Either an opt-in or an opt-out command line flag and an environment variable. I personally feel like it should be opt-out or some sort of smart default.

@ejsmith ejsmith changed the title Use NO_COLOR environment variable for disabling console color Implement a way to enable color in console even when the console output is not redirected Mar 24, 2020
@ejsmith ejsmith changed the title Implement a way to enable color in console even when the console output is not redirected Implement a way to enable color in console even when the output is not redirected Mar 24, 2020
@ethomson
Copy link

I personally feel like it should be opt-out or some sort of smart default.

Smart defaults are the status quo: color on the console, no color when going to log files. This means that interactive users get delightful colors (yay!) and nobody gets ANSI color codes spammed all over their log files (boo).

What you want is an option to turn off colors on the console (the NO_COLOR environment variable approach). And an option to enable colors when the output is redirected. Or more accurately, something that isn't a tty, like a CI system.

(The title of the issue is misleading - at the moment, color is always enabled in the console when the output is not redirected. microsoft/vstest#2370 is asking to enable color when the console is redirected.)

@ejsmith ejsmith changed the title Implement a way to enable color in console even when the output is not redirected Implement a way to enable color in console even when the output is redirected Mar 24, 2020
@ejsmith
Copy link
Author

ejsmith commented Mar 24, 2020

(The title of the issue is misleading - at the moment, color is always enabled in the console when the output is not redirected. microsoft/vstest#2370 is asking to enable color when the console is redirected.)

Fixed. Thanks!

@tannergooding
Copy link
Member

CC. @carlossanlop @eiriktsarpalis

@ejsmith
Copy link
Author

ejsmith commented May 14, 2020

Bump. @davidfowl @DamianEdwards any chance you guys can ask someone to look at this? It would make such a huge difference in quality of life running dotnet from CI services.

@davidfowl
Copy link
Member

cc @stephentoub

@stephentoub
Copy link
Member

stephentoub commented May 14, 2020

Bump

What is the actual request at this point? Is it to respect an environment variable for disabling ANSI sequences when we'd otherwise output them and another for enabling ANSI sequences when we'd otherwise not? Or is it to use ANSI escape sequences on Windows?

Was thinking that the most likely reason why this was introduced in the first place is that, until recently, Windows didn't even support ANSI escape sequences.

The .NET Core implementation today never uses ANSI escape sequences on Windows; rather it uses https://docs.microsoft.com/en-us/windows/console/setconsoletextattribute just as does .NET Framework. Rather, it was added because of complaints that we were spamming log files and the like, and not just in CI.

@ejsmith
Copy link
Author

ejsmith commented May 14, 2020

@stephentoub Ideally we would enable ANSI sequences by default in more scenarios like CI builds. The majority of CI providers (including GitHub Actions) set an environment variable called CI now. If all we did was check for that in ConsolePal.Unix.cs that would be great. I know Windows would need more checks to see if ANSI escape sequences are supported.

Alternatively, if we had a CLI flag to force ANSI support on that would work as well.

@stephentoub
Copy link
Member

I know Windows would need more checks to see if ANSI escape sequences are supported.

More than checks; there's zero code today to emit ANSI escape sequences on Windows ;-)

The majority of CI providers (including GitHub Actions) set an environment variable called CI now.

And they all deal well with ANSI escape sequences?

Has anyone looked to see what go, rust, node, and others do here?

I'm also not convinced this is as easy as just checking for an environment variable. Consider if my app launched another .NET process that wrote to stdout and had its output redirected. Today any use of Console.*Color in that child process is effectively going to be ignored. If we start changing that, either by default or arguably worse only when this "CI" environment variable is set, the app's going to behave differently, maybe only in CI.

@ejsmith
Copy link
Author

ejsmith commented May 15, 2020

I understand that it’s a potentially breaking and not simple. And I also understand that adding a check for env variable seems a bit hacky. I’d be ok with adding a CLI flag and a DOTNET_ env variable to specifically opt into using ANSI. Just really anything other than it being hard-coded to off like it is now.

@stephentoub
Copy link
Member

Thanks. Have you already researched what other languages/frameworks do here?

@ejsmith
Copy link
Author

ejsmith commented May 15, 2020

@stephentoub Checked a few languages:

NodeJS has colors and detects CI using this package supports-color

Rust has colors enabled by default

Deno has colors enabled by default and you have to use a standard environment variable to disable them.

Ideally it would be great if the dotnet CLI had a good experience by default, but again I understand it could be a breaking change and would happily settle for a way to opt in. I think it’s important that it can be done through an env variable so that something like the dotnet GitHub action could turn it on by default in their environment.

@ejsmith
Copy link
Author

ejsmith commented May 21, 2020

@stephentoub if we agree on some direction, I can try to create a PR.

@stephentoub
Copy link
Member

@ejssmith, thanks, somehow I missed your response last week. Let me read up on this a bit and I'll get back to you.

@jzabroski
Copy link
Contributor

Related: dotnet/msbuild#4299

@jeffhandley
Copy link
Member

@KathleenDollard as best as I can tell, this issue is in your court to drive us to conclusions on @stephentoub's questions. I'm going to mark #47935 as a draft until the answers to those questions are clear.

Thanks!

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 18, 2021
@ejsmith
Copy link
Author

ejsmith commented May 19, 2021

Ugh. Guess this isn't going to happen. Sure seems like it would be nice to have some color in our CI log outputs and also sure seems like this is a pretty easy win.

@rbeesley
Copy link

rbeesley commented Jun 1, 2021

@ejsmith, it looks like there are ongoing discussions with System.Console re-design. patching in this change right now might not be the best if there is going to be work to separate the concepts of Console and Terminal. Following the work on conhost.exe and Windows Terminal, that might be the right path forward as color output is part of that discussion.

@ejsmith
Copy link
Author

ejsmith commented Jun 1, 2021

@rbeesley I get that, but that is a ways off and this is literally just a flag that would hold us over until then. Seems like such a shame that all of the other languages have nice colorized easy to scan output, but we are stuck with black and white at least until .NET 7 now.

@rbeesley
Copy link

rbeesley commented Jun 1, 2021

@ejsmith, yup. I would like to have seen it too, for this cycle. Not ideal, but could be worse.

@0xced
Copy link
Contributor

0xced commented Jul 2, 2021

My pull request #47935 has been locked as resolved (it's not) and conversation has been limited to collaborators by a bot.

My last message (on March 6) in that pull request was

I'm waiting for directions before tackling the cache the environment variable evaluation in a static reviewed by Stephen.

I never got any reply, instead the pull request has been marked as draft and eventually closed (by a bot!). That's a great way to discourage external (non-Microsoft) developers to contribute to .NET if you ask me. 😞

@ejsmith
Copy link
Author

ejsmith commented Jul 2, 2021

Yeah, this whole thing seems quite ridiculous. We are basically talking about a flag here and it's been years and multiple major versions and basically no response. This could be a super easy win, but instead it's turned into a big negative.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 2, 2021
@stephentoub
Copy link
Member

@KathleenDollard, I take it there's no interest in an end-to-end coordinated story here around color, driven by the CLI, for consistency across the ecosystem. Given that, we'll just add the limited one-off, Unix-specific env var to Console and call it day. Thanks.

@stephentoub
Copy link
Member

stephentoub commented Jul 2, 2021

quite ridiculous

it's been years and multiple major versions

.NET 5 shipped in November of 2020. What other major version was released since this issue (which is in the 6.0 milestone) was opened in March 2020?

I get that you're frustrated, and I get that the bot auto-closing the PR only added to that. But calling this "ridiculous" and employing hyperbole won't win any points.

My $.02.

@KalleOlaviNiemitalo

This comment has been minimized.

@jzabroski
Copy link
Contributor

@0xced I understand what it's like to have a PR open with Microsoft. I have had some that were open for almost 3 years before they finally got merged (and I thought the project was left for dead, only to discover that PR's acceptance ignited more development from the open source community).

It's difficult to know how people will build upon your work, but I for one am thrilled with your efforts. If you're ever in Boston, I will buy you a beer if you can find me.

@jzabroski
Copy link
Contributor

@KathleenDollard, I take it there's no interest in an end-to-end coordinated story here around color, driven by the CLI, for consistency across the ecosystem. Given that, we'll just add the limited one-off, Unix-specific env var to Console and call it day. Thanks.

I had actually discussed this with Rich Turner, and he gave me a thoughtful reply from the console team's perspective. See: dotnet/msbuild#4299 (comment)

Short summary of Rich's advice:

you should start planning to adopt VT, and avoid Console APIs
-- @bitcrazed

@stephentoub
Copy link
Member

end-to-end coordinated story here around color, driven by the CLI, for consistency across the ecosystem

I had actually discussed this with Rich Turner, and he gave me a thoughtful reply from the console team's perspective

Thanks, but my end-to-end comments aren't specific to Console. They're about ensuring that everyone, regardless of how they're outputting color and whether they're using System.Console or not, plays by the same set of rules, e.g. if an app does manually emit escape codes via text as in that example, what should that app do / respect to enable or disable doing so.

@jzabroski
Copy link
Contributor

jzabroski commented Jul 2, 2021

@stephentoub I agree with you about a set of rules. In the thread I linked, I proposed "request-based mediated execution" a'la REST as an improvement to linux-style troff sub-system which couples the formatting to the left-pipe and right-pipe and requires duplicating knowledge about that media metadata. I have pushed for this design for several years. It would be awesome.

Using an env variable like NO_COLOR is a proxy for having a true browser-as-shell experience where the mime type plaintext is respected. But it's out-of-channel rather than in-channel. You just need to squint to see the future 20-30 years from now.

@ejsmith
Copy link
Author

ejsmith commented Jul 3, 2021

quite ridiculous

it's been years and multiple major versions

.NET 5 shipped in November of 2020. What other major version was released since this issue (which is in the 6.0 milestone) was opened in March 2020?

I get that you're frustrated, and I get that the bot auto-closing the PR only added to that. But calling this "ridiculous" and employing hyperbole won't win any points.

My $.02.

I get it. I overreacted a bit. I apologize. The reason I said multiple major versions is because it missed .NET 5 and it appeared that it wasn't going to happen for .NET 6 since the PR was closed and it seems like we've got to be getting close to lockdown mode for .NET 6. Again, I apologize. Thank you very much for looking into this and for getting that PR updated and merged in time for .NET 6. 🙂

@0xced
Copy link
Contributor

0xced commented Jul 5, 2021

Thanks Stephen for taking over this pull request, very much appreciated!

stephentoub pushed a commit to 0xced/runtime that referenced this issue Jul 9, 2021
…put is redirected

This  introduces an opt-in mechanism to allow writing ANSI color escape codes even when the console output is redirected.

To enable ANSI color codes, the `DOTNET_CONSOLE_ANSI_COLOR` environment variable must be set to `true` (case insensitive) or `1`.

Fixes dotnet#33980
stephentoub added a commit that referenced this issue Jul 9, 2021
…put is redirected (#47935)

* Add possibility to write ANSI color escape codes when the console output is redirected

This  introduces an opt-in mechanism to allow writing ANSI color escape codes even when the console output is redirected.

To enable ANSI color codes, the `DOTNET_CONSOLE_ANSI_COLOR` environment variable must be set to `true` (case insensitive) or `1`.

Fixes #33980

* Fix ANSI color env var handling

Co-authored-by: Stephen Toub <stoub@microsoft.com>
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 9, 2021
@ejsmith
Copy link
Author

ejsmith commented Jul 10, 2021

Thank you @0xced and @stephentoub !!

@ghost ghost locked as resolved and limited conversation to collaborators Aug 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.