Don't log task inputs and outputs when binary logger is enabled#5498
Don't log task inputs and outputs when binary logger is enabled#5498
Conversation
|
@nkolev92 do you think these logging statements are useful anymore at all? Or can we just rely on the binary logger to have this information and remove the logging calls to these messages all together? |
|
I think those messages may have lived out their usefulness now that binary logs are so common. |
When binary logger is present, LogTaskInputs is set, so MSBuild automatically logs all inputs and outputs, there's no need to manually do so. Starting with MSBuild 17.5 taskLogging.IsTaskInputLoggingEnabled is available, but we're still referencing Microsoft.Build.Utilities.v4.0.dll for net472 so it's not available. Instead of using reflection, just read the environment. This results in significant savings to binlog size and potentially faster restore as well (and less memory allocations). Fixes NuGet/Home#10696 See related: * dotnet/msbuild#6305 * dotnet/msbuild#6803
Okay thanks, I agree. I've removed all task input/output logging |
|
|
||
| testLogger.Warnings.Should().Be(0); | ||
| testLogger.Errors.Should().Be(0); | ||
| testLogger.DebugMessages.Count.Should().Be(2); |
There was a problem hiding this comment.
Should these be 0 instead of removing them completely?
There was a problem hiding this comment.
I don't think it matters unless we really care to ensure that a debug message isn't accidentally added. The code diff clearly shows that I removed the existing ones so it seems odd to me to check that they were removed. Do you feel strongly that we should update the test instead of remove this assertion?
There was a problem hiding this comment.
it's one of those that if I was writing the code originally, I would've changed it to 0.
I don't really have a strong reason to add it back though. I'm ok with it as is.
KirillOsenkov
left a comment
There was a problem hiding this comment.
Looks great to me!
Stop logging task inputs and outputs now that the binary logger makes getting this information much easier.
This results in significant savings to binlog size and potentially faster restore as well (and less memory allocations).
Fixes NuGet/Home#10696
See related:
Bug
Fixes:
Regression? Last working version:
Description
PR Checklist
PR has a meaningful title
PR has a linked issue.
Described changes
Tests
Documentation