Log and include response files in binlog#8146
Conversation
Specific version was added to avoid conflicts with newest internal version of dotnet and VS. Removed for PR
Co-authored-by: Rainer Sigwald <raines@microsoft.com>
src/MSBuild/XMake.cs
Outdated
There was a problem hiding this comment.
It probably doesn't matter too much just because there probably aren't too many response files, but this is probably faster if you add to lists. (I'm happy trying to verify or falsify that with you if you'd like to see how Benchmark.NET works!)
There was a problem hiding this comment.
I definitely agree here. Collect the initial list with .ToList(), then add to it in this loop. I think it can continue to be a local variable, too.
35f6f0e to
7ac66f7
Compare
rainersigwald
left a comment
There was a problem hiding this comment.
In offline discussion with @Forgind, an idea came up: is it possible to pass in a list of BuildEventArgs, rather than DeferredBuildMessage, so that if we decide we need another type in the future it's already there?
I'd also like to see some end-to-end tests of this functionality. Probably in src\Build.UnitTests\BinaryLogger_Tests.cs?
src/MSBuild/XMake.cs
Outdated
There was a problem hiding this comment.
I definitely agree here. Collect the initial list with .ToList(), then add to it in this loop. I think it can continue to be a local variable, too.
Addresses dotnet#7214. A message is logged at the beginning og a build whenever a response file(s) is used.
Addresses dotnet#7214. Using custom events, the binlogger includes the response files used when running MSBuild.
Code formatting. Fixes identation, spacing and adds missing comments
For greater flexibility and avoiding massive changes in the codebase, DeferredResponseFile has been removed. Instead, DeferredBuildMessage now has a FilePath member that when not blank, will inlcude the specified file in the binlogger
ResponseFilePath member is initialized as an empty string by default.
Co-authored-by: Rainer Sigwald <raines@microsoft.com>
Co-authored-by: Rainer Sigwald <raines@microsoft.com>
4a48c5d to
933cbd4
Compare
rainersigwald
left a comment
There was a problem hiding this comment.
A couple of style nits, but I think the big thing is to add a test, which should make the changes fairly clear.
Added RoadtripResponseFileUsedEventArgs test and specific methods at BuildEventArgsReader.cs and BuildEventArgsWriter.cs for ResponseFileUsedEventArgs
Added trailing comma to enum BinaryLogRecordKind, and replaced Message ith ResponseFilePath in tests
Fixed styling, removed unused code and did testing
Bumped to support ResponseFileUedEventArgs
…e-response-files
…-response-files
Hopefully works now 🤞
Line for assigning ResponseFilePath to ResponseFileUsedEventArgs was removed by mistake
…-response-files
src/MSBuild/XMake.cs
Outdated
| /// <summary> | ||
| /// Messages to be logged into loggrers | ||
| /// </summary> | ||
| private static IEnumerable<BuildManager.DeferredBuildMessage> messagesToLogInBuildLoggers = Enumerable.Empty<BuildManager.DeferredBuildMessage>(); |
There was a problem hiding this comment.
I'm unclear on why this has to exist at all, but ¯\(ツ)/¯
|
Lot of commits here. @dotnet/kitten please make sure to squash :) |
|
This looks good, the only thing I'd change is rename to just |
|
Binlog viewer version 2.1.758 has support for binlog format version 15 |
Fixes #7214
Context
When using response files, it can be easy to lose track of all switches used. For that reason, it would be helpful to log a message at the beginning of each build and also include them in the binary logger.
Changes Made
ResponseFileUsedEventArgswas implementedTesting
Testing was done with several resp files
Notes
Changes are only applicable to standalone msbuild, as with dotnet, dotnet itself analyzes and expands the response files