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

Make Terminal Logger respect verbosity #9810

Merged
merged 15 commits into from
Apr 3, 2024

Conversation

AR-May
Copy link
Member

@AR-May AR-May commented Mar 1, 2024

Fixes #9667, #9654

Context

We would like to make the terminal logger partially respect verbosity.

Changes Made

In case of Quiet verbosity, none of the static part of the Terminal Logger output, which is grouped by the project, is shown. Warnings and errors are shown as they come immediately.
In case of Minimal and Normal verbosity, the behavior stands intact.
In case of Detailed and Diagnostic verbosity, the terminal logger shows all the high priority messages in the static part of the output, under the corresponding project.

Testing

Locally & unit tests

@baronfel
Copy link
Member

baronfel commented Mar 1, 2024

Is is possible to show renderings of some of the test 'verified' files/scenarios? It'd be good to be able to visually verify the changes here.

I was able to use this tool to generate these locally, with this script (run from the Snapshots directory):

$verifieds = @(get-childitem TerminalLogger_Tests.*.verified.txt)

foreach ($snapshot in $verifieds) {
    $name = [System.IO.Path]::GetFileNameWithoutExtension($snapshot)
    get-content $snapshot | C:\Users\chusk\go\bin\ansisvg.exe > "$name.svg"
}

@AR-May
Copy link
Member Author

AR-May commented Mar 4, 2024

Is is possible to show renderings of some of the test 'verified' files/scenarios? It'd be good to be able to visually verify the changes here.

Sure, thank you for the pointers how to do the rendering from the test verification files.
Here are different verbosities levels for the same input (which consists of immediate message, messages with all verbosities, warning and error, all for one project):
Quiet:
TerminalLogger_Tests PrintBuildSummaryQuietVerbosity_FailedWithErrors Windows verified

Minimal, Normal:
TerminalLogger_Tests PrintBuildSummaryNormalVerbosity_FailedWithErrors Windows verified

Detailed, Diagnostic:
TerminalLogger_Tests PrintBuildSummaryDiagnosticVerbosity_FailedWithErrors Windows verified

Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to short-circuit return from the methods on quiet mode - so that diff would get less cluttered

src/MSBuild/TerminalLogger/TerminalLogger.cs Outdated Show resolved Hide resolved
src/MSBuild/TerminalLogger/TerminalLogger.cs Show resolved Hide resolved
src/MSBuild/TerminalLogger/TerminalLogger.cs Outdated Show resolved Hide resolved
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Do you want to flip to "ready for review"?

@AR-May AR-May marked this pull request as ready for review March 18, 2024 12:33
@AR-May AR-May changed the title Draft: Make Terminal Logger respect verbosity Make Terminal Logger respect verbosity Mar 18, 2024
@AR-May AR-May self-assigned this Mar 18, 2024
@AR-May AR-May requested a review from rokonec March 19, 2024 16:52
@AR-May
Copy link
Member Author

AR-May commented Mar 20, 2024

I wonder should we put this change under a change wave?

@baronfel
Copy link
Member

IMO no - terminal logger is a live, always updating feature

@AR-May
Copy link
Member Author

AR-May commented Mar 21, 2024

I ran the TL with detailed verbosity on multiple repositories to check the behavior. All seems to be fine, except for one very annoying line that shows on detailed and diagnostics verbosity. It is a line produced by "exec" task and it shows the exec command line for the compiler call as High priority message. The message is usually huge. The console logger has a work-around that allows not to show this line when minimal verbosity is used. Maybe not for this PR, but should we consider implementing similar workaround in TL or fixing the priority of this message?

@baronfel
Copy link
Member

Maybe not for this PR, but should we consider implementing similar workaround in TL or fixing the priority of this message?

Yes, we should detect + workaround this. I'm not sure we can change the priority, maybe there is some other flag/signal we can set or detect?

Now that we have forward-compatibility this could almost be a specific message type :D

@AR-May
Copy link
Member Author

AR-May commented Mar 21, 2024

Maybe not for this PR, but should we consider implementing similar workaround in TL or fixing the priority of this message?

Yes, we should detect + workaround this. I'm not sure we can change the priority, maybe there is some other flag/signal we can set or detect?

Now that we have forward-compatibility this could almost be a specific message type :D

Well, we have a specific message type TaskCommandLineEventArgs and that is how it is detected in the console logger and skipped. Although the console logger has also a specific parameter that allows for it to be shown, and I suggest having it too. In this case it would be easier for me to implement it in the second PR concerning verbosity where I add parsing of /tlp. @baronfel do we mind if some of MSBuild versions (after merging this PR and before merging my next PR concerning the verbosity) will write this annoying line for a detailed verbosity?

If needed, I can add this in this PR though.

@AR-May
Copy link
Member Author

AR-May commented Mar 22, 2024

Adding the parameter to terminal logger parameters appeared to be easier (apparently, we already have most of the code in place), so I added it in this PR.
So, the changes:

  • parameter /tlp:v=... will overwrite the verbosity of the TL that is set by /v parameter. In case of bad parameter value, it throws the error, like /clp:v=... does.
  • I added a parameter /tlp:SHOWCOMMANDLINE for opting-in the TaskCommandLineEventArgs messages, similar to the /clp:SHOWCOMMANDLINE parameter. By default, we will skip TaskCommandLineEventArgs messages on detailed and diagnostic verbosity of TL, as the console logger does so on minimal verbosity. In case of bad parameter value does not throw an error, just ignores it, like /clp does.

@JanKrivanek, @rokonec could you please review couple of latest commits?
@baronfel is the change and workaround for opting the line back in fine for you?

@AR-May AR-May requested review from JanKrivanek and rokonec March 22, 2024 16:07
@baronfel
Copy link
Member

@AR-May that seems very acceptable to me, thank you!

Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
Added some suggestions for improvements

src/Build/Logging/BaseConsoleLogger.cs Show resolved Hide resolved
src/MSBuild/TerminalLogger/TerminalLogger.cs Outdated Show resolved Hide resolved
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.

Some way to opt specific messages into printing in TerminalLogger
5 participants