Skip to content

Properly report HasLoggedErrors when converting errors into warnings #6912

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Oct 27, 2021
Merged

Properly report HasLoggedErrors when converting errors into warnings #6912

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0a5c7c6
Only report MSB4181 when not on warnandcontinue. Always set haslogged…
benvillalobos Oct 5, 2021
5eac868
Modify tests for new behavior
benvillalobos Oct 6, 2021
8232e48
Update src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs
benvillalobos Oct 7, 2021
fabbb79
Continue logging MSB4181 when warnandcontinue is set
benvillalobos Oct 11, 2021
90a17ae
Merge
benvillalobos Oct 11, 2021
a055191
Add regression tests
benvillalobos Oct 18, 2021
c903c9c
Satisfy the extra line haters
benvillalobos Oct 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 0 additions & 22 deletions src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,28 +417,6 @@ public void TaskReturnsFailureButDoesNotLogError_ShouldCauseBuildFailure()
}
}

[Fact]
public void TaskReturnsFailureButDoesNotLogError_ContinueOnError_WarnAndContinue()
{
using (TestEnvironment env = TestEnvironment.Create(_output))
{
TransientTestProjectWithFiles proj = env.CreateTestProjectWithFiles($@"
<Project>
<UsingTask TaskName = ""ReturnFailureWithoutLoggingErrorTask"" AssemblyName=""Microsoft.Build.Engine.UnitTests""/>
<Target Name='Build'>
<ReturnFailureWithoutLoggingErrorTask
ContinueOnError=""WarnAndContinue""/>
</Target>
</Project>");

MockLogger logger = proj.BuildProjectExpectSuccess();

logger.WarningCount.ShouldBe(1);

logger.AssertLogContains("MSB4181");
}
}

[Fact]
public void TaskReturnsFailureButDoesNotLogError_ContinueOnError_True()
{
Expand Down
1 change: 1 addition & 0 deletions src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -940,6 +940,7 @@ private async Task<WorkUnitResult> ExecuteInstantiatedTask(ITaskExecutionHost ta
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)
Copy link
Contributor

@Forgind Forgind Oct 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@benvillalobos benvillalobos Oct 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@Forgind Forgind Oct 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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.

Copy link
Member Author

@benvillalobos benvillalobos Oct 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@rainersigwald rainersigwald Oct 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@Forgind Forgind Oct 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
taskLoggingContext.LogWarning(null,
Expand Down
2 changes: 1 addition & 1 deletion src/Build/BackEnd/Components/RequestBuilder/TaskHost.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,8 +455,8 @@ public void LogErrorEvent(Microsoft.Build.Framework.BuildErrorEventArgs e)
{
e.BuildEventContext = _taskLoggingContext.BuildEventContext;
_taskLoggingContext.LoggingService.LogBuildEvent(e);
_taskLoggingContext.HasLoggedErrors = true;
}
_taskLoggingContext.HasLoggedErrors = true;
Copy link
Contributor

@Forgind Forgind Oct 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@rainersigwald rainersigwald Oct 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: spacing.

}
}

Expand Down