Fix color printing and --verbose #1212

Merged
merged 2 commits into from Feb 13, 2016

Conversation

Projects
None yet
7 participants
@AustinWise
Contributor

AustinWise commented Feb 3, 2016

I noticed that the escape codes are being printed instead of colors on Windows. There were a couple problems:

  • When running intrinsic commands, the AnsiPassThrough was being enabled. This caused the escape codes to never be processed for intrinsic commands.
  • Reporter.Write did not transform escape codes into colors. The Command uses this method to forward some output.

Calling ShouldPassAnsiCodesThrough() for the side effect of reading the environmental variable seems a little lame. Perhaps instead Command.CreateDotNet should always set this variable? I'm not sure if Microsoft.DotNet.Cli.Utils is meant for consumption by externals tools as a means of invoking dotnet.exe, so I did not do it there.

@dnfclas

This comment has been minimized.

Show comment
Hide comment
@dnfclas

dnfclas Feb 3, 2016

Hi @AustinWise, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

dnfclas commented Feb 3, 2016

Hi @AustinWise, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

src/dotnet/Program.cs
@@ -140,6 +140,8 @@ private static int ProcessArgs(string[] args)
{
// mimic the env variable
Environment.SetEnvironmentVariable(CommandContext.Variables.Verbose, verbose.ToString());
+ // Use the current value of AnsiPassThru but make sure child processes have it set to true.
+ CommandContext.ShouldPassAnsiCodesThrough();

This comment has been minimized.

@livarcocc

livarcocc Feb 3, 2016

Member

Is this a query or a command? If a query, should you be testing the result here? If a command, can we call it PassAnsiCodesThrough?

@livarcocc

livarcocc Feb 3, 2016

Member

Is this a query or a command? If a query, should you be testing the result here? If a command, can we call it PassAnsiCodesThrough?

This comment has been minimized.

@brthor

brthor Feb 3, 2016

Contributor

+1 livar This just returns a value, why put this here?

@brthor

brthor Feb 3, 2016

Contributor

+1 livar This just returns a value, why put this here?

This comment has been minimized.

@AustinWise

AustinWise Feb 4, 2016

Contributor

The intent to cause CommandContext to read the current value of the environmental variable before changing it on the following line. The intention is for the current process to use the value of the DOTNET_CLI_CONTEXT_ANSI_PASS_THRU as it is when the process starts but for all the children to have it set to true. Otherwise dotnet.exe will print out a bunch of escape codes on Windows because AnsiPassThrough will always be true.

Since calling that getter method for it's side effect is obtuse, perhaps I should add a new function to CommandContext that reads the environmental variable. Something like "InitialAnsiPassThroughFromEnvironment()". What do you think?

@AustinWise

AustinWise Feb 4, 2016

Contributor

The intent to cause CommandContext to read the current value of the environmental variable before changing it on the following line. The intention is for the current process to use the value of the DOTNET_CLI_CONTEXT_ANSI_PASS_THRU as it is when the process starts but for all the children to have it set to true. Otherwise dotnet.exe will print out a bunch of escape codes on Windows because AnsiPassThrough will always be true.

Since calling that getter method for it's side effect is obtuse, perhaps I should add a new function to CommandContext that reads the environmental variable. Something like "InitialAnsiPassThroughFromEnvironment()". What do you think?

This comment has been minimized.

@brthor

brthor Feb 4, 2016

Contributor

I get the intent here but I don't get the side effect you're going after.

You're forcing the variable to get read, but what does that affect?

@brthor

brthor Feb 4, 2016

Contributor

I get the intent here but I don't get the side effect you're going after.

You're forcing the variable to get read, but what does that affect?

@AustinWise

This comment has been minimized.

Show comment
Hide comment
@AustinWise

AustinWise Feb 4, 2016

Contributor

I changed the way the environmental variable is read to be explicit, instead of calling a getter method for the side effect of lazy initialization.
I also expanded the scope of this pull request a little bit by making sure the verbose environmental variable is inherited by all child processes. Currently only the first process is getting it and it will be overwritten in any child processes.

Contributor

AustinWise commented Feb 4, 2016

I changed the way the environmental variable is read to be explicit, instead of calling a getter method for the side effect of lazy initialization.
I also expanded the scope of this pull request a little bit by making sure the verbose environmental variable is inherited by all child processes. Currently only the first process is getting it and it will be overwritten in any child processes.

@AustinWise AustinWise changed the title from Fix color printing on Windows. to Fix color printing and --verbose Feb 4, 2016

src/dotnet/Program.cs
+ Environment.SetEnvironmentVariable(CommandContext.Variables.Verbose, verbose.ToString());
+ }
+ //This process should inherit AnsiPassThrough or use the default of false.
+ CommandContext.InitializeAnsiPassThroughFromEnvironment();

This comment has been minimized.

@brthor

brthor Feb 4, 2016

Contributor

How does this affect the logical flow of the program?

It reads the environment variable but how does that affect subsequent code which runs? I may be missing something here.

@brthor

brthor Feb 4, 2016

Contributor

How does this affect the logical flow of the program?

It reads the environment variable but how does that affect subsequent code which runs? I may be missing something here.

This comment has been minimized.

@brthor

brthor Feb 4, 2016

Contributor

Is this capturing the value of the environment variable before it gets changed in the next bit?

@brthor

brthor Feb 4, 2016

Contributor

Is this capturing the value of the environment variable before it gets changed in the next bit?

This comment has been minimized.

@AustinWise

AustinWise Feb 5, 2016

Contributor

Yes, CommandContext.InitializeAnsiPassThroughFromEnvironment() reads the environmental variable (or uses the default value), while the following line sets the environmental variable true, which will effect any spawned processes.

As some background, all color formatting of output is done using ASNI escape codes. However these are not supported by Windows so Ansi Console parses them and changes Console.ForegroundColor appropriately.

The changes to Console.ForegroundColor have to be in the process that was directly invoked by the user, as the output redirection used when spawning processes does not capture the color changes. So the Report class checks the environmental variable DOTNET_CLI_CONTEXT_ANSI_PASS_THRU, and skips converting the ANSI codes.

Before the this pull request, the DOTNET_CLI_CONTEXT_ANSI_PASS_THRU was getting always set for intrinsic commands, so the ANSI escape codes were being written directly to the Windows console. This pull request makes the process use the current value of the environmental variable, which will default to false for the original invokcation of dotnet.exe. However we want any child process we spawn to pass through ANSI escape codes, so we need to set the environmental variable to true.

@AustinWise

AustinWise Feb 5, 2016

Contributor

Yes, CommandContext.InitializeAnsiPassThroughFromEnvironment() reads the environmental variable (or uses the default value), while the following line sets the environmental variable true, which will effect any spawned processes.

As some background, all color formatting of output is done using ASNI escape codes. However these are not supported by Windows so Ansi Console parses them and changes Console.ForegroundColor appropriately.

The changes to Console.ForegroundColor have to be in the process that was directly invoked by the user, as the output redirection used when spawning processes does not capture the color changes. So the Report class checks the environmental variable DOTNET_CLI_CONTEXT_ANSI_PASS_THRU, and skips converting the ANSI codes.

Before the this pull request, the DOTNET_CLI_CONTEXT_ANSI_PASS_THRU was getting always set for intrinsic commands, so the ANSI escape codes were being written directly to the Windows console. This pull request makes the process use the current value of the environmental variable, which will default to false for the original invokcation of dotnet.exe. However we want any child process we spawn to pass through ANSI escape codes, so we need to set the environmental variable to true.

This comment has been minimized.

@brthor

brthor Feb 5, 2016

Contributor

I have an incoming PR which may overlap a bit but it shouldn't be too bad. 😄

I get what you are trying to do now, but the way of doing this feels hacky.
Reading through this bit it wouldn't be clear to me that we are saving the value so that it's not affected by the subsequent set.

With your other change this should be entirely unnecessary for internal commands now no? Because they'll all be printing to the same console, without any forwarding involved.

So why not only set this environment variable when calling external commands, leaving it in the command.create below but removing it altogether for internal commands.

@brthor

brthor Feb 5, 2016

Contributor

I have an incoming PR which may overlap a bit but it shouldn't be too bad. 😄

I get what you are trying to do now, but the way of doing this feels hacky.
Reading through this bit it wouldn't be clear to me that we are saving the value so that it's not affected by the subsequent set.

With your other change this should be entirely unnecessary for internal commands now no? Because they'll all be printing to the same console, without any forwarding involved.

So why not only set this environment variable when calling external commands, leaving it in the command.create below but removing it altogether for internal commands.

This comment has been minimized.

@AustinWise

AustinWise Feb 5, 2016

Contributor

Yeah, it does not seem a bit hacky.

I think it would not be too hard to make all external external commands. Since all the commands use Command.CreateDotnet, I could change that to apply the ANSI forwarding.

@AustinWise

AustinWise Feb 5, 2016

Contributor

Yeah, it does not seem a bit hacky.

I think it would not be too hard to make all external external commands. Since all the commands use Command.CreateDotnet, I could change that to apply the ANSI forwarding.

@AustinWise

This comment has been minimized.

Show comment
Hide comment
@AustinWise

AustinWise Feb 5, 2016

Contributor

For reference, this is what it looks like before this change:
Before
And after:
After
Note the funny looking "←[33m" text in the before picture.

Contributor

AustinWise commented Feb 5, 2016

For reference, this is what it looks like before this change:
Before
And after:
After
Note the funny looking "←[33m" text in the before picture.

@AustinWise

This comment has been minimized.

Show comment
Hide comment
@AustinWise

AustinWise Feb 5, 2016

Contributor

I push a new commit that just applies the environmental variable to all children created with the Command class. I think this is the right place to set the variable as the Command does the output forwarding that depends on this variable being set in children.

Contributor

AustinWise commented Feb 5, 2016

I push a new commit that just applies the environmental variable to all children created with the Command class. I think this is the right place to set the variable as the Command does the output forwarding that depends on this variable being set in children.

@@ -28,6 +28,7 @@ private Command(CommandSpec commandSpec)
RedirectStandardError = true,
RedirectStandardOutput = true
};
+ psi.Environment[CommandContext.Variables.AnsiPassThru] = bool.TrueString;

This comment has been minimized.

@brthor

brthor Feb 5, 2016

Contributor

I think there are situations where we might want to use command without being forced to set this value. Testing is the first thing that comes to mind, but also this is intended to be a general purpose Command Class for use by CLI.

I prefer the more explicit method of declaring exactly what environment variables you want set with each command.

@brthor

brthor Feb 5, 2016

Contributor

I think there are situations where we might want to use command without being forced to set this value. Testing is the first thing that comes to mind, but also this is intended to be a general purpose Command Class for use by CLI.

I prefer the more explicit method of declaring exactly what environment variables you want set with each command.

This comment has been minimized.

@AustinWise

AustinWise Feb 5, 2016

Contributor

I pushed a revision where I only set the environmental variable if output is being redirected to the Reporter, which knows how to handle the ANSI escape codes.

This way if someone is using the Command class to forward output, colors will just work. And if the command class is being used to capture output, the consumer of the output will not unexpectedly see escape codes unless they explicitly ask for them.

I think this is preferable to having to remember to set the environmental variable each time you want to redirect output.

@AustinWise

AustinWise Feb 5, 2016

Contributor

I pushed a revision where I only set the environmental variable if output is being redirected to the Reporter, which knows how to handle the ANSI escape codes.

This way if someone is using the Command class to forward output, colors will just work. And if the command class is being used to capture output, the consumer of the output will not unexpectedly see escape codes unless they explicitly ask for them.

I think this is preferable to having to remember to set the environmental variable each time you want to redirect output.

@@ -149,6 +149,7 @@ public Command ForwardStdOut(TextWriter to = null, bool onlyIfVerbose = false)
if (to == null)
{
_stdOut.ForwardTo(write: Reporter.Output.Write, writeLine: Reporter.Output.WriteLine);
+ _process.StartInfo.Environment[CommandContext.Variables.AnsiPassThru] = bool.TrueString;

This comment has been minimized.

@brthor

brthor Feb 5, 2016

Contributor

Alright this seems like a reasonable default, but I think there are testing scenarios where we won't want to have this set.

Can we create a Boolean option which defaults to true for these two methods, and set the value of this environment variable to that Boolean?

That way we will have a method of turning it off if we need to.

@brthor

brthor Feb 5, 2016

Contributor

Alright this seems like a reasonable default, but I think there are testing scenarios where we won't want to have this set.

Can we create a Boolean option which defaults to true for these two methods, and set the value of this environment variable to that Boolean?

That way we will have a method of turning it off if we need to.

This comment has been minimized.

@AustinWise

AustinWise Feb 5, 2016

Contributor

Oh, that's a good idea, I'll add that in.

@AustinWise

AustinWise Feb 5, 2016

Contributor

Oh, that's a good idea, I'll add that in.

@brthor

This comment has been minimized.

Show comment
Hide comment
@brthor

brthor Feb 5, 2016

Contributor

This LGTM.

We are having some issues with tests on the CI right now so we'll hold off on merging for the moment.

I'd also like to see how this interacts with the new streamforwarder changes in #1240. I don't expect any issues but there is potential.

Contributor

brthor commented Feb 5, 2016

This LGTM.

We are having some issues with tests on the CI right now so we'll hold off on merging for the moment.

I'd also like to see how this interacts with the new streamforwarder changes in #1240. I don't expect any issues but there is potential.

@AustinWise

This comment has been minimized.

Show comment
Hide comment
@AustinWise

AustinWise Feb 5, 2016

Contributor

If #1240 get's merged first I'll rebase ontop of it and try it out. It does not look like it would cause any problem.
Thanks for reviewing my changes!

Contributor

AustinWise commented Feb 5, 2016

If #1240 get's merged first I'll rebase ontop of it and try it out. It does not look like it would cause any problem.
Thanks for reviewing my changes!

@AustinWise

This comment has been minimized.

Show comment
Hide comment
@AustinWise

AustinWise Feb 9, 2016

Contributor

I rebased onto the tip of 1.0.0 to fix the merge conflicts.

Contributor

AustinWise commented Feb 9, 2016

I rebased onto the tip of 1.0.0 to fix the merge conflicts.

@AustinWise

This comment has been minimized.

Show comment
Hide comment
@AustinWise

AustinWise Feb 11, 2016

Contributor

I rebased and it seems like #1364 has fixed the build problems.
I tested out my changes the colors are still fixed.

Contributor

AustinWise commented Feb 11, 2016

I rebased and it seems like #1364 has fixed the build problems.
I tested out my changes the colors are still fixed.

@brthor

This comment has been minimized.

Show comment
Hide comment
@brthor

brthor Feb 11, 2016

Contributor

I think we should merge this today.

@anurse want to take a quick look?

Contributor

brthor commented Feb 11, 2016

I think we should merge this today.

@anurse want to take a quick look?

@anurse

This comment has been minimized.

Show comment
Hide comment
@anurse

anurse Feb 11, 2016

Collaborator

LGTM too. ANSI pass-through should be able to go away once we change Command.cs to only redirect stdout/stderr when we actually want to capture the output. It's only there because Windows uses APIs to manage Console Colors rather than ANSI codes :)

Collaborator

anurse commented Feb 11, 2016

LGTM too. ANSI pass-through should be able to go away once we change Command.cs to only redirect stdout/stderr when we actually want to capture the output. It's only there because Windows uses APIs to manage Console Colors rather than ANSI codes :)

@AustinWise

This comment has been minimized.

Show comment
Hide comment
@AustinWise

AustinWise Feb 12, 2016

Contributor

Since ANSI pass through is temporary, when I rebased I did not add the new parameter to ICommand.

As an aside, during testing I discovered that Windows 10 v1511 added support for these ANSI codes.

Contributor

AustinWise commented Feb 12, 2016

Since ANSI pass through is temporary, when I rebased I did not add the new parameter to ICommand.

As an aside, during testing I discovered that Windows 10 v1511 added support for these ANSI codes.

@brthor

This comment has been minimized.

Show comment
Hide comment
@brthor

brthor Feb 12, 2016

Contributor

@dotnet-bot Windows_NT Debug Build please

Contributor

brthor commented Feb 12, 2016

@dotnet-bot Windows_NT Debug Build please

@AustinWise

This comment has been minimized.

Show comment
Hide comment
@AustinWise

AustinWise Feb 12, 2016

Contributor

@brthor Odd, I don't understand why ProcessStartInfo.Enironment stopped working on just Windows. Do you have any ideas?

'ProcessStartInfo' does not contain a definition for 'Environment'
Contributor

AustinWise commented Feb 12, 2016

@brthor Odd, I don't understand why ProcessStartInfo.Enironment stopped working on just Windows. Do you have any ideas?

'ProcessStartInfo' does not contain a definition for 'Environment'
@@ -149,6 +149,7 @@ public ICommand ForwardStdOut(TextWriter to = null, bool onlyIfVerbose = false)
if (to == null)
{
_stdOut.ForwardTo(writeLine: Reporter.Output.WriteLine);
+ _process.StartInfo.Environment[CommandContext.Variables.AnsiPassThru] = bool.TrueString;

This comment has been minimized.

@brthor

brthor Feb 12, 2016

Contributor

Why change this back?

@brthor

brthor Feb 12, 2016

Contributor

Why change this back?

@brthor

This comment has been minimized.

Show comment
Hide comment
@brthor

brthor Feb 12, 2016

Contributor

@AustinWise Not sure, seems weird only on Windows. Will need to investigate

Contributor

brthor commented Feb 12, 2016

@AustinWise Not sure, seems weird only on Windows. Will need to investigate

AustinWise added some commits Feb 5, 2016

Fix color printing and verbosity.
Only apply the ANSI passthrough for child commands. Otherwise the
escape codes are printed to the console on Windows

Make Report.Write process escape codes so that forwarded output will have
its escape codes processed.

Also make verbose be inherited by all child processes.
@AustinWise

This comment has been minimized.

Show comment
Hide comment
@AustinWise

AustinWise Feb 13, 2016

Contributor

@dotnet-bot Windows_NT Debug Build please

Contributor

AustinWise commented Feb 13, 2016

@dotnet-bot Windows_NT Debug Build please

@AustinWise

This comment has been minimized.

Show comment
Hide comment
@AustinWise

AustinWise Feb 13, 2016

Contributor

@brthor I fixed the compile error. As part of doing that I put the ansiPassThrough back on Command and added it to ICommand.

Contributor

AustinWise commented Feb 13, 2016

@brthor I fixed the compile error. As part of doing that I put the ansiPassThrough back on Command and added it to ICommand.

TheRealPiotrP added a commit that referenced this pull request Feb 13, 2016

Merge pull request #1212 from AustinWise/fixColors
Fix color printing and --verbose

@TheRealPiotrP TheRealPiotrP merged commit e8ef94e into dotnet:rel/1.0.0 Feb 13, 2016

8 checks passed

CentOS7.1 Debug Build Build finished. 133 tests run, 1 skipped, 0 failed.
Details
CentOS7.1 Release Build Build finished. 133 tests run, 1 skipped, 0 failed.
Details
OSX Debug Build Build finished. 133 tests run, 1 skipped, 0 failed.
Details
OSX Release Build Build finished. 133 tests run, 1 skipped, 0 failed.
Details
Ubuntu Debug Build Build finished. 133 tests run, 1 skipped, 0 failed.
Details
Ubuntu Release Build Build finished. 133 tests run, 1 skipped, 0 failed.
Details
Windows_NT Debug Build Build finished. 133 tests run, 0 skipped, 0 failed.
Details
Windows_NT Release Build Build finished. 133 tests run, 0 skipped, 0 failed.
Details
@TheRealPiotrP

This comment has been minimized.

Show comment
Hide comment
@TheRealPiotrP

TheRealPiotrP Feb 13, 2016

Collaborator

Thanks @AustinWise!

Collaborator

TheRealPiotrP commented Feb 13, 2016

Thanks @AustinWise!

@AustinWise

This comment has been minimized.

Show comment
Hide comment
@AustinWise

AustinWise Feb 13, 2016

Contributor

@piotrpMSFT Thanks for @brthor reviewing my code. I'm looking forward to seeing pretty colors in my terminal again!

Contributor

AustinWise commented Feb 13, 2016

@piotrpMSFT Thanks for @brthor reviewing my code. I'm looking forward to seeing pretty colors in my terminal again!

@AustinWise AustinWise deleted the AustinWise:fixColors branch Feb 13, 2016

@brthor brthor referenced this pull request Feb 18, 2016

Closed

Consistency in command line interface #1476

3 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment