Warnings-as-errors fail BuildSubmission results (#6006)

Fixes #5837 by using the approach used in BuildManager to check whether warnings-as-errors were emitted to convert the result to a failure at the submission level, because MSBuild was promoting it to failure too late (in time to log it but not to fail the msbuild invocation. Remove SetOverallResultIfWarningsAsErrors since it's handled at the submission level now.
dotnet · Jan 7, 2021 · c99c6ee · c99c6ee
1 parent 56b853f
commit c99c6ee
Show file tree

Hide file tree

Showing 3 changed files with 27 additions and 23 deletions.
diff --git a/src/Build/BackEnd/BuildManager/BuildManager.cs b/src/Build/BackEnd/BuildManager/BuildManager.cs
@@ -693,8 +693,6 @@ public BuildResult BuildRequest(BuildRequestData requestData)
             BuildSubmission submission = PendBuildRequest(requestData);
             BuildResult result = submission.Execute();
 
-            SetOverallResultIfWarningsAsErrors(result);
-
             return result;
         }
 
@@ -2084,8 +2082,6 @@ private void CheckSubmissionCompletenessAndRemove(BuildSubmission submission)
                 // If the submission has completed or never started, remove it.
                 if (submission.IsCompleted || submission.BuildRequest == null)
                 {
-                    SetOverallResultIfWarningsAsErrors(submission.BuildResult);
-
                     _buildSubmissions.Remove(submission.SubmissionId);
 
                     // Clear all cached SDKs for the submission
@@ -2345,25 +2341,6 @@ private static I ExpectPacketType<I>(INodePacket packet, NodePacketType expected
             return castPacket;
         }
 
-        /// <summary>
-        /// Sets the overall result of a build only if the user had specified /warnaserror and there were any errors.
-        /// This ensures the old behavior stays intact where builds could succeed even if a failure was logged.
-        /// </summary>
-        private void SetOverallResultIfWarningsAsErrors(BuildResult result)
-        {
-            if (result?.OverallResult == BuildResultCode.Success)
-            {
-                ILoggingService loggingService = ((IBuildComponentHost)this).LoggingService;
-
-                if (loggingService.HasBuildSubmissionLoggedErrors(result.SubmissionId))
-                {
-                    result.SetOverallResult(overallResult: false);
-
-                    _overallBuildSuccess = false;
-                }
-            }
-        }
-
         /// <summary>
         ///  Shutdown the logging service
         /// </summary>

diff --git a/src/Build/BackEnd/BuildManager/BuildSubmission.cs b/src/Build/BackEnd/BuildManager/BuildSubmission.cs
@@ -198,6 +198,13 @@ private void CheckForCompletion()
                 bool hasCompleted = (Interlocked.Exchange(ref _completionInvoked, 1) == 1);
                 if (!hasCompleted)
                 {
+                    // Did this submission have warnings elevated to errors? If so, mark it as
+                    // failed even though it succeeded (with warnings--but they're errors).
+                    if (((IBuildComponentHost)BuildManager).LoggingService.HasBuildSubmissionLoggedErrors(BuildResult.SubmissionId))
+                    {
+                        BuildResult.SetOverallResult(overallResult: false);
+                    }
+
                     _completionEvent.Set();
 
                     if (_completionCallback != null)

diff --git a/src/MSBuild.UnitTests/XMake_Tests.cs b/src/MSBuild.UnitTests/XMake_Tests.cs
@@ -2215,6 +2215,26 @@ public void BinaryLogContainsImportedFiles()
             }
         }
 
+        [Fact]
+        public void EndToEndWarnAsErrors()
+        {
+            using TestEnvironment testEnvironment = UnitTests.TestEnvironment.Create();
+
+            string projectContents = ObjectModelHelpers.CleanupFileContents(@"<Project>
+
+  <Target Name=""IssueWarning"">
+    <Warning Text=""Warning!"" />
+  </Target>
+  
+</Project>");
+
+            TransientTestProjectWithFiles testProject = testEnvironment.CreateTestProjectWithFiles(projectContents);
+
+            RunnerUtilities.ExecMSBuild($"\"{testProject.ProjectFile}\" -warnaserror", out bool success, _output);
+
+            success.ShouldBeFalse();
+        }
+
 #if FEATURE_ASSEMBLYLOADCONTEXT
         /// <summary>
         /// Ensure that tasks get loaded into their own <see cref="System.Runtime.Loader.AssemblyLoadContext"/>.