Tasks Log.HasLoggedError now respects MSBuildWarningsAsErrors#6040
Tasks Log.HasLoggedError now respects MSBuildWarningsAsErrors#6040benvillalobos wants to merge 4 commits intodotnet:masterfrom
Log.HasLoggedError now respects MSBuildWarningsAsErrors#6040Conversation
|
Brain dump:
The problem in general with fixing this is that it would require overhauling how warnings are turned into errors. The current implementation is that everything generically gets logged and potentially sent async to a queue of generic build events. It then gets processed and converted accordingly, but by the time it's processed we can't tell the task that logged the warning that it was converted to an error. I thought about trying to use the Potential solutions
|
| bool fillInLocation = (String.IsNullOrEmpty(file) && (lineNumber == 0) && (columnNumber == 0)); | ||
|
|
||
| // Keep track of warnings logged and compare to the what the build engine logged as an error. | ||
| _warningCodesLogged.Add(warningCode); |
There was a problem hiding this comment.
Likely not, I can add a lock here.
Part of me wants to give every taskhost (on creation) the entire set of warnings as errors. Then I can make every task logger check their taskhost and, instead of logging a warning that will eventually be turned into an error, just have it log the error ahead of time.
That'd require:
- Making sure every logger has access to a taskhost (or the set of warningsaserrors)
- Ensuring every logger does this check when they want to log a warning
- Ensuring this isn't a breaking change somehow
Not sure exactly how this would affect an outofproc task host. Any thoughts or suggestions?
There was a problem hiding this comment.
Yea, either lock (this class apparently already has a lock object) or concurrent collection.
Regarding the entire-set-of-warnings-for-each-taskhost approach, there's two benefits right?
- it works with out of proc task host whereas the current approach doesn't because the out of proc task host does not have access to a logging service.
- it's easier to reason about
In reply to: 562785330 [](ancestors = 562785330)
|
NTS: Current latest is on |
Fixes #5511
Context
Tasks have a Log.HasLoggedErrors property that does not respect warnings that were thrown as errors when listed under
MSBuildWarningsAsErrors.For an easier code review, commits 1 & 3 are the most relevant. Then check the main diff.
Changes Made
LoggingServicenow exposes itsShouldLogWarningAsErrormethod through theILoggingServiceinterface.HashSet<string>that contains all warning codes that were logged as errors.HasLoggedErrorsproperty returns whether it logged an error OR if its build engine has thrown a warning that the task previously logged.Testing
TaskHostis generated per 'bucket'Notes
Comment from Rainer in previous PR about this: #5957