Adding a static factory for the TerminalLogger #11318

MichalPavlik · 2025-01-21T13:36:04Z

Context

Although I created the SDK hotfix to always use the ConsoleLogger, we want to use TerminalLogger when it's possible.

Changes Made

Added a static factory that can return instance of Terminal or Console logger based on current environment. The usage of TerminalLogger in this scenario can be explicitly disabled by using of already existing env. variable or CLI argument --tl:off.
This change also prevents emitting of progress indication when the verbosity is set to quiet.

Notes

I tried to move TL to Microsoft.Build, where it belongs, but we don't want to make it public. Unfortunately, using the InternalsVisibleTo attribute to expose this type to the MSBuild project causes many type conflicts due to shared sources. Using the reflection is an ugly, but unnecessary workaround to make the factory public :(

…er instance based on settings and terminal capabilities.

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

src/MSBuild/TerminalLogger/TerminalLogger.cs:1089

[nitpick] The variable name 'tlArg' is ambiguous. It should be renamed to 'terminalLoggerArgument' for better readability.

string tlArg = args?.FirstOrDefault(a => a.StartsWith("--tl:", StringComparison.InvariantCultureIgnoreCase)) ?? string.Empty;

src/MSBuild/TerminalLogger/TerminalLogger.cs:1087

Ensure that the new behavior introduced by the 'CreateTerminalOrConsoleLogger' method is covered by tests.

public static ILogger CreateTerminalOrConsoleLogger(LoggerVerbosity verbosity, string[]? args)

MichalPavlik · 2025-01-21T13:48:15Z

@baronfel, I know you would like to have full support for CLI logger configuration, but it will require some refactoring as the code for argument parsing is mostly placed in XMake. I wanted to break up XMake years ago, but now we have a valid reason to refactor at least part of the huge file :) If you don't mind, I would create a separate issue for this work.

baronfel

LGTM, but should we use this factory in our own codebase to dogfood?

baronfel · 2025-01-21T17:12:44Z

@baronfel, I know you would like to have full support for CLI logger configuration, but it will require some refactoring as the code for argument parsing is mostly placed in XMake. I wanted to break up XMake years ago, but now we have a valid reason to refactor at least part of the huge file :) If you don't mind, I would create a separate issue for this work.

Yeah, that is quite a large unit of work - makes sense to carve it out.

MichalPavlik · 2025-01-22T10:06:17Z

LGTM, but should we use this factory in our own codebase to dogfood?

I wanted to do it from the beginning, but the entry point logic emits global messages when it detects that TL couldn't be used. The easiest solution is to make the list of deferred messages in XMake internal, but I don't like it from an engineering point of view (separation of concerns, class coupling, etc.).

I think we could move the factory to some already public type like ConsoleLogger, so the CLI doesn't have to use reflection. Then we could move the list of deferred messages from XMake to the Logger class and use it directly in the factory or, in a cleaner way, return some indication of why the TL cannot be used and let XMake create the messages. Let's sync offline.

Edit: I'm looking at XMake once again, and actually, there are multiple things that make dogfooding the factory... problematic :(

MichalPavlik · 2025-01-27T14:13:37Z

We decided to reuse parameters parsing logic to support all CL and TL logger parameters in this case.

MichalPavlik · 2025-02-05T16:37:56Z

We agreed to use the factory as is and make it public. Support for all parameters will be added later.

…ed unit test

src/Build/Logging/ConsoleLogger.cs

rainersigwald · 2025-02-07T21:56:15Z

src/Build/Logging/ConsoleLogger.cs

+                    a.StartsWith("/tl:", StringComparison.InvariantCultureIgnoreCase) ||
+                    a.StartsWith("-tl:", StringComparison.InvariantCultureIgnoreCase) ||
+                    a.StartsWith("--tl:", StringComparison.InvariantCultureIgnoreCase)) ?? string.Empty;
+
+            bool isDisabled =
+                tlArg.EndsWith("tl:on", StringComparison.InvariantCultureIgnoreCase) ? false :
+                tlArg.EndsWith("tl:off", StringComparison.InvariantCultureIgnoreCase) ? true :


I think we need to support the long form too, which makes this complex enough that I lean to "screw it use a regex"

(?:/|-|--)(?:tl|terminallogger):(?'value'on|off)

rainersigwald · 2025-02-07T21:57:32Z

src/MSBuild/MSBuild.csproj

@@ -136,7 +136,6 @@
    <Compile Include="CommandLineSwitchException.cs" />
    <Compile Include="..\Shared\CoreCLRAssemblyLoader.cs" Condition="'$(TargetFrameworkIdentifier)' != '.NETFramework'" />
    <Compile Include="DistributedLoggerRecord.cs" />
-    <Compile Include="TerminalLogger\*.cs" />


Why un-glob?

rainersigwald · 2025-02-07T22:01:42Z

src/MSBuild/TerminalLogger/TerminalLogger.cs

@@ -396,7 +403,7 @@ private void BuildStarted(object sender, BuildStartedEventArgs e)

        _buildStartTime = e.Timestamp;

-        if (Terminal.SupportsProgressReporting)
+        if (Terminal.SupportsProgressReporting && Verbosity != LoggerVerbosity.Quiet)


I'm not sure I see why "quiet" implies "don't make the spinner go", can you elaborate on that a bit?

rainersigwald · 2025-02-07T22:02:45Z

src/Build.UnitTests/ConsoleLogger_Tests.cs

+        [InlineData("-tl:on", true, true, "off", "TerminalLogger")]
+        public void CreateTerminalOrConsoleLogger_CreatesCorrectLoggerInstance(string argsString, bool supportsAnsi, bool outputIsScreen, string evnVariableValue, string expectedLoggerName)
+        {
+            string originalValue = Environment.GetEnvironmentVariable("MSBUILDTERMINALLOGGER");


TestEnvironment has helpers for this, can you switch to them?

rainersigwald · 2025-02-07T22:03:57Z

src/Build/Logging/ConsoleLogger.cs

+        /// </summary>
+        /// <param name="verbosity">Level of detail to show in the log.</param>
+        /// <param name="args">Command line arguments for the logger configuration. Currently, only '--tl:off' and '--tl:on' are supported right now.</param>
+        public static ILogger CreateTerminalOrConsoleLogger(LoggerVerbosity verbosity, string[] args)


It is surprising to me that we're not extracting the verbosity from the command line too. Should we?

MichalPavlik added 5 commits November 21, 2024 08:24

Adds a static factory that can create a TerminalLogger or ConsoleLogg…

a8e7cc0

…er instance based on settings and terminal capabilities.

Disable TL progress ctrl codes when verbosity is 'quiet'

f0cbec2

Added --tl:<val> arg support

1685284

Removing debugging code

db35e8a

Merge branch 'main' into dev/mipavlik/tl-static-factory

d38dd70

MichalPavlik requested a review from Copilot January 21, 2025 13:38

Copilot AI reviewed Jan 21, 2025

View reviewed changes

MichalPavlik mentioned this pull request Jan 21, 2025

[17.12] Adding a static factory for the TerminalLogger #11016

Closed

MichalPavlik requested a review from baronfel January 21, 2025 13:50

SimaTian added the Area: Terminal Logger Problems with the livelogger/fancylogger/terminallogger -tl functionality. label Jan 21, 2025

baronfel reviewed Jan 21, 2025

View reviewed changes

MichalPavlik marked this pull request as draft January 27, 2025 14:13

MichalPavlik added 4 commits February 5, 2025 10:26

Merge branch 'main' into dev/mipavlik/tl-static-factory

8dbfeb8

Fixed failing tests

d040fd9

The factory method is now public

94a1378

Unnecessary usings were removed

01607a8

MichalPavlik added 2 commits February 7, 2025 10:40

Moving the TerminalLogger back due to issues with shared sources. Add…

9eb5464

…ed unit test

Updated parameter doc

d2dbfce

MichalPavlik marked this pull request as ready for review February 7, 2025 10:25

JanProvaznik reviewed Feb 7, 2025

View reviewed changes

src/Build/Logging/ConsoleLogger.cs Outdated Show resolved Hide resolved

Addded support for more args prefixes and returned --tl:on option

2defbfe

JanProvaznik approved these changes Feb 7, 2025

View reviewed changes

rainersigwald reviewed Feb 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding a static factory for the TerminalLogger #11318

Adding a static factory for the TerminalLogger #11318

MichalPavlik commented Jan 21, 2025 •

edited

Loading

MichalPavlik commented Jan 21, 2025

baronfel left a comment

baronfel commented Jan 21, 2025

MichalPavlik commented Jan 22, 2025 •

edited

Loading

MichalPavlik commented Jan 27, 2025

MichalPavlik commented Feb 5, 2025

rainersigwald Feb 7, 2025

rainersigwald Feb 7, 2025

rainersigwald Feb 7, 2025

rainersigwald Feb 7, 2025

rainersigwald Feb 7, 2025

Adding a static factory for the TerminalLogger #11318

Are you sure you want to change the base?

Adding a static factory for the TerminalLogger #11318

Conversation

MichalPavlik commented Jan 21, 2025 • edited Loading

Context

Changes Made

Notes

Choose a reason for hiding this comment

MichalPavlik commented Jan 21, 2025

baronfel left a comment

Choose a reason for hiding this comment

baronfel commented Jan 21, 2025

MichalPavlik commented Jan 22, 2025 • edited Loading

MichalPavlik commented Jan 27, 2025

MichalPavlik commented Feb 5, 2025

rainersigwald Feb 7, 2025

Choose a reason for hiding this comment

rainersigwald Feb 7, 2025

Choose a reason for hiding this comment

rainersigwald Feb 7, 2025

Choose a reason for hiding this comment

rainersigwald Feb 7, 2025

Choose a reason for hiding this comment

rainersigwald Feb 7, 2025

Choose a reason for hiding this comment

MichalPavlik commented Jan 21, 2025 •

edited

Loading

MichalPavlik commented Jan 22, 2025 •

edited

Loading