From c99c6eea0baca2a187cbadcab12db53bc9cb0be4 Mon Sep 17 00:00:00 2001
From: Rainer Sigwald <raines@microsoft.com>
Date: Thu, 7 Jan 2021 15:34:29 -0600
Subject: [PATCH] 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.
---
 .../BackEnd/BuildManager/BuildManager.cs      | 23 -------------------
 .../BackEnd/BuildManager/BuildSubmission.cs   |  7 ++++++
 src/MSBuild.UnitTests/XMake_Tests.cs          | 20 ++++++++++++++++
 3 files changed, 27 insertions(+), 23 deletions(-)

diff --git a/src/Build/BackEnd/BuildManager/BuildManager.cs b/src/Build/BackEnd/BuildManager/BuildManager.cs
index 72d7b1e9c51..030adec848d 100644
--- 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
index 3a53fbbf3ee..91356f814c6 100644
--- 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
index 06997104d73..4278d8a6734 100644
--- 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"/>.