diff --git a/src/Build/BackEnd/Components/BuildRequestEngine/BuildRequestEngine.cs b/src/Build/BackEnd/Components/BuildRequestEngine/BuildRequestEngine.cs index 4fc3f6db70a..ed79f9d09d6 100644 --- a/src/Build/BackEnd/Components/BuildRequestEngine/BuildRequestEngine.cs +++ b/src/Build/BackEnd/Components/BuildRequestEngine/BuildRequestEngine.cs @@ -283,8 +283,11 @@ public void CleanupForBuild() throw new AggregateException(deactivateExceptions); } - var buildCheckManager = (_componentHost.GetComponent(BuildComponentType.BuildCheckManagerProvider) as IBuildCheckManagerProvider)!.Instance; + IBuildCheckManagerProvider buildCheckProvider = (_componentHost.GetComponent(BuildComponentType.BuildCheckManagerProvider) as IBuildCheckManagerProvider); + var buildCheckManager = buildCheckProvider!.Instance; buildCheckManager.FinalizeProcessing(_nodeLoggingContext); + // Clears the instance so that next call (on node reuse) to 'GetComponent' leads to reinitialization. + buildCheckProvider.ShutdownComponent(); }, isLastTask: true); diff --git a/src/Build/BackEnd/Components/Logging/LoggingService.cs b/src/Build/BackEnd/Components/Logging/LoggingService.cs index df990251a96..69c729a7cd1 100644 --- a/src/Build/BackEnd/Components/Logging/LoggingService.cs +++ b/src/Build/BackEnd/Components/Logging/LoggingService.cs @@ -854,6 +854,8 @@ public void InitializeComponent(IBuildComponentHost buildComponentHost) _onlyLogCriticalEvents = buildComponentHost.BuildParameters.OnlyLogCriticalEvents; _serviceState = LoggingServiceState.Initialized; + + _buildEngineDataRouter = (buildComponentHost.GetComponent(BuildComponentType.BuildCheckManagerProvider) as IBuildCheckManagerProvider)?.BuildEngineDataRouter; } } diff --git a/src/Build/BackEnd/Components/Logging/LoggingServiceLogMethods.cs b/src/Build/BackEnd/Components/Logging/LoggingServiceLogMethods.cs index 40762761917..eeded58a88a 100644 --- a/src/Build/BackEnd/Components/Logging/LoggingServiceLogMethods.cs +++ b/src/Build/BackEnd/Components/Logging/LoggingServiceLogMethods.cs @@ -798,13 +798,17 @@ public void LogIncludeFile(BuildEventContext buildEventContext, string filePath) #endregion +#nullable enable + private IBuildEngineDataRouter? _buildEngineDataRouter; + public void ProcessPropertyRead(PropertyReadInfo propertyReadInfo, CheckLoggingContext checkContext) - => BuildCheckManagerProvider.GlobalBuildEngineDataRouter?.ProcessPropertyRead(propertyReadInfo, checkContext); + => _buildEngineDataRouter?.ProcessPropertyRead(propertyReadInfo, checkContext); public void ProcessPropertyWrite(PropertyWriteInfo propertyWriteInfo, CheckLoggingContext checkContext) - => BuildCheckManagerProvider.GlobalBuildEngineDataRouter?.ProcessPropertyWrite(propertyWriteInfo, checkContext); + => _buildEngineDataRouter?.ProcessPropertyWrite(propertyWriteInfo, checkContext); public void ProcessProjectEvaluationStarted(ICheckContext checkContext, string projectFullPath) - => BuildCheckManagerProvider.GlobalBuildEngineDataRouter?.ProcessProjectEvaluationStarted(checkContext, projectFullPath); + => _buildEngineDataRouter?.ProcessProjectEvaluationStarted(checkContext, projectFullPath); +#nullable disable } } diff --git a/src/Build/BuildCheck/Infrastructure/BuildCheckManagerProvider.cs b/src/Build/BuildCheck/Infrastructure/BuildCheckManagerProvider.cs index d219233aedb..649dcd4ef29 100644 --- a/src/Build/BuildCheck/Infrastructure/BuildCheckManagerProvider.cs +++ b/src/Build/BuildCheck/Infrastructure/BuildCheckManagerProvider.cs @@ -26,15 +26,11 @@ namespace Microsoft.Build.Experimental.BuildCheck.Infrastructure; /// internal sealed class BuildCheckManagerProvider : IBuildCheckManagerProvider { - private static IBuildCheckManager? s_globalInstance; + private IBuildCheckManager? _instance; - internal static IBuildCheckManager GlobalInstance => s_globalInstance ?? throw new InvalidOperationException("BuildCheckManagerProvider not initialized"); + public IBuildCheckManager Instance => _instance ?? new NullBuildCheckManager(); - public IBuildCheckManager Instance => GlobalInstance; - - public IBuildEngineDataRouter BuildEngineDataRouter => (IBuildEngineDataRouter)GlobalInstance; - - public static IBuildEngineDataRouter? GlobalBuildEngineDataRouter => (IBuildEngineDataRouter?)s_globalInstance; + public IBuildEngineDataRouter BuildEngineDataRouter => (IBuildEngineDataRouter)Instance; internal static IBuildComponent CreateComponent(BuildComponentType type) { @@ -46,25 +42,24 @@ public void InitializeComponent(IBuildComponentHost host) { ErrorUtilities.VerifyThrow(host != null, "BuildComponentHost was null"); - if (s_globalInstance == null) + if (_instance == null) { - IBuildCheckManager instance; if (host!.BuildParameters.IsBuildCheckEnabled) { - instance = new BuildCheckManager(); + _instance = new BuildCheckManager(); } else { - instance = new NullBuildCheckManager(); + _instance = new NullBuildCheckManager(); } - - // We are fine with the possibility of double creation here - as the construction is cheap - // and without side effects and the actual backing field is effectively immutable after the first assignment. - Interlocked.CompareExchange(ref s_globalInstance, instance, null); } } - public void ShutdownComponent() => GlobalInstance.Shutdown(); + public void ShutdownComponent() + { + _instance?.Shutdown(); + _instance = null; + } internal sealed class BuildCheckManager : IBuildCheckManager, IBuildEngineDataRouter { diff --git a/src/BuildCheck.UnitTests/EndToEndTests.cs b/src/BuildCheck.UnitTests/EndToEndTests.cs index 776cd08129e..8a3cafaea0c 100644 --- a/src/BuildCheck.UnitTests/EndToEndTests.cs +++ b/src/BuildCheck.UnitTests/EndToEndTests.cs @@ -45,6 +45,7 @@ public void PropertiesUsageAnalyzerTest(bool buildInOutOfProcessNode) PrepareSampleProjectsAndConfig( buildInOutOfProcessNode, out TransientTestFile projectFile, + out _, "PropsCheckTest.csproj"); string output = RunnerUtilities.ExecBootstrapedMSBuild($"{projectFile.Path} -check", out bool success); @@ -62,6 +63,65 @@ public void PropertiesUsageAnalyzerTest(bool buildInOutOfProcessNode) Regex.Matches(output, "BC0203 .* Property").Count.ShouldBe(2); } + [Fact] + public void ConfigChangeReflectedOnReuse() + { + PrepareSampleProjectsAndConfig( + // we need out of proc build - to test node reuse + true, + out TransientTestFile projectFile, + out TransientTestFile editorconfigFile, + "PropsCheckTest.csproj"); + + // Build without BuildCheck - no findings should be reported + string output = RunnerUtilities.ExecBootstrapedMSBuild($"{projectFile.Path}", out bool success); + _env.Output.WriteLine(output); + _env.Output.WriteLine("========================="); + success.ShouldBeTrue(output); + output.ShouldNotContain("BC0201"); + output.ShouldNotContain("BC0202"); + output.ShouldNotContain("BC0203"); + + // Build with BuildCheck - findings should be reported + output = RunnerUtilities.ExecBootstrapedMSBuild($"{projectFile.Path} -check", out success); + _env.Output.WriteLine(output); + _env.Output.WriteLine("========================="); + success.ShouldBeTrue(output); + output.ShouldContain("warning BC0201"); + output.ShouldContain("warning BC0202"); + output.ShouldContain("warning BC0203"); + + // Flip config in editorconfig + string editorConfigChange = """ + + build_check.BC0201.Severity=error + build_check.BC0202.Severity=error + build_check.BC0203.Severity=error + """; + + File.AppendAllText(editorconfigFile.Path, editorConfigChange); + + // Build with BuildCheck - findings with new severity should be reported + output = RunnerUtilities.ExecBootstrapedMSBuild($"{projectFile.Path} -check", out success); + _env.Output.WriteLine(output); + _env.Output.WriteLine("========================="); + // build should fail due to error checks + success.ShouldBeFalse(output); + output.ShouldContain("error BC0201"); + output.ShouldContain("error BC0202"); + output.ShouldContain("error BC0203"); + + // Build without BuildCheck - no findings should be reported + output = RunnerUtilities.ExecBootstrapedMSBuild($"{projectFile.Path}", out success); + _env.Output.WriteLine(output); + _env.Output.WriteLine("========================="); + success.ShouldBeTrue(output); + output.ShouldNotContain("BC0201"); + output.ShouldNotContain("BC0202"); + output.ShouldNotContain("BC0203"); + } + + [Theory] [InlineData(true, true)] [InlineData(false, true)] @@ -467,6 +527,7 @@ private void PopulateXmlAttribute(XmlDocument doc, XmlNode node, string attribut private void PrepareSampleProjectsAndConfig( bool buildInOutOfProcessNode, out TransientTestFile projectFile, + out TransientTestFile editorconfigFile, string entryProjectAssetName, IEnumerable? supplementalAssetNames = null, IEnumerable<(string RuleId, string Severity)>? ruleToSeverity = null, @@ -485,7 +546,7 @@ private void PrepareSampleProjectsAndConfig( TransientTestFile supplementalFile = _env.CreateFile(workFolder, supplementalAssetName, supplementalContent); } - _env.CreateFile(workFolder, ".editorconfig", ReadEditorConfig(ruleToSeverity, ruleToCustomConfig, testAssetsFolderName)); + editorconfigFile = _env.CreateFile(workFolder, ".editorconfig", ReadEditorConfig(ruleToSeverity, ruleToCustomConfig, testAssetsFolderName)); // OSX links /var into /private, which makes Path.GetTempPath() return "/var..." but Directory.GetCurrentDirectory return "/private/var...". // This discrepancy breaks path equality checks in MSBuild checks if we pass to MSBuild full path to the initial project. @@ -514,6 +575,7 @@ private void PrepareSampleProjectsAndConfig( => PrepareSampleProjectsAndConfig( buildInOutOfProcessNode, out projectFile, + out _, "Project1.csproj", new[] { "Project2.csproj", "ImportedFile1.props" }, ruleToSeverity,