[17.12] Adding a static factory for the TerminalLogger #11016
Conversation
… Logger based on current environment.
|
SDK PR was created: dotnet/sdk#45009 Hotfix for 9.0.1xx: dotnet/sdk#45015 |
| public static ILogger CreateTerminalOrConsoleLogger(LoggerVerbosity verbosity) | ||
| { | ||
| bool isDisabled = (Environment.GetEnvironmentVariable("MSBUILDTERMINALLOGGER") ?? string.Empty).Equals("off", StringComparison.InvariantCultureIgnoreCase); | ||
| (bool supportsAnsi, bool outputIsScreen, uint? originalConsoleMode) = NativeMethodsShared.QueryIsScreenAndTryEnableAnsiColorCodes(); |
There was a problem hiding this comment.
nit: we can skip this if TL is requested to be disabled
There was a problem hiding this comment.
I started with that intention, but this approach has lower complexity and the performance impact is negligible.
| /// Creates a Terminal logger or Console logger based on the environment. | ||
| /// This method is called by reflection from dotnet. Do not modify the name or parameters without adapting the SDK. | ||
| /// </summary> | ||
| public static ILogger CreateTerminalOrConsoleLogger(LoggerVerbosity verbosity) |
There was a problem hiding this comment.
One missing piece here is that while this does check the environment variable, the CLI flags (--tl:off, etc) are not respected. We may need a way to pass in an argv and have this method check for the presence/absence of the TL-related (and --console logger parameter!) flags to ensure uniform behavior.
|
One question I had was: the main problem was that the silent TL verbosity still caused the 'progress' indicators to be written - could those be gated behind the verbosity check as well? Ideally we'd be able to say something like "when silent verbosity is used, only the diagnostic reports will be written to stdout". |
|
@baronfel, thanks for the great feedback. While your points are valid, would you like to expand the fix in this PR? I'm asking because of the US Thanksgiving holiday. I'm not sure how much more time it will take to deliver this fix if we miss this week... |
|
As discussed offline, we have time for 9.0.200, so I'll convert this PR to draft and I will cover Chet's comments. |
Fixes #10998
Summary
dotnet rundirectly creates an instance ofTerminalLogger, bypassing all checks. This can lead to situations where ANSI escape sequences are emitted to the terminal, even if the terminal does not support them. These escape sequences are also emitted when the standard output is redirected, which can break a CI build that relies on the command's output.Changes Made
Added a static factory that can return instance of Terminal or Console logger based on current environment. The usage of
TerminalLoggerin this scenario can be explicitly disabled by using of already existing env. variable, which wasn't possible before.Customer Impact
Some customers reported unexpected behavior of
dotnet runcommand and CI builds failures.Regression?
Yes.
Testing
Manual testing with locally updated SDK.
Notes
There will be a servicing PR for SDK 9.0.2xx that uses this new method. In 9.0.1xx, TL will be disabled for the last part of
dotnet runcompletely.