Fix graph target list for multitargetting projects using BuildProjectReferences=false #8565
Conversation
| targetLists[GetOuterBuild(graph, 2)].ShouldBe(new[] { "GetTargetFrameworks" }); | ||
| foreach (ProjectGraphNode inner in GetInnerBuilds(graph, 2)) | ||
| { | ||
| // GetTargetFrameworks actually shouldn't be here... |
There was a problem hiding this comment.
Ok, I'll bite. Why is it? :)
There was a problem hiding this comment.
Because we need to rewrite how this all works :)
But specifically it's this line:
<ProjectReferenceTargets Include="Build" Targets="GetTargetFrameworks" OuterBuild="true" SkipNonexistentTargets="true" Condition="'$(IsCrossTargetingBuild)' != 'true'" />I couldn't think of a way to ensure that only applies from outer build to outer build and not the inner build.
There was a problem hiding this comment.
The problem was that inner builds were being told to build "what you should build if you're a project reference" instead of what they should be by default, and it seems like the answer you're using here is changing what targets a project reference should build based on whether you're an inner or outer build rather than changing the referencing project's inner builds to use a separate set of targets that includes .default. Is that correct? If so, what makes that way superior to changing the set of targets the inner builds actually build? My (probably wrong) concern is that this will make the inner builds of referenced projects build instead of just assuming they're up-to-date.
| <ProjectReferenceTargets Include="Build" Targets="$(ProjectReferenceTargetsForBuild)" Condition=" '$(ProjectReferenceTargetsForBuild)' != '' " /> | ||
|
|
||
| <ProjectReferenceTargets Include="Clean" Targets="$(ProjectReferenceTargetsForCleanInOuterBuild)" Condition=" '$(ProjectReferenceTargetsForCleanInOuterBuild)' != '' " OuterBuild="true" /> | ||
| <ProjectReferenceTargets Include="Clean" Targets="GetTargetFrameworks" OuterBuild="true" SkipNonexistentTargets="true" /> | ||
| <ProjectReferenceTargets Include="Clean" Targets="GetTargetFrameworks" OuterBuild="true" SkipNonexistentTargets="true" Condition="'$(IsCrossTargetingBuild)' != 'true'" /> |
There was a problem hiding this comment.
I know you just added the condition here and below, but why are we Include'ing Build and Clean multiple times with different Targets lists? I would read that as that we should execute each target multiple times, but that doesn't sound right.
There was a problem hiding this comment.
I don't follow the question. Can you clarify?
There was a problem hiding this comment.
Maybe it would help if you read it this way?
<ProjectReferenceTargets Include="X" Targets="Y" />Means "If I'm called with and entry target X, I call Y on my ProjectRefernces".
There was a problem hiding this comment.
Ah, so when you say:
<ProjectReferenceTargets Include="Clean" Targets="GetTargetFrameworks" />
<ProjectReferenceTargets Include="Clean" Targets="GetTargetFrameworksWithPlatformForSingleTargetFramework" />what you're saying is "If this project builds the Clean target as its entrypoint, build the other two." That makes more sense, thanks.
That doesn't sound right. The problem is that previously the p2p was describing that the outer build only called
Because for inner builds we still have it saying it calls |
This change fixes graph builds for multitargetting projects using BuildProjectReferences=false
Previously, when using BuildProjectReferences=false the graph would consider the outer build calling GetTargetPath on the inner builds. However, it actually calls Build when dispatching to the inner builds, and those call GetTargetPath on their dependencies. In effect this is causing graph builds to be non-graph builds since the target list is not accurately described.
This change ensures the correct behavior by separating the ProjectReferenceTargets logic for single and multi-targetting projects.
Before
Only the top-level project is in the graph, and the inner builds do not have "Build" as a predicted target:
Dispatching to inner builds does not come from cache:
Inner builds resolving dependencies does not come from cache:
After
Everything is in the graph properly:
Dispatching to inner builds comes from cache:
Inner builds resolving dependencies comes from cache:
CC @rainersigwald who was hitting this as well.
CC @DmitriyShepelev who may be interested from an isolation standpoint.