[Static graph] Support skipping nonexisting targets #5297
Conversation
cdmihai
force-pushed
the
supportSkippingNonexistingTargetsInStaticGraph
branch
from
1290f6b to
56a377d
Compare
BartoszKlonowski
left a comment
BartoszKlonowski
left a comment
There was a problem hiding this comment.
A lot of hard work in this PR.
The only disadvantage of making a PR this size is the difficulty to review it in details. Perhaps this is the reason for this PR to be still unmerged?
I've made some minor comments about the code but there's still the functionality to be checked.
Did you compare unit tests results before and after these changes?
| if (configurationsCleared != null) | ||
| if (configurationsCleared != null) | ||
| { | ||
| foreach (int configurationId in configurationsCleared) |
There was a problem hiding this comment.
If we are trying to reduce the indentations, then perhaps using ForEach() would be better:
configurationsCleared.ForEach(configurationId => _resultsCache.ClearResultsForConfiguration(configurationId));
| glob = $"$([MSBuild]::Escape('{glob}'))"; | ||
|
|
||
| projectContents = projectContents ?? $@" | ||
| projectContents ??= $@" |
There was a problem hiding this comment.
If it doesn't affect the functionality or it's unrelated to the topic of a PR, then perhaps changing this one specific line should be avoided (reduced amount of files changed in PR)?
If not, then why this specific line?
| CreateProjectGraph( | ||
| env: _env, | ||
| dependencyEdges: new Dictionary<int, int[]> { { 1, new[] { 2, 3 } } }, | ||
| dependencyEdges: new Dictionary<int, int[]> {{1, new[] {2, 3}}}, |
There was a problem hiding this comment.
With such a big commit, or even bigger PR it's a good practice to separate formatting/cosmetical changes from the functional changes.
| return true; | ||
| } | ||
|
|
||
| return _override.HasConfiguration(configId) || CurrentCache.HasConfiguration(configId); |
There was a problem hiding this comment.
Because of small refactoring done in line 139 this would also use some polishing:
_override.HasConfiguration(configId) has already been checked in line 134.
| /// <param name="configId">The configuration id.</param> | ||
| /// <param name="existingConfig">Corresponding configuration if configId is present. Null otherwise</param> | ||
| /// <returns>True if the cache contains the configuration. False otherwise. </returns> | ||
| bool TryGetConfiguration(int configId, out BuildRequestConfiguration existingConfig); |
There was a problem hiding this comment.
Try in a method's name suggests throwing an exception.
Perhaps:
GetConfigWithGivenIdIfExists would describe both actions this method does?
| // In isolated builds, cache misses from the former are accepted for requests that skip non-existing targets. | ||
| // Cache misses from the latter break the isolation constraint of no cache misses, even when non-existing targets get skipped. | ||
| bool any = false; | ||
| foreach (var target in request.Targets) |
There was a problem hiding this comment.
Perhaps request.Targets.Any() would be easier to read?
|
Closing until this problem becomes more pressing. |
…#8330) Fixes #4252 Context This PR extracts the relevant logic from the closed #5297, which adds support for the SkipNonexistentTargets metadatum on the ProjectReferenceTargets item: <ProjectReferenceTargets Include='<some-target>' Targets='<some-target1>;<some-target2>' SkipNonexistentTargets='<boolean>'> If SkipNonexistentTargets is true, then any targets in Targets are skipped if they're nonexistent. SkipNonexistentTargets cannot be added to ProjectReferenceTargets items whose Targets contain .default or .projectReferenceTargetsOrDefaultTargets, which represent the default targets and targets specified on the ProjectReference item (with fallback to default targets if none are specified), respectively. Changes Made GetTargetLists filters out skippable nonexistent targets on referenced projects. Determining whether a target is skippable required storing the defined project targets in BuildResults to ensure that corresponding BuildRequestConfigurations on the build manager node have set project targets if the build manager node created a configuration based on a request from an external node but hadn't received a result (since the project may not have been loaded locally and thus the project targets would be unknown). Added SkipNonexistentTargets='true' to GetTargetFrameworks since in the non-graph case it is added to the relevant MSBuild task. Testing UTs and manual testing with /graph and /graph /isolate (the latter run without restore being called due to #6856) on the erroring repos I saw when testing #8249. Notes Addressing this since it came up when testing #8249.
2 participants
Depends on #5222. Review that one first.
In msbuild, one can call targets which do not exist by setting
SkipNonexistentTargetson the MSBuild task. This PR adds support for nonexistent targets in the static graph target protocol.To mark a target as optional, users can now define the following:
<ProjectReferenceTargets Include='Build' Targets='GetTargetFrameworks' SkipNonexistentTargets='true'>.Implementation overview:
ProjectGraph.GetTargetLists()removes nonexistent targets. Since nonexistent targets do not have a BuildResult, the Scheduler now needs to know about this case when it checks whether cache misses are allowed. This required saving a project's targets in a newBuildRequestConfiguration.ProjectTargets, and a corresponding field inBuildResult.Targetswhich ferries the targets from the out of proc nodes to the build manager node.