Ensure binlogs from file-based apps have evaluation data #49588
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR rearranges the MSBuild initialization sequence in order to include evaluation data in binlogs for file-based apps. Key changes include declaring and reusing an external projectCollection instance, inserting additional BeginBuild calls to force evaluation logging, and removing explicit shutdown calls for binary and console loggers in favor of disposing projectCollection.
Comments suppressed due to low confidence (3)
src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs:203
- [nitpick] Multiple BeginBuild calls are used to ensure the binlog contains Evaluation data, but the difference between the call at this line and the conditional call under NoRestore could be clarified with an additional comment to explain the necessity of both.
BuildManager.DefaultBuildManager.BeginBuild(parameters);
src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs:221
- [nitpick] The conditional BeginBuild call for the NoRestore scenario should be further explained with a comment, clarifying why an additional BeginBuild invocation is required in this branch to capture Evaluation data in the binlog.
{
src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs:251
- [nitpick] The removal of explicit logger shutdown calls in favor of disposing projectCollection should be documented to ensure that the loggers are being disposed of appropriately.
projectCollection?.Dispose();
|
Should we pull some MSBuild folks in here to verify we're doing things in the correct order? |
| @@ -212,6 +215,13 @@ public override int Execute() | |||
| var buildRequest = new BuildRequestData( | |||
| CreateProjectInstance(projectCollection), | |||
| targetsToBuild: [BuildTarget]); | |||
|
|
|||
| // For some reason we need to BeginBuild after creating BuildRequestData otherwise the binlog doesn't contain Evaluation. | |||
There was a problem hiding this comment.
Should we pull some MSBuild folks in here to verify we're doing things in the correct order?
Good idea. @rainersigwald.
If you would like a simplified self-contained repro, use this:
jjonescz/MSBuildInMemory@599e596
that last commit is also the difference between binlog containing evaluation data and not (in the previous commit of the repro app).
There was a problem hiding this comment.
These details are not obvious to me and I'd like @YuliiaKovalova to take a look but don't block merging this on that.
| @@ -161,6 +161,7 @@ public override int Execute() | |||
| } | |||
|
|
|||
| Dictionary<string, string?> savedEnvironmentVariables = []; | |||
| ProjectCollection? projectCollection = null; | |||
There was a problem hiding this comment.
Should we add a test that the binlog from dotnet build app.cs has evaluation data?
There was a problem hiding this comment.
Found two issues in binary log reader (KirillOsenkov/MSBuildStructuredLog#877, KirillOsenkov/MSBuildStructuredLog#878), but managed to add a working test after all :)
| @@ -76,6 +76,7 @@ | |||
| </ItemGroup> | |||
|
|
|||
| <ItemGroup> | |||
| <PackageReference Include="Basic.CompilerLog.Util" /> | |||
There was a problem hiding this comment.
do we need this dependency?
There was a problem hiding this comment.
It's for the binary log reader used in the test. It's true I could use just MSBuild.StructuredLogger dependency, but I'm using Basic.CompilerLog.Util anyway in another PR for more complex tests and it might be useful in other tests as well so why not...
Binlogs from file-based apps didn't have the evaluation data (or at least not visible in the binlog viewer), which is pretty important for build investigations, so I shuffled stuff around until it started working.