Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert changes for "ResultsCache ignores some of the BuildRequest dat… #9718

Merged
merged 1 commit into from
Feb 8, 2024
Merged

Revert changes for "ResultsCache ignores some of the BuildRequest dat… #9718

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion documentation/wiki/ChangeWaves.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ A wave of features is set to "rotate out" (i.e. become standard functionality) t
- [Change Version switch output to finish with a newline](https://github.com/dotnet/msbuild/pull/9485)
- [Load Microsoft.DotNet.MSBuildSdkResolver into default load context (MSBuild.exe only)](https://github.com/dotnet/msbuild/pull/9439)
- [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)


Expand Down
121 changes: 0 additions & 121 deletions src/Build.UnitTests/BackEnd/ResultsCache_Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,127 +188,6 @@ 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<string>(),
new List<string>(new string[] { targetName }),
skippedResultsDoNotCauseCacheMiss: false);

ResultsCacheResponse cachedResponseForProjectState = cache.SatisfyRequest(
requestWithProjectStateFlag,
new List<string>(),
new List<string>(new string[] { targetName }),
skippedResultsDoNotCauseCacheMiss: false);

ResultsCacheResponse cacheResponseForNoBuildDataFlags2 = cache.SatisfyRequest(
requestWithNoBuildDataFlags2,
new List<string>(),
new List<string>(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<string>(),
new List<string>(new string[] { targetName }),
skippedResultsDoNotCauseCacheMiss: false);

ResultsCacheResponse cachedResponseWithSubsetFlag2 = cache.SatisfyRequest(
requestWithSubsetFlag2,
new List<string>(),
new List<string>(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()
{
Expand Down
92 changes: 31 additions & 61 deletions src/Build/BackEnd/Components/Caching/ResultsCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,6 @@ namespace Microsoft.Build.BackEnd
/// </summary>
internal class ResultsCache : IResultsCache
{
/// <summary>
/// The presence of any of these flags affects build result for the specified request.
/// </summary>
private const BuildRequestDataFlags FlagsAffectingBuildResults =
BuildRequestDataFlags.ProvideProjectStateAfterBuild
| BuildRequestDataFlags.SkipNonexistentTargets
| BuildRequestDataFlags.IgnoreMissingEmptyAndInvalidImports
| BuildRequestDataFlags.FailOnUnresolvedSdk;

/// <summary>
/// The table of all build results. This table is indexed by configuration id and
/// contains BuildResult objects which have all of the target information.
Expand Down Expand Up @@ -149,11 +140,10 @@ public BuildResult GetResultsForConfiguration(int configurationId)

/// <summary>
/// Attempts to satisfy the request from the cache. The request can be satisfied only if:
/// 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
/// 1. 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.
/// 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
/// 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
/// in the cache.
/// </summary>
/// <param name="request">The request whose results we should return.</param>
Expand All @@ -173,53 +163,47 @@ public ResultsCacheResponse SatisfyRequest(BuildRequest request, List<string> co
{
if (_resultsByConfiguration.TryGetValue(request.ConfigurationId, out BuildResult allResults))
{
bool buildDataFlagsSatisfied = ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_10)
? CheckBuildDataFlagsResults(request.BuildRequestDataFlags, allResults.BuildRequestDataFlags) : true;
// Check for targets explicitly specified.
bool explicitTargetsSatisfied = CheckResults(allResults, request.Targets, response.ExplicitTargetsToBuild, skippedResultsDoNotCauseCacheMiss);

if (buildDataFlagsSatisfied)
if (explicitTargetsSatisfied)
{
// Check for targets explicitly specified.
bool explicitTargetsSatisfied = CheckResults(allResults, request.Targets, response.ExplicitTargetsToBuild, skippedResultsDoNotCauseCacheMiss);
// All of the explicit targets, if any, have been satisfied
response.Type = ResultsCacheResponseType.Satisfied;

if (explicitTargetsSatisfied)
// 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))
{
// All of the explicit targets, if any, have been satisfied
response.Type = ResultsCacheResponseType.Satisfied;
response.Type = ResultsCacheResponseType.NotSatisfied;
}

// 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))
// 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))
{
response.Type = ResultsCacheResponseType.NotSatisfied;
}
}

// We could still be missing implicit targets, so check those...
if (request.Targets.Count == 0)
// Now report those results requested, if they are satisfied.
if (response.Type == ResultsCacheResponseType.Satisfied)
{
List<string> targetsToAddResultsFor = new List<string>(configInitialTargets);

// Now report either the explicit targets or the default targets
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))
{
response.Type = ResultsCacheResponseType.NotSatisfied;
}
targetsToAddResultsFor.AddRange(request.Targets);
}

// Now report those results requested, if they are satisfied.
if (response.Type == ResultsCacheResponseType.Satisfied)
else
{
List<string> targetsToAddResultsFor = new List<string>(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);
targetsToAddResultsFor.AddRange(configDefaultTargets);
}

response.Results = new BuildResult(request, allResults, targetsToAddResultsFor.ToArray(), null);
}
}
}
Expand Down Expand Up @@ -344,20 +328,6 @@ private static bool CheckResults(BuildResult result, List<string> targets, HashS
return returnValue;
}

/// <summary>
/// Checks results for the specified build flags.
/// </summary>
/// <param name="buildRequestDataFlags">The current request build flags.</param>
/// <param name="buildResultDataFlags">The existing build result data flags.</param>
/// <returns>False if there is any difference in the data flags that can cause missed build data, true otherwise.</returns>
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<BuildResult> GetEnumerator()
{
return _resultsByConfiguration.Values.GetEnumerator();
Expand Down
5 changes: 0 additions & 5 deletions src/Build/BackEnd/Shared/BuildRequest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,11 +119,6 @@ private BuildRequest(
_nodeRequestId = nodeRequestId;
_buildRequestDataFlags = buildRequestDataFlags;
_requestedProjectState = requestedProjectState;

if (_requestedProjectState != null)
{
_buildRequestDataFlags |= BuildRequestDataFlags.ProvideSubsetOfStateAfterBuild;
}
}

/// <summary>
Expand Down
13 changes: 0 additions & 13 deletions src/Build/BackEnd/Shared/BuildResult.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,11 +116,6 @@ public class BuildResult : INodePacket, IBuildResults
/// </summary>
private ProjectInstance _projectStateAfterBuild;

/// <summary>
/// The flags provide additional control over the build results and may affect the cached value.
/// </summary>
private BuildRequestDataFlags _buildRequestDataFlags;

private string _schedulerInducedError;

private HashSet<string> _projectTargets;
Expand Down Expand Up @@ -209,7 +204,6 @@ internal BuildResult(BuildRequest request, BuildResult existingResults, string[]
_nodeRequestId = request.NodeRequestId;
_circularDependency = false;
_baseOverallResult = true;
_buildRequestDataFlags = request.BuildRequestDataFlags;
YuliiaKovalova marked this conversation as resolved.
Show resolved Hide resolved

if (existingResults == null)
{
Expand Down Expand Up @@ -386,12 +380,6 @@ public ProjectInstance ProjectStateAfterBuild
set => _projectStateAfterBuild = value;
}

/// <summary>
/// Gets the flags that were used in the build request to which these results are associated.
/// See <see cref="Execution.BuildRequestDataFlags"/> for examples of the available flags.
/// </summary>
public BuildRequestDataFlags BuildRequestDataFlags => _buildRequestDataFlags;

/// <summary>
/// Returns the node packet type.
/// </summary>
Expand Down Expand Up @@ -593,7 +581,6 @@ 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);
}

/// <summary>
Expand Down