Properly report HasLoggedErrors when converting errors into warnings #6912

benvillalobos · 2021-10-05T20:34:32Z

Context

When a task logs an error, its logging helper properly realizes that it logged an error. The overall build, however, converts that error into a warning. This leads to very confusing scenarios where a task can return "I logged an error" that the TaskBuilder recognizes, but doesn't see that an error was logged because it was turned into a warning.

Changes Made

When a taskhost logs an error, even if it was converted into a warning, it understands that an error was logged but does not fail the build.

Testing

Notes

…errors when logging an error

Forgind · 2021-10-07T16:56:38Z

src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs

                IBuildEngine be = host.TaskInstance.BuildEngine;
                if (taskReturned && !taskResult && !taskLoggingContext.HasLoggedErrors && (be is TaskHost th ? th.BuildRequestsSucceeded : false) && (be is IBuildEngine7 be7 ? !be7.AllowFailureWithoutError : true))
                {
-                    if (_continueOnError == ContinueOnError.WarnAndContinue)


Why not log a warning here? The idea was to convert the error into a warning, not to ignore it completely.

The message is redundant in the case that the error was turned into the warning. Though it's worth noting that this hides a real issue where a task sets WarnAndContinue but truly doesn't log an error and returns false. Need to think about this some more.

In practice, this says that if ContinueOnError is set, we don't log MSB4181, period, no matter what they did. HasLoggedErrors can (and should) be set if they tried to log an error but converted it into a warning; a lot of tasks return !HasLoggedErrors anyway. I don't feel at all bad about logging two warnings for a poorly-constructed task, but it is bad to not log anything when a task returns false.

Reading this = 💡

Is the fix here really just...making sure RemoveDir returns !HasLoggedErrors? It straight up returns whether it actually deleted the file 🤦‍♂️

In the implementation of RemoveDir, it returns true or false based on whether or not it actually deleted the directory. However we want our typical task to return true as long as it successfully did something, otherwise it logs an error and reports whether it did. The issue with RemoveDir is it doesn't follow that structure. I think we need to change the logic of how RemoveDir works and add an Output parameter similar to copy's "DidActuallyWriteAFile".

RemoveDir for reference: https://github.com/dotnet/msbuild/blob/main/src/Tasks/RemoveDir.cs

It is probably a good idea to change RemoveDir to use the !Log.HasLoggedErrors pattern that we recommend. But I'm not sure it's wrong to just never log this malformed-task error in the presence of ContinueOnErrors. It is definitely wrong to report it when the task tried to log an error but was prevented.

I agree that the other change in this PR is the critical one. IIRC when we were talking about this I wanted to do this as well but I can't remember exactly why.

Let's do this: please add a new test task (or find a configuration that does this): ReturnFailureAfterLoggingErrorTask, and add a test for it (doesn't log the supplemental error when logging an error that gets converted). That should fail against main and pass with the TaskHost change alone. And maybe that's sufficient?

If we don't already have it, I think it would also be nice to have a separate task that does not attempt to log an error but returns false, then verify that with ContinueOnError.WarnAndContinue, MSB4181 is still displayed, just as a warning.

Forgind · 2021-10-07T16:57:34Z

src/Build/BackEnd/Components/RequestBuilder/TaskHost.cs

                    _taskLoggingContext.LoggingService.LogBuildEvent(e);
-                    _taskLoggingContext.HasLoggedErrors = true;
                }
+                    _taskLoggingContext.HasLoggedErrors = true;


Does this fail the build? If someone want to "WarnAndContinue," we probably shouldn't, but this does seem like the change that would prevent 4181.

Nit: spacing.

src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs

Co-authored-by: Forgind <[email protected]>

src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs

rainersigwald

IIRC there was a request for a new test scenario, right?

rainersigwald · 2021-10-18T15:22:25Z

src/Build/BackEnd/Components/RequestBuilder/TaskHost.cs

                    _taskLoggingContext.LoggingService.LogBuildEvent(e);
-                    _taskLoggingContext.HasLoggedErrors = true;
                }
+                    _taskLoggingContext.HasLoggedErrors = true;


Nit: spacing.

benvillalobos · 2021-10-18T17:10:03Z

@rainersigwald Fixed. Added the deleted test back and a new test to ensure:

Task returns false and logs error does NOT also log msb4181
Task returns false and logs nothing DOES log msb4181
both under the ContinueOnError case.

benvillalobos added 2 commits October 5, 2021 13:29

Only report MSB4181 when not on warnandcontinue. Always set haslogged…

0a5c7c6

…errors when logging an error

Modify tests for new behavior

5eac868

benvillalobos marked this pull request as ready for review October 6, 2021 17:55

Forgind reviewed Oct 7, 2021

View reviewed changes

Update src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs

8232e48

Co-authored-by: Forgind <[email protected]>

Forgind changed the title ~~Propertly report HasLoggedErrors when converting errors into warnings~~ Properly report HasLoggedErrors when converting errors into warnings Oct 11, 2021

benvillalobos added 2 commits October 11, 2021 09:25

Continue logging MSB4181 when warnandcontinue is set

fabbb79

Merge

90a17ae

Forgind reviewed Oct 11, 2021

View reviewed changes

src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs Outdated Show resolved Hide resolved

rainersigwald reviewed Oct 18, 2021

View reviewed changes

Add regression tests

a055191

benvillalobos mentioned this pull request Oct 22, 2021

RemoveDir returns !HasLoggedErrors #6933

Merged

rainersigwald added this to the MSBuild 17.1 milestone Oct 25, 2021

benvillalobos force-pushed the error-logging-fix branch from 53bc1a3 to a055191 Compare October 25, 2021 15:33

Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Oct 25, 2021

rainersigwald approved these changes Oct 25, 2021

View reviewed changes

Satisfy the extra line haters

c903c9c

rokonec merged commit 7c70afa into dotnet:main Oct 27, 2021

dependabot bot mentioned this pull request Aug 15, 2025

Bump Microsoft.Build from 15.9.20 to 17.11.31 rajkumar-rangaraj/opentelemetry-dotnet-instrumentation#1036

Closed

dependabot bot mentioned this pull request Oct 15, 2025

Bump Microsoft.Build from 15.9.20 to 17.11.48 rajkumar-rangaraj/opentelemetry-dotnet-instrumentation#1085

Open

Properly report HasLoggedErrors when converting errors into warnings #6912

Properly report HasLoggedErrors when converting errors into warnings #6912

Uh oh!

Conversation

benvillalobos commented Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Changes Made

Testing

Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rainersigwald left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benvillalobos commented Oct 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

benvillalobos commented Oct 5, 2021 •

edited

Loading