Skip to content

Commit 1185ec7

Browse files
ladiproladipro
committed
Revert "revert changes for "ResultsCache ignores some of the BuildRequest data,.." (dotnet#9718)"
This reverts commit 553649b.
1 parent 053feb0 commit 1185ec7

File tree

5 files changed

+201
-31
lines changed

5 files changed

+201
-31
lines changed

‎documentation/wiki/ChangeWaves.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ A wave of features is set to "rotate out" (i.e. become standard functionality) t
3535
- [Add Link metadata to Resources in AssignLinkMetadata target](https://github.com/dotnet/msbuild/pull/9464)
3636
- [Change Version switch output to finish with a newline](https://github.com/dotnet/msbuild/pull/9485)
3737
- [Load NuGet.Frameworks into secondary AppDomain (MSBuild.exe only)](https://github.com/dotnet/msbuild/pull/9446)
38+
- [ResultsCache ignores some of the BuildRequest data, may return incorrect results](https://github.com/dotnet/msbuild/pull/9565)
3839
- [Update Traits when environment has been changed](https://github.com/dotnet/msbuild/pull/9655)
3940
- [Exec task does not trim leading whitespaces for ConsoleOutput](https://github.com/dotnet/msbuild/pull/9722)
4041
- [Introduce [MSBuild]::StableStringHash overloads](https://github.com/dotnet/msbuild/issues/9519)

‎src/Build.UnitTests/BackEnd/ResultsCache_Tests.cs‎

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,127 @@ public void TestRetrieveSubsetTargetsFromResult()
188188
Assert.Equal(BuildResultCode.Success, response.Results.OverallResult);
189189
}
190190

191+
[Fact]
192+
public void TestCacheOnDifferentBuildFlagsPerRequest_ProvideProjectStateAfterBuild()
193+
{
194+
string targetName = "testTarget1";
195+
int submissionId = 1;
196+
int nodeRequestId = 0;
197+
int configurationId = 1;
198+
199+
BuildRequest requestWithNoBuildDataFlags = new BuildRequest(
200+
submissionId,
201+
nodeRequestId,
202+
configurationId,
203+
new string[1] { targetName } /* escapedTargets */,
204+
null /* hostServices */,
205+
BuildEventContext.Invalid /* parentBuildEventContext */,
206+
null /* parentRequest */,
207+
BuildRequestDataFlags.None);
208+
209+
BuildRequest requestWithProjectStateFlag = new BuildRequest(
210+
submissionId,
211+
nodeRequestId,
212+
configurationId,
213+
new string[1] { targetName } /* escapedTargets */,
214+
null /* hostServices */,
215+
BuildEventContext.Invalid /* parentBuildEventContext */,
216+
null /* parentRequest */,
217+
BuildRequestDataFlags.ProvideProjectStateAfterBuild);
218+
219+
BuildRequest requestWithNoBuildDataFlags2 = new BuildRequest(
220+
submissionId,
221+
nodeRequestId,
222+
configurationId,
223+
new string[1] { targetName } /* escapedTargets */,
224+
null /* hostServices */,
225+
BuildEventContext.Invalid /* parentBuildEventContext */,
226+
null /* parentRequest */,
227+
BuildRequestDataFlags.None);
228+
229+
BuildResult resultForRequestWithNoBuildDataFlags = new(requestWithNoBuildDataFlags);
230+
resultForRequestWithNoBuildDataFlags.AddResultsForTarget(targetName, BuildResultUtilities.GetEmptySucceedingTargetResult());
231+
ResultsCache cache = new();
232+
cache.AddResult(resultForRequestWithNoBuildDataFlags);
233+
234+
ResultsCacheResponse cacheResponseForRequestWithNoBuildDataFlags = cache.SatisfyRequest(
235+
requestWithNoBuildDataFlags,
236+
new List<string>(),
237+
new List<string>(new string[] { targetName }),
238+
skippedResultsDoNotCauseCacheMiss: false);
239+
240+
ResultsCacheResponse cachedResponseForProjectState = cache.SatisfyRequest(
241+
requestWithProjectStateFlag,
242+
new List<string>(),
243+
new List<string>(new string[] { targetName }),
244+
skippedResultsDoNotCauseCacheMiss: false);
245+
246+
ResultsCacheResponse cacheResponseForNoBuildDataFlags2 = cache.SatisfyRequest(
247+
requestWithNoBuildDataFlags2,
248+
new List<string>(),
249+
new List<string>(new string[] { targetName }),
250+
skippedResultsDoNotCauseCacheMiss: false);
251+
252+
Assert.Equal(ResultsCacheResponseType.Satisfied, cacheResponseForRequestWithNoBuildDataFlags.Type);
253+
254+
// Because ProvideProjectStateAfterBuildFlag was provided as a part of BuildRequest
255+
Assert.Equal(ResultsCacheResponseType.NotSatisfied, cachedResponseForProjectState.Type);
256+
257+
Assert.Equal(ResultsCacheResponseType.Satisfied, cacheResponseForNoBuildDataFlags2.Type);
258+
}
259+
260+
[Fact]
261+
public void TestCacheOnDifferentBuildFlagsPerRequest_ProvideSubsetOfStateAfterBuild()
262+
{
263+
string targetName = "testTarget1";
264+
int submissionId = 1;
265+
int nodeRequestId = 0;
266+
int configurationId = 1;
267+
268+
BuildRequest requestWithSubsetFlag1 = new BuildRequest(
269+
submissionId,
270+
nodeRequestId,
271+
configurationId,
272+
new string[1] { targetName } /* escapedTargets */,
273+
null /* hostServices */,
274+
BuildEventContext.Invalid /* parentBuildEventContext */,
275+
null /* parentRequest */,
276+
BuildRequestDataFlags.ProvideSubsetOfStateAfterBuild);
277+
278+
BuildRequest requestWithSubsetFlag2 = new BuildRequest(
279+
submissionId,
280+
nodeRequestId,
281+
configurationId,
282+
new string[1] { targetName } /* escapedTargets */,
283+
null /* hostServices */,
284+
BuildEventContext.Invalid /* parentBuildEventContext */,
285+
null /* parentRequest */,
286+
BuildRequestDataFlags.ProvideSubsetOfStateAfterBuild);
287+
288+
BuildResult resultForRequestWithSubsetFlag1 = new(requestWithSubsetFlag1);
289+
resultForRequestWithSubsetFlag1.AddResultsForTarget(targetName, BuildResultUtilities.GetEmptySucceedingTargetResult());
290+
ResultsCache cache = new();
291+
cache.AddResult(resultForRequestWithSubsetFlag1);
292+
293+
ResultsCacheResponse cachedResponseWithSubsetFlag1 = cache.SatisfyRequest(
294+
requestWithSubsetFlag1,
295+
new List<string>(),
296+
new List<string>(new string[] { targetName }),
297+
skippedResultsDoNotCauseCacheMiss: false);
298+
299+
ResultsCacheResponse cachedResponseWithSubsetFlag2 = cache.SatisfyRequest(
300+
requestWithSubsetFlag2,
301+
new List<string>(),
302+
new List<string>(new string[] { targetName }),
303+
skippedResultsDoNotCauseCacheMiss: false);
304+
305+
// It was agreed not to return cache results if ProvideSubsetOfStateAfterBuild is passed,
306+
// because RequestedProjectState may have different user filters defined.
307+
// It is more reliable to ignore the cached value.
308+
Assert.Equal(ResultsCacheResponseType.NotSatisfied, cachedResponseWithSubsetFlag1.Type);
309+
Assert.Equal(ResultsCacheResponseType.NotSatisfied, cachedResponseWithSubsetFlag2.Type);
310+
}
311+
191312
[Fact]
192313
public void TestClearResultsCache()
193314
{

‎src/Build/BackEnd/Components/Caching/ResultsCache.cs‎

Lines changed: 61 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,15 @@ namespace Microsoft.Build.BackEnd
1818
/// </summary>
1919
internal class ResultsCache : IResultsCache
2020
{
21+
/// <summary>
22+
/// The presence of any of these flags affects build result for the specified request.
23+
/// </summary>
24+
private const BuildRequestDataFlags FlagsAffectingBuildResults =
25+
BuildRequestDataFlags.ProvideProjectStateAfterBuild
26+
| BuildRequestDataFlags.SkipNonexistentTargets
27+
| BuildRequestDataFlags.IgnoreMissingEmptyAndInvalidImports
28+
| BuildRequestDataFlags.FailOnUnresolvedSdk;
29+
2130
/// <summary>
2231
/// The table of all build results. This table is indexed by configuration id and
2332
/// contains BuildResult objects which have all of the target information.
@@ -140,10 +149,11 @@ public BuildResult GetResultsForConfiguration(int configurationId)
140149

141150
/// <summary>
142151
/// Attempts to satisfy the request from the cache. The request can be satisfied only if:
143-
/// 1. All specified targets in the request have successful results in the cache or if the sequence of target results
152+
/// 1. The passed BuildRequestDataFlags can not affect the result data.
153+
/// 2. All specified targets in the request have successful results in the cache or if the sequence of target results
144154
/// includes 0 or more successful targets followed by at least one failed target.
145-
/// 2. All initial targets in the configuration for the request have non-skipped results in the cache.
146-
/// 3. If there are no specified targets, then all default targets in the request must have non-skipped results
155+
/// 3. All initial targets in the configuration for the request have non-skipped results in the cache.
156+
/// 4. If there are no specified targets, then all default targets in the request must have non-skipped results
147157
/// in the cache.
148158
/// </summary>
149159
/// <param name="request">The request whose results we should return.</param>
@@ -163,47 +173,53 @@ public ResultsCacheResponse SatisfyRequest(BuildRequest request, List<string> co
163173
{
164174
if (_resultsByConfiguration.TryGetValue(request.ConfigurationId, out BuildResult allResults))
165175
{
166-
// Check for targets explicitly specified.
167-
bool explicitTargetsSatisfied = CheckResults(allResults, request.Targets, response.ExplicitTargetsToBuild, skippedResultsDoNotCauseCacheMiss);
176+
bool buildDataFlagsSatisfied = ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_10)
177+
? CheckBuildDataFlagsResults(request.BuildRequestDataFlags, allResults.BuildRequestDataFlags) : true;
168178

169-
if (explicitTargetsSatisfied)
179+
if (buildDataFlagsSatisfied)
170180
{
171-
// All of the explicit targets, if any, have been satisfied
172-
response.Type = ResultsCacheResponseType.Satisfied;
181+
// Check for targets explicitly specified.
182+
bool explicitTargetsSatisfied = CheckResults(allResults, request.Targets, response.ExplicitTargetsToBuild, skippedResultsDoNotCauseCacheMiss);
173183

174-
// Check for the initial targets. If we don't know what the initial targets are, we assume they are not satisfied.
175-
if (configInitialTargets == null || !CheckResults(allResults, configInitialTargets, null, skippedResultsDoNotCauseCacheMiss))
184+
if (explicitTargetsSatisfied)
176185
{
177-
response.Type = ResultsCacheResponseType.NotSatisfied;
178-
}
186+
// All of the explicit targets, if any, have been satisfied
187+
response.Type = ResultsCacheResponseType.Satisfied;
179188

180-
// We could still be missing implicit targets, so check those...
181-
if (request.Targets.Count == 0)
182-
{
183-
// Check for the default target, if necessary. If we don't know what the default targets are, we
184-
// assume they are not satisfied.
185-
if (configDefaultTargets == null || !CheckResults(allResults, configDefaultTargets, null, skippedResultsDoNotCauseCacheMiss))
189+
// Check for the initial targets. If we don't know what the initial targets are, we assume they are not satisfied.
190+
if (configInitialTargets == null || !CheckResults(allResults, configInitialTargets, null, skippedResultsDoNotCauseCacheMiss))
186191
{
187192
response.Type = ResultsCacheResponseType.NotSatisfied;
188193
}
189-
}
190194

191-
// Now report those results requested, if they are satisfied.
192-
if (response.Type == ResultsCacheResponseType.Satisfied)
193-
{
194-
List<string> targetsToAddResultsFor = new List<string>(configInitialTargets);
195-
196-
// Now report either the explicit targets or the default targets
197-
if (request.Targets.Count > 0)
195+
// We could still be missing implicit targets, so check those...
196+
if (request.Targets.Count == 0)
198197
{
199-
targetsToAddResultsFor.AddRange(request.Targets);
198+
// Check for the default target, if necessary. If we don't know what the default targets are, we
199+
// assume they are not satisfied.
200+
if (configDefaultTargets == null || !CheckResults(allResults, configDefaultTargets, null, skippedResultsDoNotCauseCacheMiss))
201+
{
202+
response.Type = ResultsCacheResponseType.NotSatisfied;
203+
}
200204
}
201-
else
205+
206+
// Now report those results requested, if they are satisfied.
207+
if (response.Type == ResultsCacheResponseType.Satisfied)
202208
{
203-
targetsToAddResultsFor.AddRange(configDefaultTargets);
209+
List<string> targetsToAddResultsFor = new List<string>(configInitialTargets);
210+
211+
// Now report either the explicit targets or the default targets
212+
if (request.Targets.Count > 0)
213+
{
214+
targetsToAddResultsFor.AddRange(request.Targets);
215+
}
216+
else
217+
{
218+
targetsToAddResultsFor.AddRange(configDefaultTargets);
219+
}
220+
221+
response.Results = new BuildResult(request, allResults, targetsToAddResultsFor.ToArray(), null);
204222
}
205-
206-
response.Results = new BuildResult(request, allResults, targetsToAddResultsFor.ToArray(), null);
207223
}
208224
}
209225
}
@@ -328,6 +344,20 @@ private static bool CheckResults(BuildResult result, List<string> targets, HashS
328344
return returnValue;
329345
}
330346

347+
/// <summary>
348+
/// Checks results for the specified build flags.
349+
/// </summary>
350+
/// <param name="buildRequestDataFlags">The current request build flags.</param>
351+
/// <param name="buildResultDataFlags">The existing build result data flags.</param>
352+
/// <returns>False if there is any difference in the data flags that can cause missed build data, true otherwise.</returns>
353+
private static bool CheckBuildDataFlagsResults(BuildRequestDataFlags buildRequestDataFlags, BuildRequestDataFlags buildResultDataFlags) =>
354+
355+
// Even if both buildRequestDataFlags and buildResultDataFlags have ProvideSubsetOfStateAfterBuild flag,
356+
// the underlying RequestedProjectState may have different user filters defined.
357+
// It is more reliable to ignore the cached value.
358+
!buildRequestDataFlags.HasFlag(BuildRequestDataFlags.ProvideSubsetOfStateAfterBuild)
359+
&& (buildRequestDataFlags & FlagsAffectingBuildResults) == (buildResultDataFlags & FlagsAffectingBuildResults);
360+
331361
public IEnumerator<BuildResult> GetEnumerator()
332362
{
333363
return _resultsByConfiguration.Values.GetEnumerator();

‎src/Build/BackEnd/Shared/BuildRequest.cs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,11 @@ private BuildRequest(
119119
_nodeRequestId = nodeRequestId;
120120
_buildRequestDataFlags = buildRequestDataFlags;
121121
_requestedProjectState = requestedProjectState;
122+
123+
if (_requestedProjectState != null)
124+
{
125+
_buildRequestDataFlags |= BuildRequestDataFlags.ProvideSubsetOfStateAfterBuild;
126+
}
122127
}
123128

124129
/// <summary>

‎src/Build/BackEnd/Shared/BuildResult.cs‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,11 @@ public class BuildResult : INodePacket, IBuildResults
116116
/// </summary>
117117
private ProjectInstance _projectStateAfterBuild;
118118

119+
/// <summary>
120+
/// The flags provide additional control over the build results and may affect the cached value.
121+
/// </summary>
122+
private BuildRequestDataFlags _buildRequestDataFlags;
123+
119124
private string _schedulerInducedError;
120125

121126
private HashSet<string> _projectTargets;
@@ -204,6 +209,7 @@ internal BuildResult(BuildRequest request, BuildResult existingResults, string[]
204209
_nodeRequestId = request.NodeRequestId;
205210
_circularDependency = false;
206211
_baseOverallResult = true;
212+
_buildRequestDataFlags = request.BuildRequestDataFlags;
207213

208214
if (existingResults == null)
209215
{
@@ -380,6 +386,12 @@ public ProjectInstance ProjectStateAfterBuild
380386
set => _projectStateAfterBuild = value;
381387
}
382388

389+
/// <summary>
390+
/// Gets the flags that were used in the build request to which these results are associated.
391+
/// See <see cref="Execution.BuildRequestDataFlags"/> for examples of the available flags.
392+
/// </summary>
393+
public BuildRequestDataFlags BuildRequestDataFlags => _buildRequestDataFlags;
394+
383395
/// <summary>
384396
/// Returns the node packet type.
385397
/// </summary>
@@ -581,6 +593,7 @@ void ITranslatable.Translate(ITranslator translator)
581593
translator.Translate(ref _savedCurrentDirectory);
582594
translator.Translate(ref _schedulerInducedError);
583595
translator.TranslateDictionary(ref _savedEnvironmentVariables, StringComparer.OrdinalIgnoreCase);
596+
translator.TranslateEnum(ref _buildRequestDataFlags, (int)_buildRequestDataFlags);
584597
}
585598

586599
/// <summary>

0 commit comments

Comments
 (0)