-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
Ask ProjectReferences if they cross-target #1866
Ask ProjectReferences if they cross-target #1866
Conversation
Before cross-targeting was implemented, a project had a single output that was queried in ResolveProjectReferences by either `GetTargetPath` (in single-project-build scenarios) or the default target (`Build`) in command line builds. A cross-targeting project doesn't have a single output--it has one for each of its `TargetFrameworks`. A ProjectReference, then, has to first figure out which output to ask for--the one that's the best fit for the current project. The initial implementation of this was simple: unconditionally call `GetTargetFrameworkProperties` on every ProjectReference, passing a `ReferringTargetFramework`. Then, if the project cross-targets, specify the resulting TF on future calls to the ProjectReference. If the project has a single TF, do not specify it to avoid building it once (from the solution build) and again (from a reference, explicitly specifying the one TF). Unfortunately, adding a global property for `ReferringTargetFramework` will result in a separate evaluation for the project. This means that almost _every_ project in a solution will be evaluated at least twice--once directly, with no special global properties, and once for each unique `ReferringTargetFramework` that refers to it. This can be particularly onerous for C++ projects (.vcxproj), where evaluation time is nontrivial and doubling it is noticeable. That was mitigated by special-casing vcxproj in dotnet#1278, but that is inelegant and doesn't address the root cause. This change alters the calling pattern for `ProjectReference`s. Instead of asking all references for the correct TF, it breaks the problem into multiple phases: * Ask all references whether they cross-target, and split into cross-targeting and non-cross-targeting lists. * Perform the `GetTargetFrameworkProperties` query only on projects that reported that they cross-targeted. * Update (in place) the existing `_MSBuildProjectReferenceExistent` item to pass or undefine the relevant properties based on whether the reference cross-targets (and what TF is the best match). Fixes dotnet#1276.
I feel like I had a more elegant solution to the problem "how do I ensure that TF and RID get removed from global properties in non-cross-targeting projects" in my previous attempt (now lost due to my foolishness/angering of the Bitlocker gremlins). Comments on general approach eagerly desired, as well as implementation improvements. Especially interested in feedback from @nguerrera. |
target called with a ReferringTargetFramework. If "false", | ||
the single output of the project is used. | ||
--> | ||
<Target Name="IsCrossTargetingProject" Returns="false" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the Returns="true" version need to go in the SDK or should it be in Microsoft.Common.CrossTargeting.targets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put it in the sdk but it should be in common.crosstargeting. Nice catch.
<UndefineProperties Condition="$(_ProjectReferenceTargetFrameworkProperties.Contains(`ProjectIsRidAgnostic=true`))">%(_MSBuildProjectReferenceExistent.UndefineProperties);RuntimeIdentifier;ProjectIsRidAgnostic</UndefineProperties> | ||
<!-- Unconditionally remove the property that was set as a marker to indicate that for this call we should remove RuntimeIdentifier --> | ||
<UndefineProperties Condition="!$(_ProjectReferenceTargetFrameworkProperties.Contains(`ProjectIsRidAgnostic=true`))">%(_MSBuildProjectReferenceExistent.UndefineProperties);ProjectIsRidAgnostic</UndefineProperties> | ||
<!-- If the project has only one RID, assume it's compatible with the current project and don't pass this one along. --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"If the project has only one RID" doesn't match IsRidAgnostic == true, does it? I thought that is the case where the project has no mention of RIDs. cc @piotrpMSFT
<_ProjectReferencesWithTargetFrameworkProperties Include="@(_ProjectReferenceTargetFrameworkProperties->'%(OriginalItemSpec)')" /> | ||
<_ProjectReferencesWithTargetFrameworkProperties Include="@(_MSBuildProjectReferenceNoCrossTargeting)"> | ||
<HasSingleTargetFramework>true</HasSingleTargetFramework> | ||
<IsRidAgnostic>true</IsRidAgnostic> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe IsRidAgnostic=true holds for any non-cross-targeting project.
If I have two single-targeted projects with project A having RIDs and proejct B having none, with A referencing B, then dotnet build -r some-rid A.csproj
still needs to remove the global RID property from B's build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering about making IsCrossTargetingProject more general GetProjectCapabilities
or something so that we can answer more than one question about the reference with this same call. i.e. whether it is rid-agnostic independently from whether it is cross-targeting. But that would require removing TargetFramework and RuntimeIdentifier from the evaluation and get us back to where we started perf-wise, won't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that IsRidAgnostic=true
means "do not pass down a RID", so setting it on non-cross-targeting projects would be the right thing for your scenario, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sheesh, got my example wrong. The case that differs from prior implementation is when B has RuntimeIdentifiers defined but is single-targeted. In that case, the RID would flow before, but not now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I broke that case. That should be fixed by the changes I'm making for dotnet/sdk#993 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So thinking about this I think the question we want to ask isn't really "IsCrossTargetingProject?" but more like "ShouldCallGetMatchingProperties?". Then I can move that back into the sdk (from common.crosstargeting) and teach it to return "yes" in the multi-rid-but-single-tfm situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented this in the latest push here and in dotnet/sdk#993.
Do you have numbers on the perf improvement? |
Instead of returning a semicolon-delimited string, which must be parsed on the receiving end and cannot be easily converted to an item (with relevant metadata like which project it's associated with), return structured data in the only way MSBuild can: as an item with metadata. Metadata includes the closest matching TargetFramework and whether or not the project should have TargetFramework and/or RuntimeIdentifier stripped before calling it. Consumed by Microsoft.Common.CurrentVersion.targets in the coordinated change dotnet/msbuild#1866.
* Rename targets to be more descriptive * Leave ShouldQueryForProperties as a property of the SDK
Projects="%(_MSBuildProjectReferenceExistent.Identity)" | ||
Targets="GetTargetFrameworkProperties" | ||
Projects="@(_MSBuildProjectReferenceCrossTargeting)" | ||
Targets="GetReferenceProperties" | ||
BuildInParallel="$(BuildInParallel)" | ||
Properties="%(_MSBuildProjectReferenceExistent.SetConfiguration); %(_MSBuildProjectReferenceExistent.SetPlatform); ReferringTargetFramework=$(ReferringTargetFrameworkForProjectReferences)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self, these metadata need to refer to the projects that are actually being built!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. ;)
BuildInParallel="$(BuildInParallel)" | ||
Properties="%(_MSBuildProjectReferenceExistent.SetConfiguration); %(_MSBuildProjectReferenceExistent.SetPlatform)" | ||
ContinueOnError="!$(BuildingProject)" | ||
RemoveProperties="%(_MSBuildProjectReferenceExistent.GlobalPropertiesToRemove);TargetFramework;RuntimeIdentifier" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see that we do remove TargetFramework and RuntimeIdentifier from this evaluation. Nevermind comment on SDK side.
Is this considered OK perf-wise because the common case is that neither are specified as global properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's my operating theory, yes. Additionally, if the common case is that references single-target, then that will be the evaluation we want to use later, so this doesn't come at an additional cost (just moves it around a bit).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice as escape hatch if the set of properties removed from ShouldQueryForProperties were configurable and defaulted TargetFramework;RuntimeIdentifier. I'm thinking that another global property might come along and bite us later on and an escape hatch here could help. This can be done/tracked separately, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a very good idea.
--> | ||
<ItemGroup> | ||
<_MSBuildProjectReferenceExistent Condition="'%(_MSBuildProjectReferenceExistent.SetTargetFramework)' != ''"> | ||
<SkipGetTargetFrameworkProperties>true</SkipGetTargetFrameworkProperties> | ||
<SkipGetReferenceProperties>true</SkipGetReferenceProperties> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned about breaking change here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that they are still called TargetFrameworkProperties in places. And still go to %(SetTargetFramework). I think we should rename all or none, and lean towards none for compat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. My find-and-replace was too broad.
Perf improvementsThese are measured on a set of projects that the C++ team has that mirror the structure of the customer project that originally caused us to put in the .vcxproj workaround. Timings aren't super scientific since this is on my dev box and N=1. RTW targets
RTW minus the .vcxproj workaround
This PR
Results. . . so there are more calls to the MSBuild task than RTW, but total time and MSBuild task times are similar, and both are better than when dropping the special-case for vcxproj (as we had it initially). |
@@ -1555,43 +1581,45 @@ Copyright (C) Microsoft Corporation. All rights reserved. | |||
</PropertyGroup> | |||
|
|||
<MSBuild | |||
Projects="%(_MSBuildProjectReferenceExistent.Identity)" | |||
Targets="GetTargetFrameworkProperties" | |||
Projects="@(_MSBuildProjectReferenceCrossTargeting)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could non cross-targetting projects ever have special props for their target framework?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They can. Looks like a missing rename after discussing exactly those cases.
Projects="%(_MSBuildProjectReferenceExistent.Identity)" | ||
Targets="GetTargetFrameworkProperties" | ||
Projects="@(_MSBuildProjectReferenceCrossTargeting)" | ||
Targets="GetReferenceProperties" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also rename target to _GetProjectReferenceProperties
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's "public" API surface so I don't like the initial underscore. I'm not sure I prefer GetProjectReferenceProperties
. Maybe GetDesiredProperties
?
This is a compat shim, and should be reverted once dotnet/msbuild#1866 is available for use in the SDK. It will not be used with common targets that contain that change.
</MSBuild> | ||
|
||
<ItemGroup> | ||
<_MSBuildProjectReferenceCrossTargeting Include="@(_MSBuildProjectReferenceCrossTargetingResults->'%(OriginalItemSpec)')" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/_MSBuildProjectReferenceCrossTargeting/_MSBuildProjectReferenceNeedingProperties/ (or something like that) as this is no longer coupled to cross-targeting cases alone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good modulo:
-
Naming issue where we're still mentioning cross-targeting
-
Your note-to-self https://github.com/Microsoft/msbuild/pull/1866/files#r106954830
Also, as far as validation, it would be extremely helpful to run SDK tests with a private drop of MSBuild with this change and the other half on the SDK side.
This allows an escape hatch if unconditionally removing `TargetFramework;RuntimeIdentifier` is wrong for a given project.
Allow projects to directly control the global properties that should be unset when building them as a single-targeted project.
MSBuild triage: we're not going to pursue this for now. |
Before cross-targeting was implemented, a project had a single output
that was queried in ResolveProjectReferences by either
GetTargetPath
(in single-project-build scenarios) or the default target (
Build
) incommand line builds.
A cross-targeting project doesn't have a single output--it has one for
each of its
TargetFrameworks
. A ProjectReference, then, has to firstfigure out which output to ask for--the one that's the best fit for the
current project.
The initial implementation of this was simple: unconditionally call
GetTargetFrameworkProperties
on every ProjectReference, passing aReferringTargetFramework
. Then, if the project cross-targets, specifythe resulting TF on future calls to the ProjectReference. If the project
has a single TF, do not specify it to avoid building it once (from the
solution build) and again (from a reference, explicitly specifying the
one TF).
Unfortunately, adding a global property for
ReferringTargetFramework
will result in a separate evaluation for the project. This means that
almost every project in a solution will be evaluated at least
twice--once directly, with no special global properties, and once for
each unique
ReferringTargetFramework
that refers to it. This can beparticularly onerous for C++ projects (.vcxproj), where evaluation time
is nontrivial and doubling it is noticeable. That was mitigated by
special-casing vcxproj in #1278, but that is inelegant and doesn't
address the root cause.
This change alters the calling pattern for
ProjectReference
s. Insteadof asking all references for the correct TF, it breaks the problem into
multiple phases:
cross-targeting and non-cross-targeting lists.
GetTargetFrameworkProperties
query only on projects thatreported that they cross-targeted.
_MSBuildProjectReferenceExistent
itemto pass or undefine the relevant properties based on whether the
reference cross-targets (and what TF is the best match).
Fixes #1276 (when combined with an Sdk fix, forthcoming)