search

dotnet / msbuild

The Microsoft Build Engine (MSBuild) is the build platform for .NET and Visual Studio.
https://docs.microsoft.com/visualstudio/msbuild/msbuild
MIT License
5.21k stars 1.35k forks source link

Properties and items logged at evaluation with legacy loggers for single-process builds #7219

Open KirillOsenkov opened 2 years ago

KirillOsenkov commented 2 years ago

We have an unexpected behavior for single-process builds (/bl), where if there are legacy loggers that haven't opted in via IEventSource4.IncludeEvaluationPropertiesAndItems(), and the environment variable MSBUILDLOGPROPERTIESANDITEMSAFTEREVALUATION is not set, we still enable the new behavior if there is at least one "enlightened" logger.

See test results here: https://github.com/dotnet/msbuild/pull/7217#issuecomment-1005345899

See related:

rainersigwald commented 2 years ago

Looked at this a bit this morning. Loggers work with an event-based approach, where one "SourceSink" of events can feed multiple loggers.

For a build with the default console logger, the binlog, and a random unenlightened logger, I see only one _eventSinkDictionary here:

https://github.com/dotnet/msbuild/blob/b827bf58c21f7a38770d786848c76f771d995a94/src/Build/BackEnd/Components/Logging/LoggingService.cs#L528-L530

I can also see the ProjectCollection.ReusableLogger's internal state here

https://github.com/dotnet/msbuild/blob/b827bf58c21f7a38770d786848c76f771d995a94/src/Build/Definition/ProjectCollection.cs#L2054

getting toggled while registering the ParallelConsoleLogger.

The second logger gets registered through this path:

https://github.com/dotnet/msbuild/blob/b827bf58c21f7a38770d786848c76f771d995a94/src/Build/BackEnd/Components/Logging/LoggingService.cs#L926-L931

So it sees only the EventSourceSink created in the first logger, which had IncludeEvaluationPropertiesAndItems toggled on.

In the multiproc case, we actually pass only the non-console loggers to the ProjectCollection, because the console logger is a DistributedLoggerRecord which are treated separately

https://github.com/dotnet/msbuild/blob/b827bf58c21f7a38770d786848c76f771d995a94/src/MSBuild/XMake.cs#L3202-L3220

https://github.com/dotnet/msbuild/blob/b827bf58c21f7a38770d786848c76f771d995a94/src/MSBuild/XMake.cs#L1027-L1030

and passed to the BuildParameters rather than the ProjectCollection

https://github.com/dotnet/msbuild/blob/b827bf58c21f7a38770d786848c76f771d995a94/src/MSBuild/XMake.cs#L1168

So I think the current filtering mechanism just doesn't work, and we've been squeaking by with luck :(

KirillOsenkov commented 2 years ago

Thanks for looking. When I added support for IncludePropertiesAndItems I copy pasted existing support for other settings, which probably have the same issue :|

KirillOsenkov commented 1 year ago

I just noticed that MuxLogger.SubmissionRecord only supports IEventSource2.

I think It needs to be enlightened to support IEventSource4: https://github.com/dotnet/msbuild/blob/2cbc8b6aef648cf21c6a68a0dab7fe09a614e475/src/Utilities/MuxLogger.cs#L339

Here I think: https://github.com/dotnet/msbuild/blob/2cbc8b6aef648cf21c6a68a0dab7fe09a614e475/src/Utilities/MuxLogger.cs#L1336

KirillOsenkov commented 1 year ago

Hmm, that's probably fine. I'm not even sure what SubmissionRecord is used for or whether that is relevant in any way.