From dc481b9d0879e013e77aafd0d68585e1f0233eee Mon Sep 17 00:00:00 2001 From: Jan Krivanek Date: Fri, 1 Nov 2024 16:08:11 +0100 Subject: [PATCH 1/3] No helplink for custom check diags --- src/Build/BuildCheck/API/BuildCheckResult.cs | 9 +++++++-- .../API/{InternalCheck.cs => WorkerNodeCheck.cs} | 2 +- src/Build/BuildCheck/Checks/DoubleWritesCheck.cs | 2 +- .../Checks/NoEnvironmentVariablePropertyCheck.cs | 4 ++-- src/Build/BuildCheck/Checks/PropertiesUsageCheck.cs | 12 ++++++------ src/Build/BuildCheck/Checks/SharedOutputPathCheck.cs | 2 +- 6 files changed, 18 insertions(+), 13 deletions(-) rename src/Build/BuildCheck/API/{InternalCheck.cs => WorkerNodeCheck.cs} (96%) diff --git a/src/Build/BuildCheck/API/BuildCheckResult.cs b/src/Build/BuildCheck/API/BuildCheckResult.cs index 6471d717056..f7c68f61b9d 100644 --- a/src/Build/BuildCheck/API/BuildCheckResult.cs +++ b/src/Build/BuildCheck/API/BuildCheckResult.cs @@ -18,6 +18,9 @@ public sealed class BuildCheckResult : IBuildCheckResult { public static BuildCheckResult Create(CheckRule rule, IMSBuildElementLocation location, params string[] messageArgs) => new BuildCheckResult(rule, location, messageArgs); + internal static BuildCheckResult CreateBuiltIn(CheckRule rule, IMSBuildElementLocation location, + params string[] messageArgs) => new BuildCheckResult(rule, location, messageArgs) { _isBuiltIn = true }; + public BuildCheckResult(CheckRule checkConfig, IMSBuildElementLocation location, string[] messageArgs) { CheckRule = checkConfig; @@ -47,9 +50,11 @@ internal BuildEventArgs ToEventArgs(CheckResultSeverity severity) public string MessageFormat => CheckRule.MessageFormat; - // Here we will provide different link for built-in rules and custom rules - once we have the base classes differentiated. public string FormatMessage() => - _message ??= $"https://aka.ms/buildcheck/codes#{CheckRule.Id} - {string.Format(CheckRule.MessageFormat, MessageArgs)}"; + _message ??= _isBuiltIn + ? $"https://aka.ms/buildcheck/codes#{CheckRule.Id} - {string.Format(CheckRule.MessageFormat, MessageArgs)}" + : string.Format(CheckRule.MessageFormat, MessageArgs); private string? _message; + private bool _isBuiltIn; } diff --git a/src/Build/BuildCheck/API/InternalCheck.cs b/src/Build/BuildCheck/API/WorkerNodeCheck.cs similarity index 96% rename from src/Build/BuildCheck/API/InternalCheck.cs rename to src/Build/BuildCheck/API/WorkerNodeCheck.cs index 242c513e655..c081af2d1e5 100644 --- a/src/Build/BuildCheck/API/InternalCheck.cs +++ b/src/Build/BuildCheck/API/WorkerNodeCheck.cs @@ -6,7 +6,7 @@ namespace Microsoft.Build.Experimental.BuildCheck.Checks; -internal abstract class InternalCheck : Check +internal abstract class WorkerNodeCheck : Check { /// /// Used by the implementors to subscribe to data and events they are interested in. diff --git a/src/Build/BuildCheck/Checks/DoubleWritesCheck.cs b/src/Build/BuildCheck/Checks/DoubleWritesCheck.cs index 25d99b2bb91..8a2c12e4316 100644 --- a/src/Build/BuildCheck/Checks/DoubleWritesCheck.cs +++ b/src/Build/BuildCheck/Checks/DoubleWritesCheck.cs @@ -114,7 +114,7 @@ private void CheckWrite(BuildCheckDataContext context, if (_filesWritten.TryGetValue(fileBeingWritten, out (string projectFilePath, string taskName) existingEntry)) { - context.ReportResult(BuildCheckResult.Create( + context.ReportResult(BuildCheckResult.CreateBuiltIn( SupportedRule, context.Data.TaskInvocationLocation, context.Data.TaskName, diff --git a/src/Build/BuildCheck/Checks/NoEnvironmentVariablePropertyCheck.cs b/src/Build/BuildCheck/Checks/NoEnvironmentVariablePropertyCheck.cs index 30049c3f7ca..6e7c2bc3174 100644 --- a/src/Build/BuildCheck/Checks/NoEnvironmentVariablePropertyCheck.cs +++ b/src/Build/BuildCheck/Checks/NoEnvironmentVariablePropertyCheck.cs @@ -63,7 +63,7 @@ private void ProcessEnvironmentVariableReadAction(BuildCheckDataContext conte { _uninitializedReadsInScope.Remove(writeData.PropertyName); - context.ReportResult(BuildCheckResult.Create( + context.ReportResult(BuildCheckResult.CreateBuiltIn( _initializedAfterUsedRule, uninitInScopeReadLocation, writeData.PropertyName, writeData.ElementLocation?.LocationString ?? string.Empty)); @@ -160,7 +160,7 @@ private void ProcessPropertyWrite(BuildCheckDataContext conte { _uninitializedReadsOutOfScope.Remove(writeData.PropertyName); - context.ReportResult(BuildCheckResult.Create( + context.ReportResult(BuildCheckResult.CreateBuiltIn( _initializedAfterUsedRule, uninitOutScopeReadLocation, writeData.PropertyName, writeData.ElementLocation?.LocationString ?? string.Empty)); @@ -205,7 +205,7 @@ private void ProcessPropertyRead(BuildCheckDataContext context readData.ElementLocation, readData.ProjectFilePath)) { // report immediately - context.ReportResult(BuildCheckResult.Create( + context.ReportResult(BuildCheckResult.CreateBuiltIn( _usedBeforeInitializedRule, readData.ElementLocation, readData.PropertyName)); @@ -220,7 +220,7 @@ private void DoneWithProject(BuildCheckDataContext Date: Mon, 4 Nov 2024 14:46:50 +0100 Subject: [PATCH 2/3] Add help link customization --- src/Build/BuildCheck/API/BuildCheckResult.cs | 5 ++- src/Build/BuildCheck/API/CheckRule.cs | 35 +++++++++++++++++++ src/BuildCheck.UnitTests/EndToEndTests.cs | 8 ++--- .../TestAssets/CustomCheck/Check1.cs | 3 +- .../content/Microsoft.CheckTemplate/Check1.cs | 3 +- 5 files changed, 47 insertions(+), 7 deletions(-) diff --git a/src/Build/BuildCheck/API/BuildCheckResult.cs b/src/Build/BuildCheck/API/BuildCheckResult.cs index f7c68f61b9d..5492a09fb77 100644 --- a/src/Build/BuildCheck/API/BuildCheckResult.cs +++ b/src/Build/BuildCheck/API/BuildCheckResult.cs @@ -52,8 +52,11 @@ internal BuildEventArgs ToEventArgs(CheckResultSeverity severity) public string FormatMessage() => _message ??= _isBuiltIn + // Builtin rules get unified helplink. ? $"https://aka.ms/buildcheck/codes#{CheckRule.Id} - {string.Format(CheckRule.MessageFormat, MessageArgs)}" - : string.Format(CheckRule.MessageFormat, MessageArgs); + // Custom rules can provide their own helplink. + : (!string.IsNullOrEmpty(CheckRule.HelpLinkUri) ? $"{CheckRule.HelpLinkUri} - " : null) + + string.Format(CheckRule.MessageFormat, MessageArgs); private string? _message; private bool _isBuiltIn; diff --git a/src/Build/BuildCheck/API/CheckRule.cs b/src/Build/BuildCheck/API/CheckRule.cs index 3fdf97bbc44..c37615ae9b5 100644 --- a/src/Build/BuildCheck/API/CheckRule.cs +++ b/src/Build/BuildCheck/API/CheckRule.cs @@ -10,6 +10,39 @@ namespace Microsoft.Build.Experimental.BuildCheck; /// public class CheckRule { + /// + /// Creates the descriptor of the BuildCheck rule + /// + /// The id of the rule - used to denote the violation in the outputs + /// The title of the rule - currently unused + /// The detailed description of the rule - currently unused + /// The message format to be used during reporting the violation. + /// The default config of this rule - applicable if user doesn't specify custom values in .editorconfig. + /// Optional link to more detailed help for the violation. + public CheckRule( + string id, + string title, + string description, + string messageFormat, + CheckConfiguration defaultConfiguration, + string helpLinkUri) + { + Id = id; + Title = title; + Description = description; + MessageFormat = messageFormat; + DefaultConfiguration = defaultConfiguration; + HelpLinkUri = helpLinkUri; + } + + /// + /// Creates the descriptor of the BuildCheck rule + /// + /// The id of the rule - used to denote the violation in the outputs + /// The title of the rule - currently unused + /// The detailed description of the rule - currently unused + /// The message format to be used during reporting the violation. + /// The default config of this rule - applicable if user doesn't specify custom values in .editorconfig. public CheckRule( string id, string title, @@ -51,6 +84,8 @@ public CheckRule( /// public string MessageFormat { get; } + public string HelpLinkUri { get; } = string.Empty; + /// /// The default configuration - overridable by the user via .editorconfig. /// If no user specified configuration is provided, this default will be used. diff --git a/src/BuildCheck.UnitTests/EndToEndTests.cs b/src/BuildCheck.UnitTests/EndToEndTests.cs index 58891677b6a..797775c0f32 100644 --- a/src/BuildCheck.UnitTests/EndToEndTests.cs +++ b/src/BuildCheck.UnitTests/EndToEndTests.cs @@ -487,9 +487,9 @@ public void CustomCheckTest_NoEditorConfig(string checkCandidate, string[] expec } } - [Theory(Skip = "https://github.com/dotnet/msbuild/issues/10702")] - [InlineData("CheckCandidate", "X01234", "error", "error X01234")] - [InlineData("CheckCandidateWithMultipleChecksInjected", "X01234", "warning", "warning X01234")] + [Theory] + [InlineData("CheckCandidate", "X01234", "error", "error X01234: http://sampleling.com/X01234")] + [InlineData("CheckCandidateWithMultipleChecksInjected", "X01234", "warning", "warning X01234: http://samplelink.com/X01234")] public void CustomCheckTest_WithEditorConfig(string checkCandidate, string ruleId, string severity, string expectedMessage) { using (var env = TestEnvironment.Create()) @@ -514,7 +514,7 @@ public void CustomCheckTest_WithEditorConfig(string checkCandidate, string ruleI } } - [Theory(Skip = "https://github.com/dotnet/msbuild/issues/10702")] + [Theory] [InlineData("X01236", "Something went wrong initializing")] // These tests are for failure one different points, will be addressed in a different PR // https://github.com/dotnet/msbuild/issues/10522 diff --git a/src/BuildCheck.UnitTests/TestAssets/CustomCheck/Check1.cs b/src/BuildCheck.UnitTests/TestAssets/CustomCheck/Check1.cs index a02c34afc7c..c2673d5b508 100644 --- a/src/BuildCheck.UnitTests/TestAssets/CustomCheck/Check1.cs +++ b/src/BuildCheck.UnitTests/TestAssets/CustomCheck/Check1.cs @@ -14,7 +14,8 @@ public sealed class Check1 : Check "Title", "Description", "Message format: {0}", - new CheckConfiguration()); + new CheckConfiguration(), + "http://samplelink.com/X01234"); public override string FriendlyName => "CustomRule1"; diff --git a/template_feed/content/Microsoft.CheckTemplate/Check1.cs b/template_feed/content/Microsoft.CheckTemplate/Check1.cs index d15d11436be..194e451d533 100644 --- a/template_feed/content/Microsoft.CheckTemplate/Check1.cs +++ b/template_feed/content/Microsoft.CheckTemplate/Check1.cs @@ -11,7 +11,8 @@ public sealed class Check1 : Check "Title", "Description", "Message format: {0}", - new CheckConfiguration()); + new CheckConfiguration(), + "http://sampleHelpLink.com/X01234"); public override string FriendlyName => "Company.Check1"; From 9d7f19acc61eaf9ac8eb6f063c7b00f6e0b89888 Mon Sep 17 00:00:00 2001 From: Jan Krivanek Date: Mon, 4 Nov 2024 18:00:49 +0100 Subject: [PATCH 3/3] Fix typo --- src/BuildCheck.UnitTests/EndToEndTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/BuildCheck.UnitTests/EndToEndTests.cs b/src/BuildCheck.UnitTests/EndToEndTests.cs index 797775c0f32..8eaacb481e8 100644 --- a/src/BuildCheck.UnitTests/EndToEndTests.cs +++ b/src/BuildCheck.UnitTests/EndToEndTests.cs @@ -488,7 +488,7 @@ public void CustomCheckTest_NoEditorConfig(string checkCandidate, string[] expec } [Theory] - [InlineData("CheckCandidate", "X01234", "error", "error X01234: http://sampleling.com/X01234")] + [InlineData("CheckCandidate", "X01234", "error", "error X01234: http://samplelink.com/X01234")] [InlineData("CheckCandidateWithMultipleChecksInjected", "X01234", "warning", "warning X01234: http://samplelink.com/X01234")] public void CustomCheckTest_WithEditorConfig(string checkCandidate, string ruleId, string severity, string expectedMessage) {