-
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
Enable P2P's to determine their own compilation RID #1674
Enable P2P's to determine their own compilation RID #1674
Conversation
@AndyGerlicher @rainersigwald I wasn't sure what level of testing you would want here. Please do suggest. |
3075c71
to
45d0642
Compare
ContinueOnError="!$(BuildingProject)" | ||
RemoveProperties="%(_MSBuildProjectReferenceExistent.GlobalPropertiesToRemove);TargetFramework" | ||
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.
I think this new dimension will reintroduce build races between sln traversal and P2P traversal. Sln traversal will build all projects without /p:RuntimeIdentifier, but P2P traversal will always pass /p:RuntimeIdentifier. This will cause MSBuild to think it can do both builds in parallel but they both output to the same directory by default.
I think we need two things:
- RID (if non-empty) must be appended to build output path by default
- We must add RuntimeIdentifier to GlobalPropertiesToRemove if it comes back empty
(1) is something we just shied away from as being too risky :(
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.
Ugh, I think even that fix is inadequate for VS builds, which would only build projects with their own RIDs and never those of callers.
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.
@nguerrera @rainersigwald how do you recommend I proceed?
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.
nvm. Just re-read what @nguerrera wrote :)
Unfortunately, it isn't. All project types can be affected by changes here as shown by the recent classic PCL regression. The reason it is here and not in the SDK is to handle refs from non-SDK to SDK. |
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.
See above
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.
Talked to @piotrpMSFT offline.
I think this is a reasonable change to take, though the risk is high at the moment.
Currently, builds are broken when you specify a RID as a global property and have a p2p ref to a ridless project.
With this change,
dotnet build -r RID some.csproj
will work (RID is explicitly queried for and unset in p2p)dotnet build -r RID larger.sln
will have the race condition @nguerrera points out between the sln -> csproj (specifies RID) build and the csproj -> csproj (queried for and unset RID) build--but that's already broken todaydotnet build larger.sln
will succeed because RID isn't set as a global property so the sln->csproj ref won't have it set in the first place, collapsing with the csproj->csproj build.
So this seems like an improvement even though it introduces races, because those races will occur in already-broken builds.
I'd like us to take some time to talk through other solutions. I'm not OK with just changing works never -> works sometimes (non-deterministically) and checking this box. |
Maybe I'm missing something, but I don't think this is right. During execution of |
After this change, we'd get
Right? Or is the engine not collapsing the first and last because we're not smart enough to distinguish between not-set and set-to-nothing? |
I thought that's what I learned from dealing with TargetFramework, but you're the expert on this. ;) I would love to be wrong. We should triple check the code and and stress test /m to be sure. And there is still another case (I think): P1 and P2 both have RuntimeIdentifier set in csproj. P1 references P2.
2 and 3 can collide, no? |
@piotrpMSFT Can you use your modified-with-privates CLI to test the |
45d0642
to
a7ec61b
Compare
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.
This could be done more efficiently folded together with the previous case, but I don't want to juggle the complexity of doing so now--risk of regression seems too high. LGTM
@@ -1576,6 +1576,14 @@ Copyright (C) Microsoft Corporation. All rights reserved. | |||
</_MSBuildProjectReferenceExistent> | |||
</ItemGroup> | |||
|
|||
<ItemGroup> |
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 should be able to put this in the prior ItemGroup
.
<_MSBuildProjectReferenceExistent Condition="'%(_MSBuildProjectReferenceExistent.Identity)' == '%(Identity)' and '$(_ProjectReferenceTargetFrameworkProperties)' != ''"> | ||
<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> |
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 must be missing something since the above code with same pattern would also be broken, but where is %(UndefineProperties) handled? I'm only seeing it here in search of whole common targets file. I am instead seeing %(GlobalPropertiesToRemove) elsewhere, including just above.
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.
Just waiting on the answer to my question. Approach seems right.
It's handled intrinsically by the msbuild task. In retrospect I probably should have used the one that's used explicitly in the targets but now I favor consistency.
|
Fixes the RID P2P component of dotnet/sdk#696
This is part of a two-part PR. The second part is dotnet/sdk#828.
What
These PRs, together, enable the following class of projects:
With this fix in place, one can:
dotnet restore App.csproj
dotnet build -r osx.10.11-x64 App.csproj
Without the fix, Library.csproj will not build because the
osx.10.11-x64
runtime identifier will be passed through as a global property, forcing it to build for a target that does not exist in the NuGet assets file.How
The MSBuild portion of the fix adds
RuntimeIdentifier
to the list ofRemoveProperties
passed to the P2P in_GetProjectReferenceTargetFrameworkProperties
. It also passes along the value ofRuntimeIdentifier
asReferringRuntimeIdentifier
so that the dependency can decide how it wants to interpret that Runtime Identifier. The model used is identical to that used for TargetFramework.The SDK portion of the fix expands the
GetTargetFrameworkProperties
target to also account for RuntimeIdentifiers. The logic is as follows:RuntimeIdentifier
orRuntimeIdentifiers
set then use the ReferringRuntimeIdentifier''
as the RuntimeIdentifierThe effect is that RID-specific projects will attempt to build with the RID passed to the root project. Portable projects will be built without a RID specified.
Testing
The PR includes a thorough test for the scenario in question, building a RID-specific project that references both a RID-specific and Portable library. Both libraries are proven to compile successfuly through runtime invocation. The RID-specific library both invokes a RID-specific native dependency AND inspects its own RID via an overloaded AssemblyInfo Description attribute.
The feature was enabled without impacting any existing SDK tests.
The combination of these changes was tested on my local machine by manually editing
Microsoft.Common.CurrentVersion.targets
in the Stage0dotnet
directory.Risk
This feature requires augmenting
Microsoft.Common.CurrentVersion.targets
. Despite this being a central target, there are mitigating circumstances:Merge Strategy
Due to the codeflow for these fixes, the following will need to happen in order:
/cc @srivatsn @DustinCampbell @livarcocc @jonsequitur @rainersigwald @AndyGerlicher @nguerrera @dsplaisted