From 9866f9a2ceed634f8563185fe2c08e9158b13e07 Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Thu, 28 Mar 2024 14:57:28 +0100 Subject: [PATCH] Revert "revert changes for "ResultsCache ignores some of the BuildRequest data,.." (#9718)" This reverts commit 553649bd6d8765beaead2f6ccee45c041afc3bb3. --- documentation/wiki/ChangeWaves.md | 1 + .../BackEnd/ResultsCache_Tests.cs | 121 ++++++++++++++++++ .../Components/Caching/ResultsCache.cs | 92 ++++++++----- src/Build/BackEnd/Shared/BuildRequest.cs | 5 + src/Build/BackEnd/Shared/BuildResult.cs | 13 ++ 5 files changed, 201 insertions(+), 31 deletions(-) diff --git a/documentation/wiki/ChangeWaves.md b/documentation/wiki/ChangeWaves.md index 8282f5e8ed2..c3c8f3cc341 100644 --- a/documentation/wiki/ChangeWaves.md +++ b/documentation/wiki/ChangeWaves.md @@ -31,6 +31,7 @@ A wave of features is set to "rotate out" (i.e. become standard functionality) t - [Add Link metadata to Resources in AssignLinkMetadata target](https://github.com/dotnet/msbuild/pull/9464) - [Change Version switch output to finish with a newline](https://github.com/dotnet/msbuild/pull/9485) - [Load NuGet.Frameworks into secondary AppDomain (MSBuild.exe only)](https://github.com/dotnet/msbuild/pull/9446) +- [ResultsCache ignores some of the BuildRequest data, may return incorrect results](https://github.com/dotnet/msbuild/pull/9565) - [Update Traits when environment has been changed](https://github.com/dotnet/msbuild/pull/9655) - [Exec task does not trim leading whitespaces for ConsoleOutput](https://github.com/dotnet/msbuild/pull/9722) - [Introduce [MSBuild]::StableStringHash overloads](https://github.com/dotnet/msbuild/issues/9519) diff --git a/src/Build.UnitTests/BackEnd/ResultsCache_Tests.cs b/src/Build.UnitTests/BackEnd/ResultsCache_Tests.cs index d832aa878b3..39f61d819a9 100644 --- a/src/Build.UnitTests/BackEnd/ResultsCache_Tests.cs +++ b/src/Build.UnitTests/BackEnd/ResultsCache_Tests.cs @@ -188,6 +188,127 @@ public void TestRetrieveSubsetTargetsFromResult() Assert.Equal(BuildResultCode.Success, response.Results.OverallResult); } + [Fact] + public void TestCacheOnDifferentBuildFlagsPerRequest_ProvideProjectStateAfterBuild() + { + string targetName = "testTarget1"; + int submissionId = 1; + int nodeRequestId = 0; + int configurationId = 1; + + BuildRequest requestWithNoBuildDataFlags = new BuildRequest( + submissionId, + nodeRequestId, + configurationId, + new string[1] { targetName } /* escapedTargets */, + null /* hostServices */, + BuildEventContext.Invalid /* parentBuildEventContext */, + null /* parentRequest */, + BuildRequestDataFlags.None); + + BuildRequest requestWithProjectStateFlag = new BuildRequest( + submissionId, + nodeRequestId, + configurationId, + new string[1] { targetName } /* escapedTargets */, + null /* hostServices */, + BuildEventContext.Invalid /* parentBuildEventContext */, + null /* parentRequest */, + BuildRequestDataFlags.ProvideProjectStateAfterBuild); + + BuildRequest requestWithNoBuildDataFlags2 = new BuildRequest( + submissionId, + nodeRequestId, + configurationId, + new string[1] { targetName } /* escapedTargets */, + null /* hostServices */, + BuildEventContext.Invalid /* parentBuildEventContext */, + null /* parentRequest */, + BuildRequestDataFlags.None); + + BuildResult resultForRequestWithNoBuildDataFlags = new(requestWithNoBuildDataFlags); + resultForRequestWithNoBuildDataFlags.AddResultsForTarget(targetName, BuildResultUtilities.GetEmptySucceedingTargetResult()); + ResultsCache cache = new(); + cache.AddResult(resultForRequestWithNoBuildDataFlags); + + ResultsCacheResponse cacheResponseForRequestWithNoBuildDataFlags = cache.SatisfyRequest( + requestWithNoBuildDataFlags, + new List(), + new List(new string[] { targetName }), + skippedResultsDoNotCauseCacheMiss: false); + + ResultsCacheResponse cachedResponseForProjectState = cache.SatisfyRequest( + requestWithProjectStateFlag, + new List(), + new List(new string[] { targetName }), + skippedResultsDoNotCauseCacheMiss: false); + + ResultsCacheResponse cacheResponseForNoBuildDataFlags2 = cache.SatisfyRequest( + requestWithNoBuildDataFlags2, + new List(), + new List(new string[] { targetName }), + skippedResultsDoNotCauseCacheMiss: false); + + Assert.Equal(ResultsCacheResponseType.Satisfied, cacheResponseForRequestWithNoBuildDataFlags.Type); + + // Because ProvideProjectStateAfterBuildFlag was provided as a part of BuildRequest + Assert.Equal(ResultsCacheResponseType.NotSatisfied, cachedResponseForProjectState.Type); + + Assert.Equal(ResultsCacheResponseType.Satisfied, cacheResponseForNoBuildDataFlags2.Type); + } + + [Fact] + public void TestCacheOnDifferentBuildFlagsPerRequest_ProvideSubsetOfStateAfterBuild() + { + string targetName = "testTarget1"; + int submissionId = 1; + int nodeRequestId = 0; + int configurationId = 1; + + BuildRequest requestWithSubsetFlag1 = new BuildRequest( + submissionId, + nodeRequestId, + configurationId, + new string[1] { targetName } /* escapedTargets */, + null /* hostServices */, + BuildEventContext.Invalid /* parentBuildEventContext */, + null /* parentRequest */, + BuildRequestDataFlags.ProvideSubsetOfStateAfterBuild); + + BuildRequest requestWithSubsetFlag2 = new BuildRequest( + submissionId, + nodeRequestId, + configurationId, + new string[1] { targetName } /* escapedTargets */, + null /* hostServices */, + BuildEventContext.Invalid /* parentBuildEventContext */, + null /* parentRequest */, + BuildRequestDataFlags.ProvideSubsetOfStateAfterBuild); + + BuildResult resultForRequestWithSubsetFlag1 = new(requestWithSubsetFlag1); + resultForRequestWithSubsetFlag1.AddResultsForTarget(targetName, BuildResultUtilities.GetEmptySucceedingTargetResult()); + ResultsCache cache = new(); + cache.AddResult(resultForRequestWithSubsetFlag1); + + ResultsCacheResponse cachedResponseWithSubsetFlag1 = cache.SatisfyRequest( + requestWithSubsetFlag1, + new List(), + new List(new string[] { targetName }), + skippedResultsDoNotCauseCacheMiss: false); + + ResultsCacheResponse cachedResponseWithSubsetFlag2 = cache.SatisfyRequest( + requestWithSubsetFlag2, + new List(), + new List(new string[] { targetName }), + skippedResultsDoNotCauseCacheMiss: false); + + // It was agreed not to return cache results if ProvideSubsetOfStateAfterBuild is passed, + // because RequestedProjectState may have different user filters defined. + // It is more reliable to ignore the cached value. + Assert.Equal(ResultsCacheResponseType.NotSatisfied, cachedResponseWithSubsetFlag1.Type); + Assert.Equal(ResultsCacheResponseType.NotSatisfied, cachedResponseWithSubsetFlag2.Type); + } + [Fact] public void TestClearResultsCache() { diff --git a/src/Build/BackEnd/Components/Caching/ResultsCache.cs b/src/Build/BackEnd/Components/Caching/ResultsCache.cs index 0823f86cffe..ec5c888265a 100644 --- a/src/Build/BackEnd/Components/Caching/ResultsCache.cs +++ b/src/Build/BackEnd/Components/Caching/ResultsCache.cs @@ -18,6 +18,15 @@ namespace Microsoft.Build.BackEnd /// internal class ResultsCache : IResultsCache { + /// + /// The presence of any of these flags affects build result for the specified request. + /// + private const BuildRequestDataFlags FlagsAffectingBuildResults = + BuildRequestDataFlags.ProvideProjectStateAfterBuild + | BuildRequestDataFlags.SkipNonexistentTargets + | BuildRequestDataFlags.IgnoreMissingEmptyAndInvalidImports + | BuildRequestDataFlags.FailOnUnresolvedSdk; + /// /// The table of all build results. This table is indexed by configuration id and /// contains BuildResult objects which have all of the target information. @@ -140,10 +149,11 @@ public BuildResult GetResultsForConfiguration(int configurationId) /// /// Attempts to satisfy the request from the cache. The request can be satisfied only if: - /// 1. All specified targets in the request have successful results in the cache or if the sequence of target results + /// 1. The passed BuildRequestDataFlags can not affect the result data. + /// 2. All specified targets in the request have successful results in the cache or if the sequence of target results /// includes 0 or more successful targets followed by at least one failed target. - /// 2. All initial targets in the configuration for the request have non-skipped results in the cache. - /// 3. If there are no specified targets, then all default targets in the request must have non-skipped results + /// 3. All initial targets in the configuration for the request have non-skipped results in the cache. + /// 4. If there are no specified targets, then all default targets in the request must have non-skipped results /// in the cache. /// /// The request whose results we should return. @@ -163,47 +173,53 @@ public ResultsCacheResponse SatisfyRequest(BuildRequest request, List co { if (_resultsByConfiguration.TryGetValue(request.ConfigurationId, out BuildResult allResults)) { - // Check for targets explicitly specified. - bool explicitTargetsSatisfied = CheckResults(allResults, request.Targets, response.ExplicitTargetsToBuild, skippedResultsDoNotCauseCacheMiss); + bool buildDataFlagsSatisfied = ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_10) + ? CheckBuildDataFlagsResults(request.BuildRequestDataFlags, allResults.BuildRequestDataFlags) : true; - if (explicitTargetsSatisfied) + if (buildDataFlagsSatisfied) { - // All of the explicit targets, if any, have been satisfied - response.Type = ResultsCacheResponseType.Satisfied; + // Check for targets explicitly specified. + bool explicitTargetsSatisfied = CheckResults(allResults, request.Targets, response.ExplicitTargetsToBuild, skippedResultsDoNotCauseCacheMiss); - // Check for the initial targets. If we don't know what the initial targets are, we assume they are not satisfied. - if (configInitialTargets == null || !CheckResults(allResults, configInitialTargets, null, skippedResultsDoNotCauseCacheMiss)) + if (explicitTargetsSatisfied) { - response.Type = ResultsCacheResponseType.NotSatisfied; - } + // All of the explicit targets, if any, have been satisfied + response.Type = ResultsCacheResponseType.Satisfied; - // We could still be missing implicit targets, so check those... - if (request.Targets.Count == 0) - { - // Check for the default target, if necessary. If we don't know what the default targets are, we - // assume they are not satisfied. - if (configDefaultTargets == null || !CheckResults(allResults, configDefaultTargets, null, skippedResultsDoNotCauseCacheMiss)) + // Check for the initial targets. If we don't know what the initial targets are, we assume they are not satisfied. + if (configInitialTargets == null || !CheckResults(allResults, configInitialTargets, null, skippedResultsDoNotCauseCacheMiss)) { response.Type = ResultsCacheResponseType.NotSatisfied; } - } - // Now report those results requested, if they are satisfied. - if (response.Type == ResultsCacheResponseType.Satisfied) - { - List targetsToAddResultsFor = new List(configInitialTargets); - - // Now report either the explicit targets or the default targets - if (request.Targets.Count > 0) + // We could still be missing implicit targets, so check those... + if (request.Targets.Count == 0) { - targetsToAddResultsFor.AddRange(request.Targets); + // Check for the default target, if necessary. If we don't know what the default targets are, we + // assume they are not satisfied. + if (configDefaultTargets == null || !CheckResults(allResults, configDefaultTargets, null, skippedResultsDoNotCauseCacheMiss)) + { + response.Type = ResultsCacheResponseType.NotSatisfied; + } } - else + + // Now report those results requested, if they are satisfied. + if (response.Type == ResultsCacheResponseType.Satisfied) { - targetsToAddResultsFor.AddRange(configDefaultTargets); + List targetsToAddResultsFor = new List(configInitialTargets); + + // Now report either the explicit targets or the default targets + if (request.Targets.Count > 0) + { + targetsToAddResultsFor.AddRange(request.Targets); + } + else + { + targetsToAddResultsFor.AddRange(configDefaultTargets); + } + + response.Results = new BuildResult(request, allResults, targetsToAddResultsFor.ToArray(), null); } - - response.Results = new BuildResult(request, allResults, targetsToAddResultsFor.ToArray(), null); } } } @@ -328,6 +344,20 @@ private static bool CheckResults(BuildResult result, List targets, HashS return returnValue; } + /// + /// Checks results for the specified build flags. + /// + /// The current request build flags. + /// The existing build result data flags. + /// False if there is any difference in the data flags that can cause missed build data, true otherwise. + private static bool CheckBuildDataFlagsResults(BuildRequestDataFlags buildRequestDataFlags, BuildRequestDataFlags buildResultDataFlags) => + + // Even if both buildRequestDataFlags and buildResultDataFlags have ProvideSubsetOfStateAfterBuild flag, + // the underlying RequestedProjectState may have different user filters defined. + // It is more reliable to ignore the cached value. + !buildRequestDataFlags.HasFlag(BuildRequestDataFlags.ProvideSubsetOfStateAfterBuild) + && (buildRequestDataFlags & FlagsAffectingBuildResults) == (buildResultDataFlags & FlagsAffectingBuildResults); + public IEnumerator GetEnumerator() { return _resultsByConfiguration.Values.GetEnumerator(); diff --git a/src/Build/BackEnd/Shared/BuildRequest.cs b/src/Build/BackEnd/Shared/BuildRequest.cs index 428eea19656..8951500b8d6 100644 --- a/src/Build/BackEnd/Shared/BuildRequest.cs +++ b/src/Build/BackEnd/Shared/BuildRequest.cs @@ -119,6 +119,11 @@ private BuildRequest( _nodeRequestId = nodeRequestId; _buildRequestDataFlags = buildRequestDataFlags; _requestedProjectState = requestedProjectState; + + if (_requestedProjectState != null) + { + _buildRequestDataFlags |= BuildRequestDataFlags.ProvideSubsetOfStateAfterBuild; + } } /// diff --git a/src/Build/BackEnd/Shared/BuildResult.cs b/src/Build/BackEnd/Shared/BuildResult.cs index 68aa197381f..cee4212033e 100644 --- a/src/Build/BackEnd/Shared/BuildResult.cs +++ b/src/Build/BackEnd/Shared/BuildResult.cs @@ -116,6 +116,11 @@ public class BuildResult : INodePacket, IBuildResults /// private ProjectInstance _projectStateAfterBuild; + /// + /// The flags provide additional control over the build results and may affect the cached value. + /// + private BuildRequestDataFlags _buildRequestDataFlags; + private string _schedulerInducedError; private HashSet _projectTargets; @@ -204,6 +209,7 @@ internal BuildResult(BuildRequest request, BuildResult existingResults, string[] _nodeRequestId = request.NodeRequestId; _circularDependency = false; _baseOverallResult = true; + _buildRequestDataFlags = request.BuildRequestDataFlags; if (existingResults == null) { @@ -380,6 +386,12 @@ public ProjectInstance ProjectStateAfterBuild set => _projectStateAfterBuild = value; } + /// + /// Gets the flags that were used in the build request to which these results are associated. + /// See for examples of the available flags. + /// + public BuildRequestDataFlags BuildRequestDataFlags => _buildRequestDataFlags; + /// /// Returns the node packet type. /// @@ -581,6 +593,7 @@ void ITranslatable.Translate(ITranslator translator) translator.Translate(ref _savedCurrentDirectory); translator.Translate(ref _schedulerInducedError); translator.TranslateDictionary(ref _savedEnvironmentVariables, StringComparer.OrdinalIgnoreCase); + translator.TranslateEnum(ref _buildRequestDataFlags, (int)_buildRequestDataFlags); } ///