Dotnet run does not preserve colors in child processes #1977

Closed
brthor opened this Issue Mar 21, 2016 · 37 comments

Projects

None yet
@brthor
brthor commented Mar 21, 2016

@mattwarren in BenchmarkDotNet brought this to my attention

Steps to reproduce

dotnet new

make Program.cs

using System;

namespace ConsoleApplication
{
    public class Program
    {
        public static void Main(string[] args)
        {
            Console.ForegroundColor = ConsoleColor.Gray;
            Console.WriteLine("Hello World!");
        }
    }
}

dotnet restore
dotnet run

Expected behavior

Outputs gray text

Actual behavior

save

Environment data

dotnet --version output:

C:\Users\brthor\code\repro\run-color>dotnet --version
.NET Command Line Tools (1.0.0-beta-001828)

Product Information:
 Version:     1.0.0-beta-001828
 Commit Sha:  N/A

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.10586
 OS Platform: Windows
 Runtime Id:  win10-x64

I'd expect this to be present anywhere because we don't do anything special to handle Console.SetColor or Console.ForegroundColor calls in child processes.

cc @anurse any ideas on how we could handle this?

@brthor brthor added the bug label Mar 21, 2016
@anurse
Collaborator
anurse commented Mar 21, 2016

We call ForwardStdOut on the command we launch. We shouldn't. It's basically that simple :). Then the colors should flow properly on Windows.

@Peter-Schneider
Contributor

@anurse I think #1985 is also related to ForwardStdOut...

@dougbu
dougbu commented Mar 27, 2016

Similarly dotnet test on Windows outputs only grey text while xunit.console.exe provides useful differentiation in its log spew.

Is this the same issue? If not I'll file something new.

E.g. logging during test execution:

dotnet test

dotnet

xunit.console.exe

console

@schotime

I'm seeing this issue too.
Will it be fixed for RTM?

@Eilon
Member
Eilon commented May 19, 2016

@piotrpMSFT @Petermarcu can we have someone look at this? It's quite difficult to read the un-colorized output when using dotnet run (or similar). It seems the fix is as easy as not piping the sub-command's output. The piping breaks streaming the data "live" as well as breaks any console commands that don't go out on the stream, such as setting the current console colors.

cc @DamianEdwards

@BrennanConroy

This is fixed by #3013 I believe.

@shanselman
Contributor

This is a problem, it seems, with all things that wrap dotnet run, like dotnet test and dotnet watch.

I am seeing this even today with 1.0.0-preview2-003119. That's pretty new. We need to fix this soon IMHO, it's super annoying. @Eilon
image

@Eilon
Member
Eilon commented Jun 20, 2016

@piotrpMSFT / @anurse I thought maybe this was fixed in the newest builds? Is @shanselman 's build just too old?

@BrennanConroy

I just tried with cli 3118 and it has color (although not the 100% correct color)

@shanselman
Contributor

That's 3119 in the screenshot above, @BrennanConroy. Did you do dotnet watch?

@BrennanConroy

After another look I only get color for info and it only has the foreground color set, not the background. I was on OpenSuse using dotnet watch run

@shanselman
Contributor

I'm not sure if this is another bug or related, but the tools change the color sometimes and leave it in a broken state. After this, your terminal colors are messed up. Note:

image

@BrennanConroy

I believe the bug is that the AnsiConsole the CLI uses doesn't understand all the colors https://github.com/dotnet/cli/blob/04f40f906dce2678d80fb9787e68de76ee6bf57e/src/Microsoft.DotNet.Cli.Utils/AnsiConsole.cs#L100-L135
This would explain why only green is working because ASP.NET Core's logger uses "DarkGreen" for information which is 32 and "Light" colors for the rest of the log types which are in the 90's

@shanselman
Contributor
shanselman commented Jun 20, 2016 edited

Might be worth talking to @miniksa. He knows EVERYTHING about the console as he owns it.

What you're showing in AnsiConsole explains the color stuff but not the loss of color with dotnet test and dotnet watch (things that call run), right?

@BrennanConroy

It explains them, they redirect output to the Reporter, which uses the AnsiConsole which uses the incorrect values for colors, and the ConsoleLogger was changed semi-recently to use the correct color values. So when the logger sends the correct values now the AnsiConsole doesn't understand them and doesn't change the color of anything.

@shanselman
Contributor

@BrennanConroy Plus these colors shouldn't be messed with on newer Windows, which supports Ansi directly. AnsiConsole @stephentoub needs to detect when it's on a 1511+ version of Windows and no-op, I think.

@BrennanConroy

That would be good, and on Unix systems they also support Ansi directly

@miniksa
miniksa commented Jun 20, 2016

Hey there,

I would highly recommend that you detect when you're on Windows 10 v1511 or better with the console that supports virtual terminal parsing (not the Legacy console) that you simply pass through the stream with all the embedded sequences and let our parser handle detecting which color it is instead of reinterpreting it.

I discussed generally how to do this last week with the libuv folks in this comment: libuv/libuv#889 (comment)
I'm not totally certain how to set the output mode from the CLR though. I don't think modes/properties are exposed. You might have to P/Invoke.

You would still have to fix it up for compatibility with previous versions of Windows, at least enough to not look terrible. But ongoing support should likely be a function of the console host now that we're processing sequences like Unix systems (e.g. my problem not yours :P).

If there's anything more I can add or questions I can answer, please feel free to ask away!

Thanks,
--Michael

@stephentoub
Member

on Unix systems they also support Ansi directly

From System.Console.dll, on Unix we use the escape sequences dictated by evaluating the data in the terminfo database for the current platform. Note, though, that we only output escape sequences when stdout is detected to be a character device, e.g. the terminal. This is to avoid writing gobbleydook when output has been redirected to, for example, a file (these sequences are used for more than just color, but pretty much every operation on the Console class, such as getting the current cursor position, moving the cursor, clearing the screen, changing the window title, etc.). If necessary, we could look into opening this up to also write them when redirected to pipes (it's a simple check, it's just a question of the ramifications), and it would also be for post-1.0 obviously.

To be clear, though, I'm not sure that's relevant. That's totally separate from AnsiConsole, which is a helper class in the CLI repo.

@shanselman
Contributor

Ok, so forgive me, @stephentoub we have a few things here and I'm confused.

  • calling dotnet test, watch or any wrapper causes dotnet run to lose all color
  • dotnet cli colors often leave the command prompt in a broken or "last color" state
  • AnsiConsole is missing colors?
  • Win10 1511+ has a new console that understands Ansi and would like it passed directly (but we need to probe for it, somewhere).

Are these separate bugs/issues, and how do we divvy them out so they:

  • aren't missed or forgotten
  • make a "it just works" experience for everyone who runs dotnet
@livarcocc
Member

@shanselman dotnet test does not use dotnet run as its executing mechanism. Though I believe watcher does.

@stephentoub
Member
stephentoub commented Jun 20, 2016 edited

Win10 1511+ has a new console that understands Ansi and would like it passed directly (but we need to probe for it, somewhere).

@miniksa, System.Console.dll on Windows (both in the .NET Framework and in .NET Core) uses SetConsoleTextAttribute to change console colors. Is there (and what is the) value in switching to writing ANSI escape codes to stdout instead? And if it's worthwhile, how would you recommend we detect the version to determine whether to switch? We've been explicitly told not to do such version checks, most of the APIs for getting OS version info lie without proper manifests in the containing application, etc.
cc: @weshaggard, @ianhays

@BrennanConroy

@livarcocc It isn't dotnet run that causes the issue, its the command.Create(...).ForwardStdOut().ForwardStdErr(); that calls AnsiConsole which causes the issue

@livarcocc
Member

@BrennanConroy That makes a lot more sense. Yes.

@brthor
brthor commented Jun 20, 2016 edited

For some time now dotnet run has stopped using ForwardStdErr/ForwardStdOut, so it's not clear to me how AnsiConsole would be being invoked there.

Colors should simply pass through.

https://github.com/dotnet/cli/blob/rel/1.0.0-preview2/src/dotnet/commands/dotnet-run/RunCommand.cs#L194

cc @eerhardt Who made this change to dotnet run

@miniksa
miniksa commented Jun 20, 2016

@stephentoub There isn't a particular value to switching for applications that are completely based on the existing Win32 API paradigm. The ANSI support was added to the Windows 10 v1511 console in order to support the Docker client and the Bash on Ubuntu on Windows environment so its stated goal is really to help software that already knows the *nix way of doing things and would prefer to communicate that way instead of parsing and translating it to the Win32 API surface.

The currently recommended way of detecting is detailed in the libuv comment I linked as well as the Docker client code sample linked at that comment: Attempt to set the console mode and if it takes, it's ready. If it rejects it as invalid, then it's not ready. Not a version check. Probe the API and see what it says. Unfortunately we don't have termcap/terminfo like *nix does at this time. Just the probe mechanism right now.

I'm speaking here in the context of one particular command line application that knows it is relaying ANSI data. In this scenario, it could be helpful to take advantage of the in-built parser in v1511+. Or not. Up to you. I just want to ensure that awareness of the functionality exists. :)

If there's desire to further understand what this could do to impact the .NET platform as a whole (System.Console.dll), we should discuss with my whole team. cc @bitcrazed (Rich Turner, our PM for the console system.)

@stephentoub
Member

Thanks, @miniksa. That's helpful. And in that case it sounds like at present there's no action item for System.Console.dll in corefx.

Which brings us back to @shanselman's list of issues:

calling dotnet test, watch or any wrapper causes dotnet run to lose all color

As @BrennanConroy suggested, sounds like this is just a matter of removing some calls to ForwardStdOut, and this issue could be used to track that happening.

dotnet cli colors often leave the command prompt in a broken or "last color" state

Sounds like a different issue should be used to track that.

AnsiConsole is missing colors?

Sounds like a different issue should be used to track that.

Win10 1511+ has a new console that understands Ansi and would like it passed directly (but we need to probe for it, somewhere).

Nothing here for corefx. Potentially some kind of work item for dotnet if these issues aren't addressed by simply removing the superfluous ForwardStdOut calls.

@brthor
brthor commented Jun 20, 2016

@BrennanConroy Run should not go through that codepath as it is a builtin command and should succeed the check : s_builtIns.TryGetValue(command, out builtIn) causing it to be run in proc.

https://github.com/dotnet/cli/blob/rel/1.0.0/src/dotnet/Program.cs#L147

If that's not actually happening it seems like that would be the bug here.

@pakrym
Contributor
pakrym commented Jun 20, 2016 edited

@brthor in #1977 (comment) @shanselman is talking about dotnet watch and dotnet test not dotnet run having this issue. So @BrennanConroy codepath is being hit.

@shanselman
Contributor

To be clear, run is fine. Stuff that is outside/wrapping run doesn't. See the screenshot. Test and Watch (the ones folks will use most) lose color. Not to mention that if you call "dotnet run" and there's an error, your colors are messed up for that session.

@brthor
brthor commented Jun 21, 2016

Thanks for the clarification here @pakrym @shanselman

We should just be able to remove those .ForwardStd calls then 😄

@shanselman
Contributor

Cool. Who is doing that?

@shanselman
Contributor

Ping

@livarcocc livarcocc self-assigned this Jul 7, 2016
@livarcocc
Member

Ok, I will take a stab at it.

@livarcocc livarcocc closed this in #3808 Jul 8, 2016
@stajs
stajs commented Sep 21, 2016

Has this fix been released? I'm still seeing uncolored output when using dotnet watch.

C:\>dotnet --info
.NET Command Line Tools (1.0.0-preview2-003131)

Product Information:
 Version:            1.0.0-preview2-003131
 Commit SHA-1 hash:  635cf40e58

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.10586
 OS Platform: Windows
 RID:         win10-x64
@eerhardt
Member

Has this fix been released?

No, this fix hasn't been released. The latest release (1.0.0-preview2-003131) only had fixes for high-priority bugs, and wasn't taken from the rel/1.0.0 working branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment