Implement /WarnAsError to treat specified warnings as errors (#1355)

Specify just /WarnAsError to have all warnings logged as errors as well as have the build fail.
Specify a list of warning codes to have just that set of warnings treated as errors as well as have the build fail.

Targets will still show as succeeded and the tasks will continue to execute but the overall build result will be a failure.

Related to #68 and will close in my next change to add /NoWarn.

* Feature switch FEATURE_RESOURCEMANAGER_GETRESOURCESET
* Support for command-line arguments having empty values

dir.props

@@ -308,6 +308,8 @@
     <DefineConstants>$(DefineConstants);FEATURE_REGISTRYHIVE_DYNDATA</DefineConstants>
     <DefineConstants>$(DefineConstants);FEATURE_RESGEN</DefineConstants>
     <DefineConstants>$(DefineConstants);FEATURE_RESOURCE_EXPOSURE</DefineConstants>
+    <!-- System.Resources.ResourceManager.GetResourceSet() method is currently only in full framework -->
+    <DefineConstants>$(DefineConstants);FEATURE_RESOURCEMANAGER_GETRESOURCESET</DefineConstants>
     <DefineConstants>$(DefineConstants);FEATURE_RESX_RESOURCE_READER</DefineConstants>
     <DefineConstants>$(DefineConstants);FEATURE_RTLMOVEMEMORY</DefineConstants>
     <DefineConstants>$(DefineConstants);FEATURE_RUN_EXE_IN_TESTS</DefineConstants>

ref/net46/Microsoft.Build/Microsoft.Build.cs

@@ -940,6 +940,7 @@ public BuildParameters(Microsoft.Build.Evaluation.ProjectCollection projectColle
         public System.Collections.Generic.ICollection<Microsoft.Build.Evaluation.Toolset> Toolsets { get { throw null; } }
         public System.Globalization.CultureInfo UICulture { get { throw null; } set { } }
         public bool UseSynchronousLogging { get { throw null; } set { } }
+        public System.Collections.Generic.ISet<string> WarningsAsErrors { get { throw null; } set { } }
         public Microsoft.Build.Execution.BuildParameters Clone() { throw null; }
         public Microsoft.Build.Evaluation.Toolset GetToolset(string toolsVersion) { throw null; }
     }

src/XMakeBuildEngine/BackEnd/BuildManager/BuildManager.cs

@@ -424,7 +424,7 @@ public void BeginBuild(BuildParameters parameters)
                 }
 
                 // Set up the logging service.
-                ILoggingService loggingService = CreateLoggingService(_buildParameters.Loggers, _buildParameters.ForwardingLoggers);
+                ILoggingService loggingService = CreateLoggingService(_buildParameters.Loggers, _buildParameters.ForwardingLoggers, _buildParameters.WarningsAsErrors);
 
                 _nodeManager.RegisterPacketHandler(NodePacketType.LogMessage, LogMessagePacket.FactoryForDeserialization, loggingService as INodePacketHandler);
                 try
@@ -575,7 +575,11 @@ public BuildSubmission PendBuildRequest(BuildRequestData requestData)
         public BuildResult BuildRequest(BuildRequestData requestData)
         {
             BuildSubmission submission = PendBuildRequest(requestData);
-            return submission.Execute();
+            BuildResult result = submission.Execute();
+
+            SetOverallResultIfWarningsAsErrors(result);
+
+            return result;
         }
 
         /// <summary>
@@ -621,6 +625,12 @@ public void EndBuild()
 
                 if (loggingService != null)
                 {
+                    // Override the build success if the user specified /warnaserror and any errors were logged outside of a build submission.
+                    if (_overallBuildSuccess && loggingService.HasBuildSubmissionLoggedErrors(BuildEventContext.InvalidSubmissionId))
+                    {
+                        _overallBuildSuccess = false;
+                    }
+
                     loggingService.LogBuildFinished(_overallBuildSuccess);
                 }
 
@@ -1578,6 +1588,9 @@ private void ReportResultsToSubmission(BuildResult result)
                 if (_buildSubmissions.ContainsKey(result.SubmissionId))
                 {
                     BuildSubmission submission = _buildSubmissions[result.SubmissionId];
+
+                    SetOverallResultIfWarningsAsErrors(result);
+
                     submission.CompleteResults(result);
 
                     // If the request failed because we caught an exception from the loggers, we can assume we will receive no more logging messages for
@@ -1723,7 +1736,7 @@ private void OnProjectStarted(object sender, ProjectStartedEventArgs e)
         /// <summary>
         /// Creates a logging service around the specified set of loggers.
         /// </summary>
-        private ILoggingService CreateLoggingService(IEnumerable<ILogger> loggers, IEnumerable<ForwardingLoggerRecord> forwardingLoggers)
+        private ILoggingService CreateLoggingService(IEnumerable<ILogger> loggers, IEnumerable<ForwardingLoggerRecord> forwardingLoggers, ISet<string> warningsAsErrors)
         {
             int cpuCount = _buildParameters.MaxNodeCount;
 
@@ -1750,6 +1763,7 @@ private ILoggingService CreateLoggingService(IEnumerable<ILogger> loggers, IEnum
             loggingService.OnLoggingThreadException += _loggingThreadExceptionEventHandler;
             loggingService.OnProjectStarted += _projectStartedEventHandler;
             loggingService.OnProjectFinished += _projectFinishedEventHandler;
+            loggingService.WarningsAsErrors = warningsAsErrors;
 
             try
             {
@@ -1820,6 +1834,23 @@ private I ExpectPacketType<I>(INodePacket packet, NodePacketType expectedType) w
             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);
+                }
+            }
+        }
+
         /// <summary>
         ///  Shutdown the logging service
         /// </summary>

src/XMakeBuildEngine/BackEnd/BuildManager/BuildParameters.cs

@@ -200,6 +200,11 @@ public class BuildParameters : INodePacketTranslatable
         /// </summary>
         private bool _onlyLogCriticalEvents = false;
 
+        /// <summary>
+        /// A list of warnings to treat as errors.
+        /// </summary>
+        private ISet<string> _warningsAsErrors = null;
+
         /// <summary>
         /// The location of the toolset definitions.
         /// </summary>
@@ -320,6 +325,7 @@ private BuildParameters(BuildParameters other)
             _disableInProcNode = other._disableInProcNode;
             _logTaskInputs = other._logTaskInputs;
             _logInitialPropertiesAndItems = other._logInitialPropertiesAndItems;
+            _warningsAsErrors = other._warningsAsErrors == null ? null : new HashSet<string>(other._warningsAsErrors, StringComparer.OrdinalIgnoreCase);
         }
 
 #if FEATURE_THREAD_PRIORITY
@@ -583,6 +589,15 @@ public bool OnlyLogCriticalEvents
             set { _onlyLogCriticalEvents = value; }
         }
 
+        /// <summary>
+        /// A list of warnings to treat as errors.  To treat all warnings as errors, set this to an empty <see cref="HashSet{String}"/>.  
+        /// </summary>
+        public ISet<string> WarningsAsErrors
+        {
+            get { return _warningsAsErrors; }
+            set { _warningsAsErrors = value; }
+        }
+
         /// <summary>
         /// Locations to search for toolsets.
         /// </summary>

src/XMakeBuildEngine/BackEnd/Components/Logging/BuildEventArgTransportSink.cs

@@ -81,6 +81,19 @@ public bool HaveLoggedBuildFinishedEvent
             set;
         }
 
+        /// <summary>
+        /// This property is ignored by this event sink and relies on the receiver to treat warnings as errors.
+        /// </summary>
+        public ISet<string> WarningsAsErrors
+        {
+            get;
+            set;
+        }
+
+        /// <summary>
+        /// This property is ignored by this event sink and relies on the receiver to keep track of whether or not any errors have been logged.
+        /// </summary>
+        public ISet<int> BuildSubmissionIdsThatHaveLoggedErrors { get; } = null;
         #endregion
         #region IBuildEventSink Methods
 

src/XMakeBuildEngine/BackEnd/Components/Logging/EventSourceSink.cs

@@ -5,6 +5,7 @@
 //-----------------------------------------------------------------------
 
 using System;
+using System.Collections.Generic;
 using Microsoft.Build.Framework;
 using Microsoft.Build.Shared;
 
@@ -129,6 +130,24 @@ public bool HaveLoggedBuildFinishedEvent
             get;
             set;
         }
+
+        /// <summary>
+        /// A list of warnings to treat as errors.  If null, nothing is treated as an error.  If an empty set, all warnings are treated as errors.
+        /// </summary>
+        public ISet<string> WarningsAsErrors
+        {
+            get;
+            set;
+        }
+
+        /// <summary>
+        /// A list of build submission IDs that have logged errors.  If an error is logged outside of a submission, the submission ID is <see cref="BuildEventContext.InvalidSubmissionId"/>.
+        /// </summary>
+        public ISet<int> BuildSubmissionIdsThatHaveLoggedErrors
+        {
+            get;
+        } = new HashSet<int>();
+
         #endregion
 
         #region Methods
@@ -203,7 +222,34 @@ public void Consume(BuildEventArgs buildEvent)
             }
             else if (buildEvent is BuildWarningEventArgs)
             {
-                this.RaiseWarningEvent(null, (BuildWarningEventArgs)buildEvent);
+                BuildWarningEventArgs warningEvent = (BuildWarningEventArgs) buildEvent;
+
+                // Treat this warning as an error if an empty set of warnings was specified or this code was specified
+                if (WarningsAsErrors != null && (WarningsAsErrors.Count == 0 || WarningsAsErrors.Contains(warningEvent.Code)))
+                {
+                    BuildErrorEventArgs errorEvent = new BuildErrorEventArgs(
+                        warningEvent.Subcategory,
+                        warningEvent.Code,
+                        warningEvent.File,
+                        warningEvent.LineNumber,
+                        warningEvent.ColumnNumber,
+                        warningEvent.EndLineNumber,
+                        warningEvent.EndColumnNumber,
+                        warningEvent.Message,
+                        warningEvent.HelpKeyword,
+                        warningEvent.SenderName,
+                        warningEvent.Timestamp)
+                    {
+                        BuildEventContext = warningEvent.BuildEventContext,
+                        ProjectFile = warningEvent.ProjectFile,
+                    };
+
+                    this.RaiseErrorEvent(null, errorEvent);
+                }
+                else
+                {
+                    this.RaiseWarningEvent(null, warningEvent);
+                }
             }
             else if (buildEvent is BuildErrorEventArgs)
             {
@@ -308,6 +354,9 @@ private void RaiseMessageEvent(object sender, BuildMessageEventArgs buildEvent)
         /// <exception cref="Exception">ExceptionHandling.IsCriticalException exceptions will not be wrapped</exception>
         private void RaiseErrorEvent(object sender, BuildErrorEventArgs buildEvent)
         {
+            // Keep track of build submissions that have logged errors.  If there is no build context, add BuildEventContext.InvalidSubmissionId.
+            BuildSubmissionIdsThatHaveLoggedErrors.Add(buildEvent?.BuildEventContext?.SubmissionId ?? BuildEventContext.InvalidSubmissionId);
+
             if (ErrorRaised != null)
             {
                 try

src/XMakeBuildEngine/BackEnd/Components/Logging/ILoggingService.cs

@@ -153,8 +153,24 @@ bool RunningOnRemoteNode
             get;
             set;
         }
+
+        /// <summary>
+        /// Set of warnings to treat as errors.  An empty non-null set will treat all warnings as errors.
+        /// </summary>
+        ISet<string> WarningsAsErrors
+        {
+            get;
+            set;
+        }
         #endregion
 
+        /// <summary>
+        /// Determines if the specified submission has logged an errors.
+        /// </summary>
+        /// <param name="submissionId">The ID of the build submission.  A value of "0" means that an error was logged outside of any build submission.</param>
+        /// <returns><code>true</code> if the build submission logged an errors, otherwise <code>false</code>.</returns>
+        bool HasBuildSubmissionLoggedErrors(int submissionId);
+
         #region Register
 
         /// <summary>
@@ -447,6 +463,20 @@ bool HaveLoggedBuildFinishedEvent
             set;
         }
 
+        /// <summary>
+        /// A list of warnings to treat as errors.  If null, nothing is treated as an error.  If an empty set, all warnings are treated as errors.
+        /// </summary>
+        ISet<string> WarningsAsErrors
+        {
+            get;
+            set;
+        }
+
+        /// <summary>
+        /// A list of build submissions that have logged errors.
+        /// </summary>
+        ISet<int> BuildSubmissionIdsThatHaveLoggedErrors { get; }
+
         #endregion
         /// <summary>
         /// Entry point for a sink to consume an event.
@@ -461,7 +491,7 @@ bool HaveLoggedBuildFinishedEvent
         void Consume(BuildEventArgs buildEvent);
 
         /// <summary>
-        /// Shutsdown the sink and any resources it may be holding
+        /// Shuts down the sink and any resources it may be holding
         /// </summary>
         void ShutDown();
     }

src/XMakeBuildEngine/BackEnd/Components/Logging/LoggingService.cs

@@ -9,6 +9,7 @@
 using System.Collections.Generic;
 using System.Globalization;
 using System.IO;
+using System.Linq;
 using System.Reflection;
 using System.Text;
 using System.Threading;
@@ -218,6 +219,11 @@ internal partial class LoggingService : ILoggingService, INodePacketHandler, IBu
         /// </summary>
         private LoggerMode _logMode = NativeMethodsShared.IsMono ? LoggerMode.Synchronous : LoggerMode.Asynchronous;
 
+        /// <summary>
+        /// A list of warnings to treat as errors.
+        /// </summary>
+        private ISet<string> _warningsAsErrors = null;
+
         #endregion
 
         #endregion
@@ -444,6 +450,33 @@ public LoggerMode LoggingMode
             }
         }
 
+        /// <summary>
+        /// Get of warnings to treat as errors.  An empty non-null set will treat all warnings as errors.
+        /// </summary>
+        public ISet<string> WarningsAsErrors
+        {
+            get { return _warningsAsErrors; }
+            set { _warningsAsErrors = value; }
+        }
+
+        /// <summary>
+        /// Determines if the specified submission has logged an errors.
+        /// </summary>
+        /// <param name="submissionId">The ID of the build submission.  A value of "0" means that an error was logged outside of any build submission.</param>
+        /// <returns><code>true</code> if the build submission logged an errors, otherwise <code>false</code>.</returns>
+        public bool HasBuildSubmissionLoggedErrors(int submissionId)
+        {
+            // Warnings as errors are not tracked if the user did not specify to do so
+            if (WarningsAsErrors == null)
+            {
+                return false;
+            }
+
+            // Determine if any of the event sinks have logged an error with this submission ID
+            return (_filterEventSource != null && _filterEventSource.BuildSubmissionIdsThatHaveLoggedErrors.Contains(submissionId))
+                || (_eventSinkDictionary != null && _eventSinkDictionary.Values.Any(i => i.BuildSubmissionIdsThatHaveLoggedErrors.Contains(submissionId)));
+        }
+
         /// <summary>
         /// Return whether or not the LoggingQueue has any events left in it
         /// </summary>
@@ -784,7 +817,10 @@ public bool RegisterDistributedLogger(ILogger centralLogger, LoggerDescription f
                 IForwardingLogger localForwardingLogger = null;
 
                 // create an eventSourceSink which the central logger will register with to receive the events from the forwarding logger
-                EventSourceSink eventSourceSink = new EventSourceSink();
+                EventSourceSink eventSourceSink = new EventSourceSink
+                {
+                    WarningsAsErrors = WarningsAsErrors == null ? null : new HashSet<string>(WarningsAsErrors, StringComparer.OrdinalIgnoreCase)
+                };
 
                 // If the logger is already in the list it should not be registered again.
                 if (_iloggerList.Contains(centralLogger))
@@ -1096,8 +1132,11 @@ private void CreateFilterEventSource()
         {
             if (_filterEventSource == null)
             {
-                _filterEventSource = new EventSourceSink();
-                _filterEventSource.Name = "Sink for Distributed/Filter loggers";
+                _filterEventSource = new EventSourceSink
+                {
+                    Name = "Sink for Distributed/Filter loggers",
+                    WarningsAsErrors = WarningsAsErrors == null ? null : new HashSet<string>(WarningsAsErrors, StringComparer.OrdinalIgnoreCase)
+                };
             }
         }
 

src/XMakeBuildEngine/BackEnd/Shared/BuildResult.cs

@@ -639,6 +639,15 @@ internal BuildResult Clone()
             return result;
         }
 
+        /// <summary>
+        /// Sets the overall result.
+        /// </summary>
+        /// <param name="overallResult"><code>true</code> if the result is success, otherwise <code>false</code>.</param>
+        internal void SetOverallResult(bool overallResult)
+        {
+            _baseOverallResult = false;
+        }
+
         /// <summary>
         /// Creates the target result dictionary.
         /// </summary>