Enable P2P's to determine their own compilation RID by TheRealPiotrP · Pull Request #828 · dotnet/sdk

TheRealPiotrP · 2017-02-07T04:37:47Z

Fixes the RID P2P component of #696

This is part of a two-part PR. The second part is dotnet/msbuild#1674.

What

These PRs, together, enable the following class of projects:

A RID-specific root project [App.csproj]
A portable dependency project [Library.csproj]

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 of RemoveProperties passed to the P2P in _GetProjectReferenceTargetFrameworkProperties. It also passes along the value of RuntimeIdentifier as ReferringRuntimeIdentifier 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:

If the project has RuntimeIdentifier or RuntimeIdentifiers set then use the ReferringRuntimeIdentifier
If the project does not specify a RID through either mechanism then use '' as the RuntimeIdentifier

The 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 Stage0 dotnet directory.

Risk

This feature requires augmenting Microsoft.Common.CurrentVersion.targets. Despite this being a central target, there are mitigating circumstances:

The target modified is specific to the new project system
The modification is a verbatim copy of the identical behavior for TargetFramework

Merge Strategy

Due to the codeflow for these fixes, the following will need to happen in order:

[] MSBuild PR is merged
[] New MSBuild is inserted into CLI
[] SDK's CLI dependency is rev'd to latest and SDK PR is merged
[] New SDK is inserted into CLI

/cc @srivatsn @DustinCampbell @livarcocc @jonsequitur @rainersigwald @AndyGerlicher @nguerrera @dsplaisted

livarcocc · 2017-02-07T04:45:08Z

+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <TargetFramework>netcoreapp1.0</TargetFramework>
+    <RuntimeIdentifiers>osx.10.11-x64</RuntimeIdentifiers>


Will this be a problem for CI? Should you have RuntimeIdentifiers with Win and Ubuntu? the ones used by the SDK CI?

Yes, I will expand to cover all CI flavors. Until the MSBuild fix is available, CI will be red anyways. I wanted to show a single instance of the test for the time being and will cross-target once we decide we're happy with the approach and have merge approval.

livarcocc · 2017-02-07T04:47:49Z

+    public class GivenThatWeWantToBuildASelfContainedAppWithRid : SdkTest
+    {
+        [Fact]
+        public void It_builds_a_runnable_output()


I don't think this describes the scenario enough.

Should this be It_builds_a_runnable_output_when_depending_on_both_rid_and_ridless_libraries?

I fixed the name a bit. The TestAssetsManager in this repo actually checks for long-file-paths and so I had to trim it down a bit. We should think about that feature in CLI!

You can pass in a value to CopyTestAsset which it will use for the folder path, so you can have a long and descriptive test method name, but the path on disk will still be short enough. Here's an example:

sdk/test/Microsoft.NET.Build.Tests/GivenThereAreDefaultItems.cs

Line 526 in b11de8d

var testAsset = _testAssetsManager.CreateTestProject(testProject, "DeduplicatePackage_Reference")

Cool, @dsplaisted! Another feature to integrate over :)

I'm pretty happy with the naming after update, but if folks want it to be different I'll take advantage of this feature.

livarcocc · 2017-02-07T04:50:12Z

    <PropertyGroup>
+      <!-- If this project is not RID-specific, specify that an empty RuntimeIdentifier should be passed. Otherwise, pass through the dependant's RuntimeIdentifier -->
+      <NearestRuntimeIdentifier>$(ReferringRuntimeIdentifier)</NearestRuntimeIdentifier>
+      <NearestRuntimeIdentifier Condition=" '$(RuntimeIdentifier)' == '' and $(RuntimeIdentifiers) == '' "></NearestRuntimeIdentifier>


You are missing single quotes on $(RuntimeIdentifiers). It should be '$(RuntimeIdentifiers'.

Ok. I'll add another csproj to test that branch as well.

@livarcocc I added the test, but it did not identify the bug. I decided to see if this was a test bug by writing the following .proj file:

<Project DefaultTargets="Default"> <Target Name="Default"> <Message Condition="$(foo) == ''" Text="empty == ''" Importance="High" /> </Target> </Project>

The Message did, in fact, print. So it looks like putting single-quotes around a property is more a convention than a rule. That said, the test is in place.

nguerrera · 2017-02-07T05:17:39Z


    <PropertyGroup>
+      <!-- If this project is not RID-specific, specify that an empty RuntimeIdentifier should be passed. Otherwise, pass through the dependant's RuntimeIdentifier -->
+      <NearestRuntimeIdentifier>$(ReferringRuntimeIdentifier)</NearestRuntimeIdentifier>


What about if both are RID-specific, are compatible, but do not match: A (win10-x86) -> B (win7-x86)?

I'm also confused by the term 'dependant' in this context. I'd say 'referrer' to match the variable used.

Thanks Nick, I 'll change the naming.

As to non-matching RIDs, for the moment I'm considering that to be a subsequent feature. The mechanics introduced here enable folks to add RIDs to their projects and get into a working state. RID equivalence mapping seems like a different feature, and a much more risky one.

If you feel that this feature is not useful in the current state please let me know and I'll take a deeper look, but I think that this is a reasonable feature scoping decision.

jonsequitur · 2017-02-07T06:03:30Z

+    {
+        static void Main(string[] args)
+        {
+            var valueFromNativeDependency = Library.NativeCode.InvokeNativeCodeAndReturnAString();


Calling this LibraryWithRid would be a bit clearer.

TheRealPiotrP · 2017-02-11T20:05:22Z

@nguerrera I've made some updates to this, and the corresponding MSBuild, PR. Here is the state of affairs:

Scenarios

Portable-only Solutions
Solution build continues to work.
No Runtime Identifier(s) specified in the project files nor on the command line. ReferringRuntimeIdentifier does not get set and does not get passed to the referenced project. GetTargetFrameworkProperties does not pass back a RuntimeIdentifier override.
Single-RID Solutions
Solution build continues to work.
RuntimeIdentifier specified in project files. RuntimeIdentifier not overridden on the command line.
ReferringRuntimeIdentifier is not set because we detect that RuntimeIdentifier was not set globally.
GetTargetFrameworkProperties does not pass back a RuntimeIdentifier override.
Multiple-RID Solutions
Solution Build continues to fail, outside the scope of this change
RID-specific build of individual projects now starts working.
RuntimeIdentifiers specified in project files. RuntimeIdentifier overridden on the command line.
ReferringRuntimeIdentifier is set because we detect RuntimeIdentifier was set globally.
GetTargetFrameworkProperties provides a RuntimeIdentifier override IFF the referenced project is portable.

Details of update

This PR now includes an SDK ability to detect if the RuntimeIdentifier is passed globally. This is a workaround for an MSBuild feature request. If we detect that the RuntimeIdentifier is not set globaly then execution continues as it does today. If it IS set globaly then we give the referenced project an opportunity to override the RuntimeIdentifier passed to it during the Build phase.

/cc @rainersigwald @nguerrera @srivatsn @DustinCampbell

rainersigwald · 2017-02-13T17:43:49Z

+
+  <!-- If the RuntimeIdentifier was set globally, pass it to GetTargetFrameworkProperties. -->
+  <PropertyGroup Condition=" '$(GlobalRuntimeIdentifier)' != '' ">
+    <AdditionalPropertiesForGetTargetFrameworkProperties>;ReferringRuntimeIdentifier=$(GlobalRuntimeIdentifier)</AdditionalPropertiesForGetTargetFrameworkProperties>


Where is AdditionalPropertiesForGetTargetFrameworkProperties consumed? I don't see it in the MSBuild side of this PR or here.

I need to push my local changes to the MSBuild change.

rainersigwald · 2017-02-13T17:45:49Z

+  <Target Name="GetTargetFrameworkProperties" Returns="TargetFramework=$(NearestTargetFramework);ProjectHasSingleTargetFramework=$(_HasSingleTargetFramework)$(RuntimeIdentifierOverride)">

    <PropertyGroup>
+      <!-- If this project is not RID-specific, specify that an empty RuntimeIdentifier should be passed. -->


Do we actually want to pass an empty RID or is this now just like ProjectHasSingleTargetFramework where it should instead be removed?

Removing it would certainly be better. If there is a clean way to do this, I'm happy to make the change. let's chat.

rainersigwald · 2017-02-13T18:58:31Z

I'm finding this whole thing intensely confusing so I'm drawing some diagrams.

The simplest version of something that would reproduce this problem is a solution with an executable (specifying one, many, or no/all RIDs) and a library (specifying no RID).

The current checked-in situation for publishing the exe for a rid R' is:

That's broken because the RID is specified for lib.csproj which doesn't need or understand it, and so fails.

With this change, I think the intention is to have this flow, with two contexts for lib.csproj but only one build completed:

A solution build would work when no rid is specified:

And that would still be true if there was a RID specified in exe.csproj (but not as a global prop to the sln build), because it's not a global property so the new stuff would work.

A sln build that specified a RID globally would still be broken:

But that's broken today so no loss.

nguerrera

👍 A couple of nits, but looks good and I think combining this with the RID path default change, all of my previous major concerns are now addressed. :)

I'm assuming you have a local setup with fully up-to-date msbuild + your changes where tests are passing. In other words: don't start the merge dance until we are certain that the end result will be green.

nguerrera · 2017-02-14T19:47:37Z

    <PropertyGroup>
+      <!-- If this project is not RID-specific, specify that an empty RuntimeIdentifier should be passed. -->
+      <ProjectIsRidAgnostic>false</ProjectIsRidAgnostic>
+      <ProjectIsRidAgnostic Condition=" '$(RuntimeIdentifier)' == '' and '$(RuntimeIdentifiers)' == '' ">true</ProjectIsRidAgnostic>


nit: inconsistency in naming of single-target framework vs rid-agnostic. I suggest _IsRidAgnostic to continue the nearby pattern.

nit 2: comment is not quite up to date with code changes here and in msbuild. How about something like "indicate to caller that project is RID agnostic so that a global property RuntimeIdentifier value can be removed" or something?

srivatsn · 2017-02-14T19:58:57Z

Tagging @MattGertz for approval.

MattGertz · 2017-02-14T20:00:49Z

From now on, all submissions need graph paper diagrams. :-)

TheRealPiotrP · 2017-02-16T02:40:04Z

Nits addressed, as well as x-plat test issues. This is ready to go once approved. Whoever merges, please squash :D

nguerrera · 2017-02-16T08:16:20Z

          DestinationFolder="$(TestsDirectory)"
          />

+<Message Text="$(DotNetTool) &quot;$(TestsDirectory)\xunit.console.netcore.exe&quot; &quot;@(TestAssembly, '&quot; &quot;')&quot; -xml &quot;@(XmlTestFile)&quot;" Importance="High" />


Did you mean to check this in? I like it, but the formatting is off and it would be nice not to duplicate the command in Message and Exec.

…0190801.3 (dotnet#828) - Microsoft.AspNetCore.Mvc.Analyzers - 5.0.0-alpha1.19401.3 - Microsoft.AspNetCore.Mvc.Api.Analyzers - 5.0.0-alpha1.19401.3 - Microsoft.AspNetCore.Analyzers - 5.0.0-alpha1.19401.3 - Microsoft.AspNetCore.Components.Analyzers - 5.0.0-alpha1.19401.3

dnfclas added the cla-already-signed label Feb 7, 2017

TheRealPiotrP changed the title ~~Piotrp msft/issues/696/p2pbuild r~~ Enable P2P's to determine their own compilation RID Feb 7, 2017

TheRealPiotrP mentioned this pull request Feb 7, 2017

Enable P2P's to determine their own compilation RID dotnet/msbuild#1674

Merged

livarcocc reviewed Feb 7, 2017

View reviewed changes

nguerrera reviewed Feb 7, 2017

View reviewed changes

jonsequitur reviewed Feb 7, 2017

View reviewed changes

TheRealPiotrP mentioned this pull request Feb 7, 2017

Runtime-specific build fails for projects with P2P references #696

Closed

TheRealPiotrP force-pushed the piotrpMSFT/issues/696/p2pbuild-r branch 2 times, most recently from 3a80358 to 27b7e0e Compare February 11, 2017 00:38

rainersigwald reviewed Feb 13, 2017

View reviewed changes

rainersigwald mentioned this pull request Feb 13, 2017

Solution-level error when a RuntimeIdentifier is specified #863

Closed

TheRealPiotrP force-pushed the piotrpMSFT/issues/696/p2pbuild-r branch from 27b7e0e to 1f18bea Compare February 14, 2017 08:20

rainersigwald approved these changes Feb 14, 2017

View reviewed changes

nguerrera approved these changes Feb 14, 2017

View reviewed changes

MattGertz added the Pending Shiproom Approval label Feb 14, 2017

natidea mentioned this pull request Feb 14, 2017

Use RID less targets to obtain compile assets #836

Merged

nguerrera mentioned this pull request Feb 15, 2017

Remove requirement for specifying a RID in desktop exe projects #847

Merged

TheRealPiotrP force-pushed the piotrpMSFT/issues/696/p2pbuild-r branch from 1f18bea to 3768b8f Compare February 16, 2017 00:01

Piotr Puszkiewicz added 6 commits February 15, 2017 17:50

Initial test asset

122974c

Red test

6e4147e

Enable building Portable P2P dependencies of RID-specific projects

bc0ed40

PR feedback

bf2aec9

Avoid RuntimeIdentifer override if ReferringRuntimeIdentifier == ''

7a3a4ce

Override Runtime Identifier IFF this proj portable & referring is not

ad9f19e

Unknown added 12 commits February 15, 2017 17:50

Detect Global RuntimeIdentifier

df90c6d

Comments.

3838328

Account for Rainer's feedback.

4b47036

Missing quote

d9d3d55

Update CLI

98e2390

Remove accidentally added .vscode files

78a0bd4

add .vscode to .gitignore

5996796

Add missing RIDs

13fc5dd

PR feedback

27381b7

Add missing RIDs to Lib

ad42880

RID-specific lib...

4235a57

Fix switch conditions

46f63c5

TheRealPiotrP force-pushed the piotrpMSFT/issues/696/p2pbuild-r branch from 96062c9 to 46f63c5 Compare February 16, 2017 01:50

Unknown added 2 commits February 15, 2017 18:09

Fix x-plat native invocation

62ad144

Disable test on Desktop MSBuild

7f5553c

nguerrera reviewed Feb 16, 2017

View reviewed changes

nguerrera approved these changes Feb 16, 2017

View reviewed changes

natidea added the Approved to merge label Feb 16, 2017

TheRealPiotrP merged commit a7ea14c into dotnet:master Feb 16, 2017

TheRealPiotrP deleted the piotrpMSFT/issues/696/p2pbuild-r branch February 16, 2017 21:59

ghost mentioned this pull request Jun 17, 2021

Net5.0 RC2: When building a solution with a target runtime ID an error is given #14281

Open

rainersigwald mentioned this pull request Oct 18, 2022

[7.0.2xx] Change Global Property Flow to Not Flow Publish* Properties #28645

Open

Conversation

TheRealPiotrP commented Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

How

Testing

Risk

Merge Strategy

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheRealPiotrP commented Feb 11, 2017

Scenarios

Details of update

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rainersigwald commented Feb 13, 2017

Uh oh!

nguerrera left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srivatsn commented Feb 14, 2017

Uh oh!

MattGertz commented Feb 14, 2017

Uh oh!

TheRealPiotrP commented Feb 16, 2017

Uh oh!

nguerrera Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

TheRealPiotrP commented Feb 7, 2017 •

edited

Loading

nguerrera Feb 16, 2017 •

edited

Loading