From c2f9b76bb511c2ef4419abe468e81855781c40a1 Mon Sep 17 00:00:00 2001 From: Jenny Bai Date: Tue, 9 Jul 2024 11:21:26 +0800 Subject: [PATCH] Fix build check build submission errors (#10227) Fixes #10071 Context While building with buildcheck enabled and MSBUILDNOINPROCNODE=1 Severity of the rule set to Error, does not reported to the all build: image This is since buildcheck error is not added into the _warningsAsErrorsByProject, WarningsAsErrors == null && _warningsAsErrorsByProject == null is true all the time. so HasBuildSubmissionLoggedErrors always return false. msbuild/src/Build/BackEnd/Components/Logging/LoggingService.cs Lines 577 to 587 in 843bfa5 public bool HasBuildSubmissionLoggedErrors(int submissionId) { // Warnings as errors are not tracked if the user did not specify to do so if (WarningsAsErrors == null && _warningsAsErrorsByProject == null) { return false; } // Determine if any of the event sinks have logged an error with this submission ID return _buildSubmissionIdsThatHaveLoggedErrors?.Contains(submissionId) == true; } Treat warning as errors or message, the buildcheckResultWarning doesn't initialize the code. So when the code of BuildWarningEventArgs is null. ShouldTreatWarningAsError returns false all the time. msbuild/src/Build/BackEnd/Components/Logging/LoggingService.cs Lines 1897 to 1908 in a9c95c7 /// true if the warning should be treated as an error, otherwise false. private bool ShouldTreatWarningAsError(BuildWarningEventArgs warningEvent) { // This only applies if the user specified /warnaserror from the command-line or added an empty set through the object model if (WarningsAsErrors != null) { // Global warnings as errors apply to all projects. If the list is empty or contains the code, the warning should be treated as an error if ((WarningsAsErrors.Count == 0 && WarningAsErrorNotOverriden(warningEvent)) || WarningsAsErrors.Contains(warningEvent.Code)) { return true; } } Changes Made when buildEventArgs is BuildErrorEventArgs, treat BuildErrorEventArgs' related warnings as errors Initialize the code of BuildCheckResultWarning that is inherited from BuildWarningEventArgs Testing Manually testing on local now set MSBUILDNOINPROCNODE=1 and change the build_check.BC0101.Severity= Error dotnet D:\WORK\msbuild\artifacts\bin\bootstrap\net8.0\MSBuild\MSBuild.dll FooBar.csproj /m:1 -nr:False -restore -analyze Change build_check.BC0101.Severity= warning dotnet D:\WORK\msbuild\artifacts\bin\bootstrap\net8.0\MSBuild\MSBuild.dll FooBar.csproj /m:1 -nr:False -restore -analyze -warnaserror --- src/Build/BackEnd/Components/Logging/LoggingService.cs | 7 +++++++ src/Build/BuildCheck/API/BuildCheckResult.cs | 6 +++--- src/Framework/BuildCheck/BuildCheckEventArgs.cs | 6 ++++-- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/Build/BackEnd/Components/Logging/LoggingService.cs b/src/Build/BackEnd/Components/Logging/LoggingService.cs index 0c7866e53a9..6aca33a892d 100644 --- a/src/Build/BackEnd/Components/Logging/LoggingService.cs +++ b/src/Build/BackEnd/Components/Logging/LoggingService.cs @@ -15,6 +15,7 @@ using Microsoft.Build.Shared; using InternalLoggerException = Microsoft.Build.Exceptions.InternalLoggerException; using LoggerDescription = Microsoft.Build.Logging.LoggerDescription; +using Microsoft.Build.Experimental.BuildCheck; #nullable disable @@ -1592,6 +1593,12 @@ private void RouteBuildEvent(object loggingEvent) _buildSubmissionIdsThatHaveLoggedErrors.Add(errorEvent.BuildEventContext?.SubmissionId ?? BuildEventContext.InvalidSubmissionId); } + if (buildEventArgs is BuildCheckResultError checkResultError) + { + // If the specified BuildCheckResultError was issued, an empty ISet signifies that the specified build check warnings should be treated as errors. + AddWarningsAsErrors(checkResultError.BuildEventContext, new HashSet()); + } + if (buildEventArgs is ProjectFinishedEventArgs projectFinishedEvent && projectFinishedEvent.BuildEventContext != null) { WarningsConfigKey key = GetWarningsConfigKey(projectFinishedEvent); diff --git a/src/Build/BuildCheck/API/BuildCheckResult.cs b/src/Build/BuildCheck/API/BuildCheckResult.cs index 0f70c5228b7..b6cb67e7d56 100644 --- a/src/Build/BuildCheck/API/BuildCheckResult.cs +++ b/src/Build/BuildCheck/API/BuildCheckResult.cs @@ -32,8 +32,8 @@ internal BuildEventArgs ToEventArgs(BuildAnalyzerResultSeverity severity) => severity switch { BuildAnalyzerResultSeverity.Suggestion => new BuildCheckResultMessage(this), - BuildAnalyzerResultSeverity.Warning => new BuildCheckResultWarning(this), - BuildAnalyzerResultSeverity.Error => new BuildCheckResultError(this), + BuildAnalyzerResultSeverity.Warning => new BuildCheckResultWarning(this, BuildAnalyzerRule.Id), + BuildAnalyzerResultSeverity.Error => new BuildCheckResultError(this, BuildAnalyzerRule.Id), _ => throw new ArgumentOutOfRangeException(nameof(severity), severity, null), }; @@ -51,7 +51,7 @@ internal BuildEventArgs ToEventArgs(BuildAnalyzerResultSeverity severity) // Here we will provide different link for built-in rules and custom rules - once we have the base classes differentiated. public string FormatMessage() => - _message ??= $"{(Equals(Location ?? ElementLocation.EmptyLocation, ElementLocation.EmptyLocation) ? string.Empty : (Location!.LocationString + ": "))}{BuildAnalyzerRule.Id}: https://aka.ms/buildcheck/codes#{BuildAnalyzerRule.Id} - {string.Format(BuildAnalyzerRule.MessageFormat, MessageArgs)}"; + _message ??= $"{(Equals(Location ?? ElementLocation.EmptyLocation, ElementLocation.EmptyLocation) ? string.Empty : (Location!.LocationString + ": "))}https://aka.ms/buildcheck/codes#{BuildAnalyzerRule.Id} - {string.Format(BuildAnalyzerRule.MessageFormat, MessageArgs)}"; private string? _message; } diff --git a/src/Framework/BuildCheck/BuildCheckEventArgs.cs b/src/Framework/BuildCheck/BuildCheckEventArgs.cs index 8cc9dfbd691..9a8c3459b87 100644 --- a/src/Framework/BuildCheck/BuildCheckEventArgs.cs +++ b/src/Framework/BuildCheck/BuildCheckEventArgs.cs @@ -107,7 +107,8 @@ internal override void CreateFromStream(BinaryReader reader, int version) } internal sealed class BuildCheckResultWarning : BuildWarningEventArgs { - public BuildCheckResultWarning(IBuildCheckResult result) + public BuildCheckResultWarning(IBuildCheckResult result, string code) + : base(subcategory: null, code: code, file: null, lineNumber: 0, columnNumber: 0, endLineNumber: 0, endColumnNumber: 0, message: result.FormatMessage(), helpKeyword: null, senderName: null) { RawMessage = result.FormatMessage(); } @@ -131,7 +132,8 @@ internal override void CreateFromStream(BinaryReader reader, int version) internal sealed class BuildCheckResultError : BuildErrorEventArgs { - public BuildCheckResultError(IBuildCheckResult result) + public BuildCheckResultError(IBuildCheckResult result, string code) + : base(subcategory: null, code: code, file: null, lineNumber: 0, columnNumber: 0, endLineNumber: 0, endColumnNumber: 0, message: result.FormatMessage(), helpKeyword: null, senderName: null) { RawMessage = result.FormatMessage(); }