Skip to content

Add possibility to write ANSI color escape codes when the console output is redirected - #47935

Merged
stephentoub merged 2 commits into
dotnet:mainfrom
0xced:unix_console_ansi_color
Jul 9, 2021
Merged

Add possibility to write ANSI color escape codes when the console output is redirected#47935
stephentoub merged 2 commits into
dotnet:mainfrom
0xced:unix_console_ansi_color

Conversation

@0xced

@0xced0xced commented Feb 5, 2021

Copy link
Copy Markdown
Contributor

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

@ghost

ghost commented Feb 5, 2021

Copy link
Copy Markdown

Tagging subscribers to this area:
See info in area-owners.md if you want to be subscribed.

Issue Details

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

Author:0xced
Assignees:-
Labels:

area-System.Console

Milestone:-

@ejsmith

Copy link
Copy Markdown

Nice! Thanks for doing this!

Comment threadsrc/libraries/System.Console/src/System/ConsolePal.Unix.cs Outdated
Comment threadsrc/libraries/System.Console/src/System/ConsolePal.Unix.cs Outdated
@stephentoubstephentoub added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 8, 2021
@stephentoub

Copy link
Copy Markdown
Member

Direction for #33980 needs to be resolved first. @KathleenDollard, please chime in.

@ethomson

Copy link
Copy Markdown

I'm sorry, but I must still disagree that this is the correct direction. CI providers already have a standard mechanism that they use to communicate that you're running in a CI environment. For better or worse, this is the status quo; tools detect the CI environment and adapt their behavior accordingly.

By introducing a novel variable, that means that you're expecting the CI systems to start setting it.

I don't mean to sound negative or difficult, but we will not, if for the practical reason that there are many more applications in the world than there are CI systems and there's already an established mechanism where the apps check to see if they're running in CI. We simply can't set dozens or hundreds (or more?) of environment variables.

(I apologize if your change is orthogonal to CI systems, but that was certainly my interest in and understanding of #33980.)

Base automatically changed from master to mainMarch 1, 2021 09:07
@0xced

0xced commented Mar 5, 2021

Copy link
Copy Markdown
ContributorAuthor

I apologize if your change is orthogonal to CI systems

Yes, this change is absolutely orthogonal to CI systems. The modification proposed here is at the Unix Console platform abstraction layer and should certainly not have anything remotely related to CI environment variables.

By the way, I just rebased on the main branch and I'm waiting for directions before tackling the cache the environment variable evaluation in a static reviewed by Stephen.

@0xced
0xcedforce-pushed the unix_console_ansi_color branch 4 times, most recently from 0239f02 to 0798728CompareMarch 8, 2021 16:21
@0xced
0xcedforce-pushed the unix_console_ansi_color branch from 0798728 to 0dbfbb3CompareApril 2, 2021 21:07
@jeffhandley

Copy link
Copy Markdown
Member

Since #33980 still has some open topics, I'm going to mark this PR as a Draft until those details are solidified.

@jeffhandley
jeffhandley marked this pull request as draft April 18, 2021 06:05
@ghostghost closed this May 18, 2021
@ghost

Copy link
Copy Markdown

Draft Pull Request was automatically closed for inactivity. Please let us know if you'd like to reopen it.

@0xced

Copy link
Copy Markdown
ContributorAuthor

closed for inactivity

☹️

@karelzkarelz added this to the 6.0.0 milestone May 20, 2021
@ejsmith

Copy link
Copy Markdown

Pretty damn sad. This seems like a really nice thing to have and an easy win.

@ghostghost locked as resolved and limited conversation to collaborators Jun 20, 2021
@stephentoubstephentoub reopened this Jul 2, 2021
@stephentoub
stephentoub marked this pull request as ready for review July 2, 2021 11:47
@dotnetdotnet unlocked this conversation Jul 2, 2021
@stephentoub
stephentoubforce-pushed the unix_console_ansi_color branch from 0dbfbb3 to 829989dCompareJuly 2, 2021 14:32
@stephentoub

Copy link
Copy Markdown
Member

@0xced, I pushed a commit to your branch that:

  • Changes the env var name
  • Also adds NO_COLOR
  • Caches the result in a field to avoid getting environment variables every time we're writing out
  • Adds a test for the env var

@dotnet/area-system-console, please review.

@stephentoubstephentoub removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 2, 2021
@ejsmith

Copy link
Copy Markdown

@stephentoub this looks good. Thank you! My only question is if converting the stored int to a bool on every single console write would be a perf hit over just storing the calculated bool value?

@stephentoub

stephentoub commented Jul 3, 2021

Copy link
Copy Markdown
Member

Thank you!

You're welcome.

My only question is if converting the stored int to a bool on every single console write would be a perf hit over just storing the calculated bool value?

No, it won't be measurable. It's just value != 0. It's also not on Console.Writes, just color changes.

I do need to delete an erroneous assert that's the cause of the test failures.

@stephentoub
stephentoubforce-pushed the unix_console_ansi_color branch from 829989d to 339f800CompareJuly 5, 2021 00:18
@tmds

tmds commented Jul 5, 2021

Copy link
Copy Markdown
Member

@stephentoub when TERM is not set and DOTNET_CONSOLE_ANSI_COLOR is set, no color codes are written because TerminalFormatStrings are null.

@stephentoub

Copy link
Copy Markdown
Member

when TERM is not set

Thanks. I can fix the test for when in CI for that. I don't think there's a product issue here, though; if you don't have TERM set, you can set it. Agreed? Or is there a larger concern I'm not aware of?

@tmds

tmds commented Jul 6, 2021

Copy link
Copy Markdown
Member

Thanks. I can fix the test for when in CI for that. I don't think there's a product issue here, though; if you don't have TERM set, you can set it. Agreed? Or is there a larger concern I'm not aware of?

Yes, it can be set. I don't know if it is expected to be set in the CI environment variables DOTNET_CONSOLE_ANSI_COLOR is meant for.
Alternatively, fixed literals can be used (as in AnsiParser.cs).

0xcedand others added 2 commits July 9, 2021 11:00
…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`.
Fixesdotnet#33980
@stephentoub
stephentoubforce-pushed the unix_console_ansi_color branch from 339f800 to 5fb9588CompareJuly 9, 2021 15:04
@stephentoub
stephentoub merged commit 0d848f6 into dotnet:mainJul 9, 2021
@runfoapprunfoappBot mentioned this pull request Jul 10, 2021
@0xced
0xced deleted the unix_console_ansi_color branch July 10, 2021 06:59
@ghostghost locked as resolved and limited conversation to collaborators Aug 9, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

8 participants

@0xced@ejsmith@stephentoub@ethomson@jeffhandley@tmds@karelz@adamsitnik