From c8c8201fc4505665c33833bb5b3f3cd50070efb4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mariana=20Dematt=C3=A9?= Date: Wed, 7 Aug 2024 17:07:09 +0200 Subject: [PATCH 1/7] Skip restore on RequestBuilder --- .../Components/RequestBuilder/RequestBuilder.cs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs b/src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs index d792e0c2c05..cf2f7ad1cfd 100644 --- a/src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs +++ b/src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Concurrent; using System.Collections.Generic; +using System.Diagnostics; using System.Globalization; using System.IO; using System.Linq; @@ -1105,9 +1106,13 @@ private async Task BuildProject() ErrorUtilities.VerifyThrow(_targetBuilder != null, "Target builder is null"); // We consider this the entrypoint for the project build for purposes of BuildCheck processing + var propertyEntry = _requestEntry.RequestConfiguration.GlobalProperties[MSBuildConstants.MSBuildIsRestoring]; - var buildCheckManager = (_componentHost.GetComponent(BuildComponentType.BuildCheckManagerProvider) as IBuildCheckManagerProvider)!.Instance; - buildCheckManager.SetDataSource(BuildCheckDataSource.BuildExecution); + var buildCheckManager = propertyEntry is null + ? (_componentHost.GetComponent(BuildComponentType.BuildCheckManagerProvider) as IBuildCheckManagerProvider)!.Instance + : null; + + buildCheckManager?.SetDataSource(BuildCheckDataSource.BuildExecution); // Make sure it is null before loading the configuration into the request, because if there is a problem // we do not wand to have an invalid projectLoggingContext floating around. Also if this is null the error will be @@ -1121,7 +1126,7 @@ private async Task BuildProject() // Load the project if (!_requestEntry.RequestConfiguration.IsLoaded) { - buildCheckManager.StartProjectEvaluation( + buildCheckManager?.StartProjectEvaluation( BuildCheckDataSource.BuildExecution, new AnalysisLoggingContext(_nodeLoggingContext.LoggingService, _requestEntry.Request.BuildEventContext), _requestEntry.RequestConfiguration.ProjectFullPath); @@ -1146,13 +1151,13 @@ private async Task BuildProject() } finally { - buildCheckManager.EndProjectEvaluation( + buildCheckManager?.EndProjectEvaluation( BuildCheckDataSource.BuildExecution, _requestEntry.Request.BuildEventContext); } _projectLoggingContext = _nodeLoggingContext.LogProjectStarted(_requestEntry); - buildCheckManager.StartProjectRequest( + buildCheckManager?.StartProjectRequest( BuildCheckDataSource.BuildExecution, _requestEntry.Request.BuildEventContext, _requestEntry.RequestConfiguration.ProjectFullPath); @@ -1224,7 +1229,7 @@ private async Task BuildProject() } finally { - buildCheckManager.EndProjectRequest( + buildCheckManager?.EndProjectRequest( BuildCheckDataSource.BuildExecution, new AnalysisLoggingContext(_nodeLoggingContext.LoggingService, _requestEntry.Request.BuildEventContext), _requestEntry.RequestConfiguration.ProjectFullPath); From 17775e6a5e227a9cc63ca23d382a924300547083 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mariana=20Dematt=C3=A9?= Date: Wed, 7 Aug 2024 17:15:36 +0200 Subject: [PATCH 2/7] Base test --- src/BuildCheck.UnitTests/EndToEndTests.cs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/BuildCheck.UnitTests/EndToEndTests.cs b/src/BuildCheck.UnitTests/EndToEndTests.cs index 740151b95b3..fc0d52fd92e 100644 --- a/src/BuildCheck.UnitTests/EndToEndTests.cs +++ b/src/BuildCheck.UnitTests/EndToEndTests.cs @@ -274,6 +274,21 @@ public void CustomAnalyzerTest_WithEditorConfig(string analysisCandidate, string } } + [Theory] + [InlineData(true)] + [InlineData(false)] + public void DoesNotRunOnRestore(bool buildInOutOfProcessNode) + { + PrepareSampleProjectsAndConfig(buildInOutOfProcessNode, out TransientTestFile projectFile, new List<(string, string)>() { ("BC0101", "warning") }); + + string output = RunnerUtilities.ExecBootstrapedMSBuild( + $"{Path.GetFileName(projectFile.Path)} /m:2 -nr:False -t:restore -check", + out bool success, timeoutMilliseconds: 120_000000); + + success.ShouldBeTrue(); + output.ShouldNotContain("BC0101"); + } + private void AddCustomDataSourceToNugetConfig(string analysisCandidatePath) { var nugetTemplatePath = Path.Combine(analysisCandidatePath, "nugetTemplate.config"); From 68ce1cba84da9aca3a04105e44e6398fe4341aa0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mariana=20Dematt=C3=A9?= Date: Wed, 7 Aug 2024 21:35:58 +0200 Subject: [PATCH 3/7] Logger handling of new event --- .../BuildCheckBuildEventHandler.cs | 31 +++++++++++++++++++ .../BuildCheckForwardingLogger.cs | 1 + 2 files changed, 32 insertions(+) diff --git a/src/Build/BuildCheck/Infrastructure/BuildCheckBuildEventHandler.cs b/src/Build/BuildCheck/Infrastructure/BuildCheckBuildEventHandler.cs index 2b6b94aa2d5..a9de11df4cc 100644 --- a/src/Build/BuildCheck/Infrastructure/BuildCheckBuildEventHandler.cs +++ b/src/Build/BuildCheck/Infrastructure/BuildCheckBuildEventHandler.cs @@ -10,6 +10,7 @@ using Microsoft.Build.Experimental.BuildCheck.Acquisition; using Microsoft.Build.Experimental.BuildCheck.Utilities; using Microsoft.Build.Framework; +using Microsoft.Build.Shared; namespace Microsoft.Build.Experimental.BuildCheck.Infrastructure; @@ -20,6 +21,8 @@ internal class BuildCheckBuildEventHandler private readonly Dictionary> _eventHandlers; + private IDictionary _projectRestore; + internal BuildCheckBuildEventHandler( IAnalysisContextFactory analyzerContextFactory, IBuildCheckManager buildCheckManager) @@ -27,8 +30,10 @@ internal BuildCheckBuildEventHandler( _buildCheckManager = buildCheckManager; _analyzerContextFactory = analyzerContextFactory; + _projectRestore = new Dictionary(); _eventHandlers = new() { + { typeof(BuildSubmissionStartedEventArgs), (BuildEventArgs e) => HandleBuildSubmissionStartedEvent((BuildSubmissionStartedEventArgs)e) }, { typeof(ProjectEvaluationFinishedEventArgs), (BuildEventArgs e) => HandleProjectEvaluationFinishedEvent((ProjectEvaluationFinishedEventArgs)e) }, { typeof(ProjectEvaluationStartedEventArgs), (BuildEventArgs e) => HandleProjectEvaluationStartedEvent((ProjectEvaluationStartedEventArgs)e) }, { typeof(EnvironmentVariableReadEventArgs), (BuildEventArgs e) => HandleEnvironmentVariableReadEvent((EnvironmentVariableReadEventArgs)e) }, @@ -45,12 +50,38 @@ internal BuildCheckBuildEventHandler( public void HandleBuildEvent(BuildEventArgs e) { + if ( + e.GetType() != typeof(BuildSubmissionStartedEventArgs) && + e.BuildEventContext is not null && + _projectRestore.TryGetValue(e.BuildEventContext.SubmissionId, out bool isRestoring) && + isRestoring) + { + return; + } + if (_eventHandlers.TryGetValue(e.GetType(), out Action? handler)) { handler(e); } } + private void HandleBuildSubmissionStartedEvent(BuildSubmissionStartedEventArgs eventArgs) + { + if (_projectRestore.TryGetValue(eventArgs.SubmissionId, out bool isRestoring)) + { + if (isRestoring) + { + _projectRestore[eventArgs.SubmissionId] = false; + } + } + else + { + eventArgs.GlobalProperties.TryGetValue(MSBuildConstants.MSBuildIsRestoring, out string? restoreProperty); + bool isRestore = restoreProperty is not null ? Convert.ToBoolean(restoreProperty) : false; + _projectRestore.Add(eventArgs.SubmissionId, isRestore); + } + } + private void HandleProjectEvaluationFinishedEvent(ProjectEvaluationFinishedEventArgs eventArgs) { if (!IsMetaProjFile(eventArgs.ProjectFile)) diff --git a/src/Build/BuildCheck/Infrastructure/BuildCheckForwardingLogger.cs b/src/Build/BuildCheck/Infrastructure/BuildCheckForwardingLogger.cs index 02808f434a5..ba4d9445232 100644 --- a/src/Build/BuildCheck/Infrastructure/BuildCheckForwardingLogger.cs +++ b/src/Build/BuildCheck/Infrastructure/BuildCheckForwardingLogger.cs @@ -38,6 +38,7 @@ internal class BuildCheckForwardingLogger : IForwardingLogger private HashSet _eventsToForward = new HashSet { typeof(EnvironmentVariableReadEventArgs), + typeof(BuildSubmissionStartedEventArgs), typeof(ProjectEvaluationFinishedEventArgs), typeof(ProjectEvaluationStartedEventArgs), typeof(ProjectStartedEventArgs), From 7034359b658c751f45532c0cfca21633541d8c8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mariana=20Dematt=C3=A9?= Date: Thu, 8 Aug 2024 16:29:14 +0200 Subject: [PATCH 4/7] PR comments and fix test --- .../RequestBuilder/RequestBuilder.cs | 5 ++--- .../BuildCheckBuildEventHandler.cs | 19 +++++++------------ src/BuildCheck.UnitTests/EndToEndTests.cs | 6 ++++-- 3 files changed, 13 insertions(+), 17 deletions(-) diff --git a/src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs b/src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs index cf2f7ad1cfd..28605114c61 100644 --- a/src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs +++ b/src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs @@ -4,7 +4,6 @@ using System; using System.Collections.Concurrent; using System.Collections.Generic; -using System.Diagnostics; using System.Globalization; using System.IO; using System.Linq; @@ -1106,9 +1105,9 @@ private async Task BuildProject() ErrorUtilities.VerifyThrow(_targetBuilder != null, "Target builder is null"); // We consider this the entrypoint for the project build for purposes of BuildCheck processing - var propertyEntry = _requestEntry.RequestConfiguration.GlobalProperties[MSBuildConstants.MSBuildIsRestoring]; + bool isRestoring = _requestEntry.RequestConfiguration.GlobalProperties[MSBuildConstants.MSBuildIsRestoring] is null; - var buildCheckManager = propertyEntry is null + var buildCheckManager = isRestoring ? (_componentHost.GetComponent(BuildComponentType.BuildCheckManagerProvider) as IBuildCheckManagerProvider)!.Instance : null; diff --git a/src/Build/BuildCheck/Infrastructure/BuildCheckBuildEventHandler.cs b/src/Build/BuildCheck/Infrastructure/BuildCheckBuildEventHandler.cs index a9de11df4cc..12597da0a86 100644 --- a/src/Build/BuildCheck/Infrastructure/BuildCheckBuildEventHandler.cs +++ b/src/Build/BuildCheck/Infrastructure/BuildCheckBuildEventHandler.cs @@ -21,7 +21,7 @@ internal class BuildCheckBuildEventHandler private readonly Dictionary> _eventHandlers; - private IDictionary _projectRestore; + private bool isRestoring = false; internal BuildCheckBuildEventHandler( IAnalysisContextFactory analyzerContextFactory, @@ -30,7 +30,6 @@ internal BuildCheckBuildEventHandler( _buildCheckManager = buildCheckManager; _analyzerContextFactory = analyzerContextFactory; - _projectRestore = new Dictionary(); _eventHandlers = new() { { typeof(BuildSubmissionStartedEventArgs), (BuildEventArgs e) => HandleBuildSubmissionStartedEvent((BuildSubmissionStartedEventArgs)e) }, @@ -50,11 +49,11 @@ internal BuildCheckBuildEventHandler( public void HandleBuildEvent(BuildEventArgs e) { + // Skip event handling during restore phase if ( + isRestoring && e.GetType() != typeof(BuildSubmissionStartedEventArgs) && - e.BuildEventContext is not null && - _projectRestore.TryGetValue(e.BuildEventContext.SubmissionId, out bool isRestoring) && - isRestoring) + e.BuildEventContext is not null) { return; } @@ -67,18 +66,14 @@ e.BuildEventContext is not null && private void HandleBuildSubmissionStartedEvent(BuildSubmissionStartedEventArgs eventArgs) { - if (_projectRestore.TryGetValue(eventArgs.SubmissionId, out bool isRestoring)) + if (isRestoring) { - if (isRestoring) - { - _projectRestore[eventArgs.SubmissionId] = false; - } + isRestoring = false; } else { eventArgs.GlobalProperties.TryGetValue(MSBuildConstants.MSBuildIsRestoring, out string? restoreProperty); - bool isRestore = restoreProperty is not null ? Convert.ToBoolean(restoreProperty) : false; - _projectRestore.Add(eventArgs.SubmissionId, isRestore); + isRestoring = restoreProperty is not null ? Convert.ToBoolean(restoreProperty) : false; } } diff --git a/src/BuildCheck.UnitTests/EndToEndTests.cs b/src/BuildCheck.UnitTests/EndToEndTests.cs index fc0d52fd92e..86c1bd05d28 100644 --- a/src/BuildCheck.UnitTests/EndToEndTests.cs +++ b/src/BuildCheck.UnitTests/EndToEndTests.cs @@ -282,11 +282,13 @@ public void DoesNotRunOnRestore(bool buildInOutOfProcessNode) PrepareSampleProjectsAndConfig(buildInOutOfProcessNode, out TransientTestFile projectFile, new List<(string, string)>() { ("BC0101", "warning") }); string output = RunnerUtilities.ExecBootstrapedMSBuild( - $"{Path.GetFileName(projectFile.Path)} /m:2 -nr:False -t:restore -check", - out bool success, timeoutMilliseconds: 120_000000); + $"{Path.GetFileName(projectFile.Path)} /m: -nr:False -t:restore -analyze", + out bool success); success.ShouldBeTrue(); output.ShouldNotContain("BC0101"); + output.ShouldNotContain("BC0102"); + output.ShouldNotContain("BC0103"); } private void AddCustomDataSourceToNugetConfig(string analysisCandidatePath) From ba7c18d2d1176119bb82fffc4579a647926e1394 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mariana=20Dematt=C3=A9?= Date: Thu, 8 Aug 2024 16:56:25 +0200 Subject: [PATCH 5/7] fixed test v2 --- 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 86c1bd05d28..e0f9b17be22 100644 --- a/src/BuildCheck.UnitTests/EndToEndTests.cs +++ b/src/BuildCheck.UnitTests/EndToEndTests.cs @@ -282,7 +282,7 @@ public void DoesNotRunOnRestore(bool buildInOutOfProcessNode) PrepareSampleProjectsAndConfig(buildInOutOfProcessNode, out TransientTestFile projectFile, new List<(string, string)>() { ("BC0101", "warning") }); string output = RunnerUtilities.ExecBootstrapedMSBuild( - $"{Path.GetFileName(projectFile.Path)} /m: -nr:False -t:restore -analyze", + $"{Path.GetFileName(projectFile.Path)} /m:1 -nr:False -t:restore -analyze", out bool success); success.ShouldBeTrue(); From 1bb626eb64a6f7f6606272dc1fb884a0473a1498 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mariana=20Dematt=C3=A9?= Date: Thu, 8 Aug 2024 17:08:59 +0200 Subject: [PATCH 6/7] Address comments --- .../BuildCheckBuildEventHandler.cs | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/src/Build/BuildCheck/Infrastructure/BuildCheckBuildEventHandler.cs b/src/Build/BuildCheck/Infrastructure/BuildCheckBuildEventHandler.cs index 12597da0a86..68528380552 100644 --- a/src/Build/BuildCheck/Infrastructure/BuildCheckBuildEventHandler.cs +++ b/src/Build/BuildCheck/Infrastructure/BuildCheckBuildEventHandler.cs @@ -21,7 +21,7 @@ internal class BuildCheckBuildEventHandler private readonly Dictionary> _eventHandlers; - private bool isRestoring = false; + private bool _isRestoring = false; internal BuildCheckBuildEventHandler( IAnalysisContextFactory analyzerContextFactory, @@ -51,9 +51,8 @@ public void HandleBuildEvent(BuildEventArgs e) { // Skip event handling during restore phase if ( - isRestoring && - e.GetType() != typeof(BuildSubmissionStartedEventArgs) && - e.BuildEventContext is not null) + _isRestoring && + e.GetType() != typeof(BuildSubmissionStartedEventArgs)) { return; } @@ -66,15 +65,8 @@ public void HandleBuildEvent(BuildEventArgs e) private void HandleBuildSubmissionStartedEvent(BuildSubmissionStartedEventArgs eventArgs) { - if (isRestoring) - { - isRestoring = false; - } - else - { - eventArgs.GlobalProperties.TryGetValue(MSBuildConstants.MSBuildIsRestoring, out string? restoreProperty); - isRestoring = restoreProperty is not null ? Convert.ToBoolean(restoreProperty) : false; - } + eventArgs.GlobalProperties.TryGetValue(MSBuildConstants.MSBuildIsRestoring, out string? restoreProperty); + _isRestoring = restoreProperty is not null ? Convert.ToBoolean(restoreProperty) : false; } private void HandleProjectEvaluationFinishedEvent(ProjectEvaluationFinishedEventArgs eventArgs) From bb3b4c32275c338f95ccd286574606de8a74fdf5 Mon Sep 17 00:00:00 2001 From: Jan Krivanek Date: Fri, 9 Aug 2024 09:04:53 +0200 Subject: [PATCH 7/7] Fix and refactor --- .../BuildCheckBuildEventHandler.cs | 28 ++++++++++--------- src/BuildCheck.UnitTests/EndToEndTests.cs | 2 +- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/src/Build/BuildCheck/Infrastructure/BuildCheckBuildEventHandler.cs b/src/Build/BuildCheck/Infrastructure/BuildCheckBuildEventHandler.cs index 7a0aaea6e4c..8e6bd5c24cc 100644 --- a/src/Build/BuildCheck/Infrastructure/BuildCheckBuildEventHandler.cs +++ b/src/Build/BuildCheck/Infrastructure/BuildCheckBuildEventHandler.cs @@ -19,9 +19,9 @@ internal class BuildCheckBuildEventHandler private readonly IBuildCheckManager _buildCheckManager; private readonly ICheckContextFactory _checkContextFactory; - private readonly Dictionary> _eventHandlers; - - private bool _isRestoring = false; + private Dictionary> _eventHandlers; + private readonly Dictionary> _eventHandlersFull; + private readonly Dictionary> _eventHandlersRestore; internal BuildCheckBuildEventHandler( ICheckContextFactory checkContextFactory, @@ -30,7 +30,7 @@ internal BuildCheckBuildEventHandler( _buildCheckManager = buildCheckManager; _checkContextFactory = checkContextFactory; - _eventHandlers = new() + _eventHandlersFull = new() { { typeof(BuildSubmissionStartedEventArgs), (BuildEventArgs e) => HandleBuildSubmissionStartedEvent((BuildSubmissionStartedEventArgs)e) }, { typeof(ProjectEvaluationFinishedEventArgs), (BuildEventArgs e) => HandleProjectEvaluationFinishedEvent((ProjectEvaluationFinishedEventArgs)e) }, @@ -45,18 +45,18 @@ internal BuildCheckBuildEventHandler( { typeof(TaskParameterEventArgs), (BuildEventArgs e) => HandleTaskParameterEvent((TaskParameterEventArgs)e) }, { typeof(BuildFinishedEventArgs), (BuildEventArgs e) => HandleBuildFinishedEvent((BuildFinishedEventArgs)e) }, }; + + // During restore we'll wait only for restore to be done. + _eventHandlersRestore = new() + { + { typeof(BuildSubmissionStartedEventArgs), (BuildEventArgs e) => HandleBuildSubmissionStartedEvent((BuildSubmissionStartedEventArgs)e) }, + }; + + _eventHandlers = _eventHandlersFull; } public void HandleBuildEvent(BuildEventArgs e) { - // Skip event handling during restore phase - if ( - _isRestoring && - e.GetType() != typeof(BuildSubmissionStartedEventArgs)) - { - return; - } - if (_eventHandlers.TryGetValue(e.GetType(), out Action? handler)) { handler(e); @@ -66,7 +66,9 @@ public void HandleBuildEvent(BuildEventArgs e) private void HandleBuildSubmissionStartedEvent(BuildSubmissionStartedEventArgs eventArgs) { eventArgs.GlobalProperties.TryGetValue(MSBuildConstants.MSBuildIsRestoring, out string? restoreProperty); - _isRestoring = restoreProperty is not null ? Convert.ToBoolean(restoreProperty) : false; + bool isRestoring = restoreProperty is not null && Convert.ToBoolean(restoreProperty); + + _eventHandlers = isRestoring ? _eventHandlersRestore : _eventHandlersFull; } private void HandleProjectEvaluationFinishedEvent(ProjectEvaluationFinishedEventArgs eventArgs) diff --git a/src/BuildCheck.UnitTests/EndToEndTests.cs b/src/BuildCheck.UnitTests/EndToEndTests.cs index 2cdec8945be..69f56d71449 100644 --- a/src/BuildCheck.UnitTests/EndToEndTests.cs +++ b/src/BuildCheck.UnitTests/EndToEndTests.cs @@ -323,7 +323,7 @@ public void DoesNotRunOnRestore(bool buildInOutOfProcessNode) PrepareSampleProjectsAndConfig(buildInOutOfProcessNode, out TransientTestFile projectFile, new List<(string, string)>() { ("BC0101", "warning") }); string output = RunnerUtilities.ExecBootstrapedMSBuild( - $"{Path.GetFileName(projectFile.Path)} /m:1 -nr:False -t:restore -analyze", + $"{Path.GetFileName(projectFile.Path)} /m:1 -nr:False -t:restore -check", out bool success); success.ShouldBeTrue();