Support ProjectConfigurationDescription when SDK loggers are attached by rainersigwald · Pull Request #7297 · dotnet/msbuild

rainersigwald · 2022-01-14T21:53:19Z

Unblocks dotnet/sdk#12030 by fixing problems that arose when combining dotnet build with ProjectConfigurationDescription.

Part of #1222.

I recommend reviewing commit-by-commit.

ladipro · 2022-01-20T13:42:47Z

Checking _forwardingSetFromParameters here means that the logic is now order-dependent. If we hit a _forwardingTable key first, FORWARDPROJECTCONTEXTEVENTS will be ignored. If we hit FORWARDPROJECTCONTEXTEVENTS first, it will be applied. Is this intentional?

Mostly! At first I didn't do this, and some tests failed because they were specifying individual events and got more than they bargained for. I refactored to this and justify it like this: if you go to the trouble of specifying individual events we should respect that.

But I'm happy to hear other ideas/prioritizations!

Gotcha - and the only way to specify FORWARDPROJECTCONTEXTEVENTS is the new code in CreateForwardingLoggerRecord? Or is it possible that the logger will see, e.g.

FORWARDPROJECTCONTEXTEVENTS;LOWMESSAGEEVENT
and not
LOWMESSAGEEVENT;FORWARDPROJECTCONTEXTEVENTS
?

The two would have different behavior which I, as a drive-by reviewer, find suspicious.

horribly: no, that's not the only way. You can in fact pass those on the command line, and you can horribly break logging today by doing, for instance:

❯ msbuild -flp:v=diag .\MSBuild.Dev.slnf /clp:PROJECTSTARTEDEVENT /v:d Microsoft (R) Build Engine version 17.1.0-preview-22055-02+797fd829a for .NET Framework Copyright (C) Microsoft Corporation. All rights reserved. Build started 1/20/2022 3:50:42 PM. Build succeeded. 0 Warning(s) 0 Error(s) Time Elapsed 00:00:38.98

(note no actual output despite fairly high verbosity because most of the events were just dropped)

These are documented in https://docs.microsoft.com/visualstudio/msbuild/writing-multi-processor-aware-loggers. Do you think it would be reasonable to just not document FORWARDPROJECTCONTEXTEVENTS? I was hoping that that would get us to the point where we could say with some confidence "only we will add this and we will only ever append it so we can assume it's last". But that's not exactly an iron-clad guarantee.

Do you think it would be reasonable to just not document FORWARDPROJECTCONTEXTEVENTS?

I think it's reasonable. Another option would be to document it, together with its overriding behavior, and tweak the code to just set a flag here and move the actual setting of _forwardingTable to ParseParameters. There is already some overriding implemented there so it wouldn't look out of place.

msbuild/src/Build/Logging/DistributedLoggers/ConfigurableForwardingLogger.cs

Lines 116 to 122 in 90275e4

// Setting events to forward on the commandline will override the verbosity and other switches such as

// showPerfSummand and ShowSummary

if (_forwardingSetFromParameters)

{

_showPerfSummary = false;

_showSummary = true;

}

I like that and switched to it.

benvillalobos · 2022-01-22T00:53:50Z

I'm somewhat confused by the naming here. should ForwardProjectContentsDescription be ForwardProjectContextDescription?

This was producing ugly strings like [S:\project.csproj:: TargetFramework=net5.0] Instead, only add the space when there's something to separate.

Messages emitted directly from a project file (for instance via `Warning` tasks) weren't getting the disambiguator, which seems wrong.

Using some fancier Shouldly features to make this test easier to understand.

rainersigwald · 2022-02-09T16:27:47Z

Re-drafted because this isn't working right in conjunction with dotnet/sdk#12030. Looking.

dotnet/sdk#12030 discovered that the .NET SDK's default build process didn't log TargetFramework details for normal projects. This turned out to be because the .NET SDK attaches a custom logger, which caused MSBuild to set up a ConfigurableForwardingLogger in front of the ConsoleLogger that forwarded based on verbosity which wasn't high enough to include the ProjectEvaluationFinished event. Pass a new ConfigurableForwardingLogger parameter from the command line handler to instruct it to pass those events even at lower verbosities.

rainersigwald · 2022-02-11T21:40:10Z

Ok, with #7386 + this PR, all looks good:

❯ dotnet msbuild S:\play\multitargeted\multitargeted.csproj -t:rebuild
Microsoft (R) Build Engine version 17.2.0-dev-22111-01+f63aed75f for .NET
Copyright (C) Microsoft Corporation. All rights reserved.

S:\play\multitargeted\multitargeted.csproj(8,5): warning : Warning from MSBuild target [TargetFramework=net48]
S:\play\multitargeted\Class1.cs(9,10): warning CS1030: #warning: 'Warning from C# in all TFs' [S:\play\multitargeted\multitargeted.csproj::TargetFramework=net48]
S:\play\multitargeted\Class1.cs(12,10): warning CS1030: #warning: 'Warning from C# in NETFRAMEWORK TF' [S:\play\multitargeted\multitargeted.csproj::TargetFramework=net48]
  multitargeted -> S:\play\multitargeted\bin\Debug\net48\multitargeted.dll
S:\play\multitargeted\multitargeted.csproj(8,5): warning : Warning from MSBuild target [TargetFramework=net5.0]
S:\play\multitargeted\Class1.cs(9,10): warning CS1030: #warning: 'Warning from C# in all TFs' [S:\play\multitargeted\multitargeted.csproj::TargetFramework=net5.0]
S:\play\multitargeted\Class1.cs(16,10): warning CS1030: #warning: 'Warning from C# in NETCOREAPP TF' [S:\play\multitargeted\multitargeted.csproj::TargetFramework=net5.0]
  multitargeted -> S:\play\multitargeted\bin\Debug\net5.0\multitargeted.dll

rainersigwald added this to the VS 17.2 milestone Jan 14, 2022

rainersigwald added the Area: Logging label Jan 14, 2022

rainersigwald changed the title ~~Support FormatEventMessage when SDK loggers are attached~~ Support ProjectConfigurationDescription when SDK loggers are attached Jan 14, 2022

rainersigwald marked this pull request as draft January 14, 2022 22:08

rainersigwald force-pushed the fancy-formatting-works-in-cli branch from 92fc308 to 507db8f Compare January 14, 2022 22:38

rainersigwald marked this pull request as ready for review January 14, 2022 23:17

rokonec approved these changes Jan 19, 2022

View reviewed changes

ladipro reviewed Jan 20, 2022

View reviewed changes

ladipro approved these changes Jan 21, 2022

View reviewed changes

benvillalobos approved these changes Jan 22, 2022

View reviewed changes

rainersigwald self-assigned this Feb 7, 2022

rainersigwald force-pushed the fancy-formatting-works-in-cli branch from 507db8f to e4f71f7 Compare February 8, 2022 19:33

rainersigwald added 3 commits February 8, 2022 13:58

Don't space first ProjectConfigurationDescription

7565126

This was producing ugly strings like [S:\project.csproj:: TargetFramework=net5.0] Instead, only add the space when there's something to separate.

Log project-file messages with disambiguator

1fadfe6

Messages emitted directly from a project file (for instance via `Warning` tasks) weren't getting the disambiguator, which seems wrong.

Improve diagnosability of ProcessConsoleLoggerSwitches

3e8e9d1

Using some fancier Shouldly features to make this test easier to understand.

rainersigwald force-pushed the fancy-formatting-works-in-cli branch 2 times, most recently from 7f41230 to 41aeedf Compare February 8, 2022 20:08

rainersigwald marked this pull request as draft February 9, 2022 16:27

rainersigwald force-pushed the fancy-formatting-works-in-cli branch from 41aeedf to 6b58f62 Compare February 11, 2022 21:39

rainersigwald marked this pull request as ready for review February 11, 2022 21:40

benvillalobos approved these changes Feb 16, 2022

View reviewed changes

rainersigwald added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Feb 17, 2022

rokonec merged commit d846c19 into dotnet:main Feb 24, 2022

rainersigwald mentioned this pull request May 10, 2022

Adds default outputProperties Item dotnet/sdk#12030

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support ProjectConfigurationDescription when SDK loggers are attached#7297

Support ProjectConfigurationDescription when SDK loggers are attached#7297
rokonec merged 4 commits intodotnet:mainfrom
rainersigwald:fancy-formatting-works-in-cli

rainersigwald commented Jan 14, 2022

Uh oh!

ladipro Jan 20, 2022

Uh oh!

rainersigwald Jan 20, 2022

Uh oh!

ladipro Jan 20, 2022

Uh oh!

rainersigwald Jan 20, 2022

Uh oh!

ladipro Jan 21, 2022

Uh oh!

rainersigwald Feb 8, 2022

Uh oh!

benvillalobos Jan 22, 2022

Uh oh!

rainersigwald Feb 8, 2022

Uh oh!

rainersigwald commented Feb 9, 2022

Uh oh!

rainersigwald commented Feb 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	// Setting events to forward on the commandline will override the verbosity and other switches such as
	// showPerfSummand and ShowSummary
	if (_forwardingSetFromParameters)
	{
	_showPerfSummary = false;
	_showSummary = true;
	}

Conversation

rainersigwald commented Jan 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rainersigwald commented Feb 9, 2022

Uh oh!

rainersigwald commented Feb 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants