Tasks Log.HasLoggedError now respects MSBuildWarningsAsErrors

[benvillalobos]

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

• LoggingService now exposes its ShouldLogWarningAsError method through the ILoggingService interface.
• IBuildEngine8 exposes a HashSet<string> that contains all warning codes that were logged as errors.
• TaskHost adds all warning codes that were logged as errors to a HashSet.
• The TaskLoggingHelper's HasLoggedErrors property returns whether it logged an error OR if its build engine has thrown a warning that the task previously logged.

Testing

• Confirmed this fixes the repro in the linked issue.
• Need to test batched builds, as a TaskHost is generated per 'bucket'

Notes

Comment from Rainer in previous PR about this: #5957

The PR that introduced warning-as-error is #1355--#1928 extended it to allow the project level properties.

Since that's done in the logging infrastructure rather than at the point of logging, I think that's the problem. Unfortunately I don't know if there's an easy way to move it. Can you investigate that angle? Is the warning-as-errors stuff available in TaskHost and if not how hard would it be to get it there?

I don't think we should attack the problem for TaskLoggingHelper alone--if you attack it at the IBuildEngine API layer, it'll work for everything, not just tasks that use the helper classes.

[benvillalobos]

Brain dump:
What if we made all HasLoggedWarning methods return a boolean, where true = success and false = turned into error.
LogWarning -> BuildEngine.LogWarning -/> LoggingService.LogWarning . It breaks between BuildEngine.LogWarning and LoggingService.LogWarning because the logging is done async.

• Actually, after it calls the async function we could potentially return true or false. However, this would change the signature of every LogWarning method.

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 HasBuildSubmissionLoggedError method exposed by ILoggingService, but a build submission is more than a single task execution.

Potential solutions

1. Change signature of LogWarning to acount for whether or not it was logged as a warning or an error.
2. Get the set of warnings that will be turned into errors and give it to the TaskHost as it's created.
   • How does this affect the outofproc task host?

[cdmihai]

Is this thread safe?

[benvillalobos]

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:

1. Making sure every logger has access to a taskhost (or the set of warningsaserrors)
2. Ensuring every logger does this check when they want to log a warning
3. Ensuring this isn't a breaking change somehow

Not sure exactly how this would affect an outofproc task host. Any thoughts or suggestions?

[cdmihai]

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)

[benvillalobos]

NTS: Current latest is on warnaserr-fix branch.