From b242e3e196c69a6542e7007dc7f947498f80c04b Mon Sep 17 00:00:00 2001 From: Viktor Hofer Date: Thu, 16 Jan 2025 22:17:40 +0100 Subject: [PATCH 1/4] Correctly respect NoWarn CLI/Task options in Compatibility logging interface --- .../SuppressibleMSBuildLog.cs | 3 ++- .../ValidateAssembliesTask.cs | 2 +- .../ValidatePackageTask.cs | 2 +- .../Program.cs | 4 ++-- .../SuppressibleConsoleLog.cs | 3 ++- .../Logging/ConsoleLog.cs | 20 ++++++++++++++++--- .../Logging/MSBuildLog.cs | 7 +++++-- 7 files changed, 30 insertions(+), 11 deletions(-) diff --git a/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompat.Task/SuppressibleMSBuildLog.cs b/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompat.Task/SuppressibleMSBuildLog.cs index 0dda7cfe46cc..57e2ff4748cf 100644 --- a/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompat.Task/SuppressibleMSBuildLog.cs +++ b/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompat.Task/SuppressibleMSBuildLog.cs @@ -10,7 +10,8 @@ namespace Microsoft.DotNet.ApiCompat.Task /// Class that can log Suppressions in an MSBuild task, by implementing MSBuildLog and ISuppressibleLog. /// internal sealed class SuppressibleMSBuildLog(NET.Build.Tasks.Logger log, - ISuppressionEngine suppressionEngine) : MSBuildLog(log), ISuppressibleLog + ISuppressionEngine suppressionEngine, + string? noWarn = null) : MSBuildLog(log, noWarn), ISuppressibleLog { /// public bool HasLoggedErrorSuppressions { get; private set; } diff --git a/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompat.Task/ValidateAssembliesTask.cs b/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompat.Task/ValidateAssembliesTask.cs index 650245dae575..424d81b4862a 100644 --- a/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompat.Task/ValidateAssembliesTask.cs +++ b/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompat.Task/ValidateAssembliesTask.cs @@ -132,7 +132,7 @@ public override bool Execute() protected override void ExecuteCore() { - SuppressibleMSBuildLog logFactory(ISuppressionEngine suppressionEngine) => new(Log, suppressionEngine); + SuppressibleMSBuildLog logFactory(ISuppressionEngine suppressionEngine) => new(Log, suppressionEngine, NoWarn); ValidateAssemblies.Run(logFactory, GenerateSuppressionFile, PreserveUnnecessarySuppressions, diff --git a/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompat.Task/ValidatePackageTask.cs b/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompat.Task/ValidatePackageTask.cs index b839ca1d4325..196d90c47272 100644 --- a/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompat.Task/ValidatePackageTask.cs +++ b/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompat.Task/ValidatePackageTask.cs @@ -140,7 +140,7 @@ public override bool Execute() protected override void ExecuteCore() { - SuppressibleMSBuildLog logFactory(ISuppressionEngine suppressionEngine) => new(Log, suppressionEngine); + SuppressibleMSBuildLog logFactory(ISuppressionEngine suppressionEngine) => new(Log, suppressionEngine, NoWarn); ValidatePackage.Run(logFactory, GenerateSuppressionFile, PreserveUnnecessarySuppressions, diff --git a/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompat.Tool/Program.cs b/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompat.Tool/Program.cs index cb248930e153..1cf403f061f1 100644 --- a/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompat.Tool/Program.cs +++ b/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompat.Tool/Program.cs @@ -196,7 +196,7 @@ static int Main(string[] args) (string, string)[]? leftAssembliesTransformationPattern = parseResult.GetValue(leftAssembliesTransformationPatternOption); (string, string)[]? rightAssembliesTransformationPattern = parseResult.GetValue(rightAssembliesTransformationPatternOption); - SuppressibleConsoleLog logFactory(ISuppressionEngine suppressionEngine) => new(suppressionEngine, verbosity); + SuppressibleConsoleLog logFactory(ISuppressionEngine suppressionEngine) => new(suppressionEngine, verbosity, noWarn); int exitCode = ValidateAssemblies.Run(logFactory, generateSuppressionFile, preserveUnnecessarySuppressions, @@ -320,7 +320,7 @@ static int Main(string[] args) Dictionary>? baselinePackageAssemblyReferences = parseResult.GetValue(baselinePackageAssemblyReferencesOption); string[]? baselinePackageFrameworksToIgnore = parseResult.GetValue(baselinePackageFrameworksToIgnoreOption); - SuppressibleConsoleLog logFactory(ISuppressionEngine suppressionEngine) => new(suppressionEngine, verbosity); + SuppressibleConsoleLog logFactory(ISuppressionEngine suppressionEngine) => new(suppressionEngine, verbosity, noWarn); int exitCode = ValidatePackage.Run(logFactory, generateSuppressionFile, preserveUnnecessarySuppressions, diff --git a/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompat.Tool/SuppressibleConsoleLog.cs b/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompat.Tool/SuppressibleConsoleLog.cs index 4bda2be1f799..5d2222e45a58 100644 --- a/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompat.Tool/SuppressibleConsoleLog.cs +++ b/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompat.Tool/SuppressibleConsoleLog.cs @@ -10,7 +10,8 @@ namespace Microsoft.DotNet.ApiCompat.Tool /// Class that can log Suppressions to the Console, by implementing ConsoleLog and ISuppressibleLog. /// internal sealed class SuppressibleConsoleLog(ISuppressionEngine suppressionEngine, - MessageImportance messageImportance) : ConsoleLog(messageImportance), ISuppressibleLog + MessageImportance messageImportance, + string? noWarn = null) : ConsoleLog(messageImportance, noWarn), ISuppressibleLog { /// public bool HasLoggedErrorSuppressions { get; private set; } diff --git a/src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Logging/ConsoleLog.cs b/src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Logging/ConsoleLog.cs index fd380e6c1214..b0e6c5a771ef 100644 --- a/src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Logging/ConsoleLog.cs +++ b/src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Logging/ConsoleLog.cs @@ -6,8 +6,10 @@ namespace Microsoft.DotNet.ApiSymbolExtensions.Logging /// /// Class to define common logging abstraction to the console across the APICompat and GenAPI codebases. /// - public class ConsoleLog(MessageImportance messageImportance) : ILog + public class ConsoleLog(MessageImportance messageImportance, string? noWarn = null) : ILog { + private readonly HashSet _noWarn = string.IsNullOrEmpty(noWarn) ? [] : new(noWarn!.Split(';')); + /// public bool HasLoggedErrors { get; private set; } @@ -30,8 +32,20 @@ public virtual void LogWarning(string message) => Console.WriteLine(message); /// - public virtual void LogWarning(string code, string message) => - Console.WriteLine($"{code}: {message}"); + public virtual void LogWarning(string code, string message) + { + string messageTextWithCode = $"{code}: {message}"; + + // Mimic MSBuild which logs suppressed warnings as low importance messages. + if (_noWarn.Contains(code)) + { + LogMessage(MessageImportance.Low, messageTextWithCode) + } + else + { + Console.WriteLine(messageTextWithCode); + } + } /// public virtual void LogMessage(string message) => diff --git a/src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Logging/MSBuildLog.cs b/src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Logging/MSBuildLog.cs index 2d85e521ecad..e7fb061b2454 100644 --- a/src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Logging/MSBuildLog.cs +++ b/src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Logging/MSBuildLog.cs @@ -8,8 +8,11 @@ namespace Microsoft.DotNet.ApiSymbolExtensions.Logging /// /// Class to define common logging abstraction for MSBuild tasks across the APICompat and GenAPI codebases. /// - internal class MSBuildLog(Logger log) : ILog + internal class MSBuildLog(Logger log, string? noWarn = null) : ILog { + // Remove passing in NoWarn when MSBuild respects it correctly in outer-builds: https://github.com/dotnet/msbuild/issues/10873 + private readonly HashSet _noWarn = string.IsNullOrEmpty(noWarn) ? [] : new(noWarn!.Split(';')); + /// public bool HasLoggedErrors => log.HasLoggedErrors; @@ -27,7 +30,7 @@ public virtual void LogWarning(string message) => /// public virtual void LogWarning(string code, string message) => - LogCore(MessageLevel.Warning, code, message); + LogCore(_noWarn.Contains(code) ? MessageLevel.LowImportance : MessageLevel.Warning, code, message); /// public virtual void LogMessage(string message) => From 212b97e0f218e6e2ba57f30c561242973f926f97 Mon Sep 17 00:00:00 2001 From: Viktor Hofer Date: Thu, 16 Jan 2025 22:19:20 +0100 Subject: [PATCH 2/4] Add comment for suppressed warnings logging --- .../Microsoft.DotNet.ApiSymbolExtensions/Logging/MSBuildLog.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Logging/MSBuildLog.cs b/src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Logging/MSBuildLog.cs index e7fb061b2454..edbb3ad53459 100644 --- a/src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Logging/MSBuildLog.cs +++ b/src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Logging/MSBuildLog.cs @@ -30,6 +30,7 @@ public virtual void LogWarning(string message) => /// public virtual void LogWarning(string code, string message) => + // Mimic MSBuild which logs suppressed warnings as low importance messages. LogCore(_noWarn.Contains(code) ? MessageLevel.LowImportance : MessageLevel.Warning, code, message); /// From 6501f41e9a1290ba638bc9e45a19d235cb8fc572 Mon Sep 17 00:00:00 2001 From: Viktor Hofer Date: Thu, 16 Jan 2025 22:29:15 +0100 Subject: [PATCH 3/4] PR feedback to use OrdinalIgnoreCase --- .../Logging/SuppressionEngine.cs | 2 +- .../Microsoft.DotNet.ApiSymbolExtensions/Logging/ConsoleLog.cs | 2 +- .../Microsoft.DotNet.ApiSymbolExtensions/Logging/MSBuildLog.cs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompatibility/Logging/SuppressionEngine.cs b/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompatibility/Logging/SuppressionEngine.cs index fef506d21bbf..8fbfbc69c1ae 100644 --- a/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompatibility/Logging/SuppressionEngine.cs +++ b/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompatibility/Logging/SuppressionEngine.cs @@ -17,7 +17,7 @@ public class SuppressionEngine(string? noWarn = null, bool baselineAllErrors = f protected const string DiagnosticIdDocumentationComment = " https://learn.microsoft.com/dotnet/fundamentals/package-validation/diagnostic-ids "; private readonly HashSet _baselineSuppressions = []; private readonly HashSet _suppressions = []; - private readonly HashSet _noWarn = string.IsNullOrEmpty(noWarn) ? [] : new(noWarn!.Split(';')); + private readonly HashSet _noWarn = string.IsNullOrEmpty(noWarn) ? [] : new(noWarn!.Split(';'), StringComparer.OrdinalIgnoreCase); /// public bool BaselineAllErrors { get; } = baselineAllErrors; diff --git a/src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Logging/ConsoleLog.cs b/src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Logging/ConsoleLog.cs index b0e6c5a771ef..813e20035871 100644 --- a/src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Logging/ConsoleLog.cs +++ b/src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Logging/ConsoleLog.cs @@ -8,7 +8,7 @@ namespace Microsoft.DotNet.ApiSymbolExtensions.Logging /// public class ConsoleLog(MessageImportance messageImportance, string? noWarn = null) : ILog { - private readonly HashSet _noWarn = string.IsNullOrEmpty(noWarn) ? [] : new(noWarn!.Split(';')); + private readonly HashSet _noWarn = string.IsNullOrEmpty(noWarn) ? [] : new(noWarn!.Split(';'), StringComparer.OrdinalIgnoreCase); /// public bool HasLoggedErrors { get; private set; } diff --git a/src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Logging/MSBuildLog.cs b/src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Logging/MSBuildLog.cs index edbb3ad53459..cd2eb8e5bc05 100644 --- a/src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Logging/MSBuildLog.cs +++ b/src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Logging/MSBuildLog.cs @@ -11,7 +11,7 @@ namespace Microsoft.DotNet.ApiSymbolExtensions.Logging internal class MSBuildLog(Logger log, string? noWarn = null) : ILog { // Remove passing in NoWarn when MSBuild respects it correctly in outer-builds: https://github.com/dotnet/msbuild/issues/10873 - private readonly HashSet _noWarn = string.IsNullOrEmpty(noWarn) ? [] : new(noWarn!.Split(';')); + private readonly HashSet _noWarn = string.IsNullOrEmpty(noWarn) ? [] : new(noWarn!.Split(';'), StringComparer.OrdinalIgnoreCase); /// public bool HasLoggedErrors => log.HasLoggedErrors; From 39d4369e9ca99bd46a79853da73d9733385e3196 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20S=C3=A1nchez=20L=C3=B3pez?= <1175054+carlossanlop@users.noreply.github.com> Date: Thu, 16 Jan 2025 16:18:52 -0800 Subject: [PATCH 4/4] Fix syntax error --- .../Microsoft.DotNet.ApiSymbolExtensions/Logging/ConsoleLog.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Logging/ConsoleLog.cs b/src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Logging/ConsoleLog.cs index 813e20035871..41754fadcfa2 100644 --- a/src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Logging/ConsoleLog.cs +++ b/src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Logging/ConsoleLog.cs @@ -39,7 +39,7 @@ public virtual void LogWarning(string code, string message) // Mimic MSBuild which logs suppressed warnings as low importance messages. if (_noWarn.Contains(code)) { - LogMessage(MessageImportance.Low, messageTextWithCode) + LogMessage(MessageImportance.Low, messageTextWithCode); } else {