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

Enable console color output by default #293

Closed
wants to merge 3 commits into from
Closed

Enable console color output by default #293

wants to merge 3 commits into from

Conversation

vsafonkin
Copy link

@vsafonkin vsafonkin commented May 3, 2022

Description:
Set environment variables DOTNET_SYSTEM_CONSOLE_ALLOW_ANSI_COLOR_REDIRECTION and TERM to enable color output by default.

Build via current setup-dotnet: https://github.com/vsafonkin/dotnet-setup-test/actions/runs/2265449492

Screenshot 2022-05-03 at 7 55 45 PM

Build via setup-dotnet from PR branch: https://github.com/vsafonkin/dotnet-setup-test/runs/6278013678

Screenshot 2022-05-03 at 7 58 40 PM

Related issue:
#288

Check list:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

@vsafonkin vsafonkin requested a review from a team May 3, 2022 18:11
@xt0rted
Copy link

xt0rted commented May 3, 2022

I ran into issues doing this in node when setting FORCE_COLOR where problem matchers stop working due to the color codes being emitted in the logs (the setup-node cache feature also stops working and returns an error). An example of this was found with my stylelint problem matcher and I had to work around it with a much more complex regex xt0rted/stylelint-problem-matcher#361 (demo run with it). I haven't tested the .net problem matcher yet to know how that works, but it's something to keep in mind when adding this in I think.

@vsafonkin
Copy link
Author

vsafonkin commented May 4, 2022

I ran into issues doing this in node when setting FORCE_COLOR where problem matchers stop working due to the color codes being emitted in the logs (the setup-node cache feature also stops working and returns an error). An example of this was found with my stylelint problem matcher and I had to work around it with a much more complex regex xt0rted/stylelint-problem-matcher#361 (demo run with it). I haven't tested the .net problem matcher yet to know how that works, but it's something to keep in mind when adding this in I think.

Hey @xt0rted, thank you for your help. I've tested the problem matcher which is defined in setup-dotnet for simple case (incorrect .NET version) and looks like the matcher works fine for this case: https://github.com/vsafonkin/dotnet-setup-test/actions/runs/2269399181

Screenshot 2022-05-04 at 1 17 37 PM

Also looks like the cache action works fine with enabled colorful output for .NET, at least I didn't face with any issues for caching. But you're right, my custom problem matcher stopped working after colorful output was enabled. It requires to change the custom problem matcher regex more complex.

In this case, I'm afraid the colorful output by default can break a part of customers who use their custom matchers. Probably, we should refuse these changes and close this PR.

cc: @marko-zivic-93 @brcrista

@brcrista
Copy link
Contributor

brcrista commented May 4, 2022

@vsafonkin thanks for the investigation. Just to make sure I understand, is the issue that ANSI color codes are interfering with problem matcher regexes?

@vsafonkin
Copy link
Author

vsafonkin commented May 4, 2022

@vsafonkin thanks for the investigation. Just to make sure I understand, is the issue that ANSI color codes are interfering with problem matcher regexes?

@brcrista, exactly, if someone uses setup-dotnet action and own problem matcher it can be broken after we enable colorful output, or rather the problem matcher will not detect problems that the customer wants to see.

@brcrista
Copy link
Contributor

brcrista commented May 4, 2022

Ok. So in the spirit of "yes, and" ... to ship this we would need to

  1. Release a new major version of the action
  2. Provide some official support for problem matching with ANSI color codes. Maybe something in actions/toolkit?

But yes, in the short term I think we'll just close this.

@vsafonkin
Copy link
Author

vsafonkin commented May 4, 2022

But yes, in the short term I think we'll just close this.

I agree, I believe the customers can set these env variables from their side if they need it and it will be safer for them.

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.

None yet

3 participants