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

nguerrera · 2017-02-09T19:19:55Z

Customer scenario

Cannot build AnyCPU desktop app (common case) using SDK/new project system without applying a cumbersome and hard-to-understand workaround.
Cannot build desktop app with native nuget dependencies without making a RID choice.

The approach taken is scoped to address these two common cases:

Building AnyCPU desktop application
Building win7+ desktop applications with native dependencies

Both of the above will happen automatically and F5 successfully without any mention of RuntimeIdentifier or PlatformTarget in the project. Other scenarios will still require adjusting the RID and PlatformTarget to resolve the native dependencies correctly and stamp the resulting binary correctly.

Follow-ups to improve the experience outside these cases:

Bugs this fixes:
Fix #396 and its myriad of duplicates/consequences.

Workarounds, if any
Specify dummy runtime identifier for AnyCPU app, and explicitly let PlatformTarget to AnyCPU.
Always pick a RID whenever taking native nuget dependencies from a desktop app.

Risk
Low. We address two common cases with new defaults. Specifying PlatformTarget and RuntimeIdentifier explicitly shuts off these defaults and gives the user the same control they had before. We simply make it so that this control needn't be used for the most common cases.

Performance impact
Minimal. A few extra checks are needed in msbuild evaluation.

Is this a regression from a previous update?
No.

Root cause analysis
N/A since we are amending a deliberate design decision in response to significant customer and partner feedback.

How was the bug found
Customer and partner reported.

@srivatsn @dsplaisted @eerhardt @dotnet/project-system

dsplaisted

This looks good. Congrats on figuring out a reasonable way of improving the user experience here.

dsplaisted · 2017-02-09T22:40:14Z

TestAssets/TestProjects/DesktopMinusRid/Program.cs

+
+#if USE_NATIVE_CODE
+        [DllImport("libuv", CallingConvention = CallingConvention.Cdecl)]
+        static extern int uv_loop_size();


This seems like the test might eventually break if the Libuv and Kestrel packages get updated to support AnyCPU .NET Framework deployments by deploying both x86 and x64 versions of the native library and Kestrel dynamically chooses which one to load.

I'm not suggesting you change this now, just pointing it out.

That would only happen if my libuv package ref was upgraded, which is unexpected and I'd be happy to have tests fail in that case.

dsplaisted · 2017-02-09T22:49:31Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.RuntimeIdentifierInference.targets

+
+      2. Building an x86 or x64 NETFramework application on and for
+         Windows with native NuGet dependencies that do not require
+         greater thanwin7.


Missing space: thanwin7

dsplaisted · 2017-02-09T22:51:22Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.RuntimeIdentifierInference.targets

@@ -60,9 +114,19 @@ Copyright (c) .NET Foundation. All rights reserved.
    <OutputPath>$(OutputPath)$(RuntimeIdentifier)\</OutputPath>
  </PropertyGroup>

-  <Target Name="_CheckRuntimeIdentifier" BeforeTargets="_CheckForInvalidConfigurationAndPlatform">
-    <NetSdkError Condition="'$(RuntimeIdentifier)' == '' and '$(TargetFrameworkIdentifier)' == '.NETFramework' and '$(OutputType)' == 'Exe'"
-                 ResourceName="RuntimeIdentifierMustBeSetForNETFramework" />


Should we remove this resource (RuntimeIdentifierMustBeSetForNETFramework)? Or is that even something we can do?

dsplaisted · 2017-02-09T22:53:56Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.RuntimeIdentifierInference.targets

+    Switch our default .NETFramework CPU architecture choice back to AnyCPU before 
+    compiling the exe if no copy-local native dependencies were resolved from NuGet 
+  -->
+  <Target Name="AdjustDefaultPlatformTargetForNetFrameworkExeWithNoNativeCopyLocalItems"


How likely is it that there could be something else that consumes the PlatformTarget property between when we set it to x86 or x64 based on the defaulted RuntimeIdentifier and when we set it back to AnyCPU because we didn't resolve any native assets?

Hmm, I'm reviewing the uses of PlatformTarget in msbuild /pp output. There's quite a bit more than I'd hoped.

I moved it to run soon after the assets are resolved.

dsplaisted · 2017-02-09T22:58:00Z

TestAssets/TestProjects/DesktopMinusRid/Program.cs

+
+        static ProcessorArchitecture GetProcessorArchitecture()
+        {
+            return AssemblyName.GetAssemblyName(Assembly.GetExecutingAssembly().Location).ProcessorArchitecture;


I'd suggest typeof(Program).Assembly.Location. I don't know that it matters at all, but Assembly.GetExecutingAssembly() is one of the methods that we originally removed from .NET Core because it doesn't play well with optimizations we might want to do (inlining across assembly boundaries).

Naming suggestion: The name GetProcessorArchitecture makes me think it will return the architecture of the physical CPU or the OS. Something like GetCurrentAssemblyProcessorArchitecture would be clearer to me, if more verbose.

I'll make the change to avoid any question, but Assembly.GetExecutingAssembly() doesn't suffer from these problems intrinsically. Assembly.GetCallingAssembly is the evil one. GetExecutingAssembly was guilty only by association and that we never bothered to make it intrinsic with identical code gen to typeof(CurrentType).Assembly.Location.

dsplaisted · 2017-02-09T23:09:34Z

@mlorbetske @vijayrkn Once this is merged, I believe you should be able to remove the RuntimeIdentifier property from the web templates (for example here. You don't necessarily have to do it for RTM if it doesn't meet the bar though.

@piotrpMSFT @livarcocc Same thing for dotnet migrate - If you have logic that sets a RuntimeIdentifier property when targeting .NET Framework because we didn't have the same default in the .NET SDK as project.json did, you can probably remove that logic once this is merged.

mlorbetske · 2017-02-10T00:48:17Z

@phenning as fyi

livarcocc · 2017-02-11T00:07:04Z

Thanks for the heads up @nguerrera. I have filled https://github.com/dotnet/cli/issues/5668 to track this on the CLI side.

nguerrera · 2017-02-13T19:27:39Z

@MattGertz for approval

nguerrera · 2017-02-13T21:27:34Z

@MattGertz @natidea @srivatsn Approved to merge or pending ship room?

srivatsn · 2017-02-13T21:33:33Z

Approved at shiproom. However @rrelyea wanted to understand if the RID that we infer is similar to what's done in NuGet.targets.

nguerrera · 2017-02-13T21:41:40Z

@rrelyea @emgarten Can you point me to what nuget.targets does?

emgarten · 2017-02-13T23:40:18Z

@nguerrera this exists in the build task: https://github.com/NuGet/NuGet.BuildTasks/blob/dev/src/Microsoft.NuGet.Build.Tasks/Microsoft.NuGet.targets#L94-L96

NuGet.targets/restore does not infer RIDs anymore like it did in preview2. It only reads the RuntimeIdentifier and RuntimeIdentifiers properties.

nguerrera · 2017-02-14T00:18:18Z

Hold on merging this.

nguerrera · 2017-02-14T21:56:22Z

Approved at shiproom. However @rrelyea wanted to understand if the RID that we infer is similar to what's done in NuGet.targets.

@nguerrera this exists in the build task: https://github.com/NuGet/NuGet.BuildTasks/blob/dev/src/Microsoft.NuGet.Build.Tasks/Microsoft.NuGet.targets#L94-L96

OK. The default RIDs (plural) there are not win7+ whereas we pick win7+ for the implicit RID here. This is a pragmatic choice to make popular packages like libuv work on F5. Ultimately, the design would be to pick as inclusive as possible for current OS, but that is tracked by #840

There is no bad interaction between those and this: restore will just use our RID plus all of the RIDs there and produce an assets file with a superset of what this change needs.

nguerrera · 2017-02-14T21:58:33Z

(But still making adjustments based on other issues.)

natidea

Waiting for test

nguerrera · 2017-02-15T02:14:37Z

@natidea @srivatsn @dsplaisted I've got all the changes discussed made and test coverage added to handle it.

411f959 is unfortunate workaround for case where the net4x app is one target of many. In that case, nuget restore doesn't see the inferred RID of inner build. The fix is a hammer to include win7-x86 and win7-x64 RIDs (plural) in all multi-targeting builds on Windows.

dsplaisted · 2017-02-15T02:46:16Z

test/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildADesktopExe.cs

+                    var ns = project.Root.Name.Namespace;
+                    var propertyGroup = project.Root.Elements(ns + "PropertyGroup").First();
+                    propertyGroup.Add(new XElement(ns + "RuntimeIdentifier", rid));
+                    propertyGroup.Add(new XElement(ns + "AppendRuntimeIdentifierToOutputPath", useAppendOption.ToString()));


Should we be testing the case where AppendRuntimeIdentifierToOutputPath is not specified in the project, as that will be the common case?

dsplaisted · 2017-02-15T02:49:43Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.RuntimeIdentifierInference.targets

@@ -122,7 +122,7 @@ Copyright (c) .NET Foundation. All rights reserved.
    compiling the exe if no copy-local native dependencies were resolved from NuGet 
  -->
  <Target Name="AdjustDefaultPlatformTargetForNetFrameworkExeWithNoNativeCopyLocalItems"
-          DependsOnTargets="ResolvePackageDependenciesForBuild"
+          AfterTargets="ResolvePackageDependenciesForBuild"


Can you explain this change? I haven't entirely grokked the difference between DependsOnTargets and AfterTargets, but I think this means that this target could run before ResolvePackageDependenciesForBuild if something else depended on this target.

I am trying to make sure that this is done as soon as we know the right PlatformTarget. Somebody could indeed force it to be sooner.

I'm not really concerned about someone trying to run this target themselves at some other time and I don't want to reset testing for this.

dsplaisted · 2017-02-15T02:52:57Z

src/Tasks/Microsoft.NET.Build.Tasks/buildCrossTargeting/Microsoft.NET.Sdk.targets

+    build, we may implicitly use one of these RIDs and so we must ensure that restore 
+    includes them.
+   -->
+  <PropertyGroup Condition="'$(RuntimeIdentifiers)' == '' and '$(OS)' == 'Windows_NT'">


Could we check to see whether RuntimeIdentifiers contains a net* target framework? Even just checking if the string contains net would allow us to avoid this in most cases where we don't need it.

It will contain 'net' in most cases: netstandard netcoreapp. I don't want to add regex or other parsing complexity here.

srivatsn · 2017-02-15T04:19:03Z

Is this ready to merge now?

nguerrera · 2017-02-15T04:40:29Z

Validating Daniel's question about change to AfterTargets and saw that there's an issue with the final change for the multi-targetng case. It won't kick in in full msbuild/VS because of the other issue where Microsoft.Nuget.targets is imported. The satellite test that's covering that is disabled on full msbuild right now.

Working on it.

nguerrera · 2017-02-15T06:01:18Z

Jenkins is down, but validated locally that d1c2fd4 closes desktop msbuild test gap, and 5046d92 turns it green. @dsplaisted, @srivatsn please review those changes.

nguerrera · 2017-02-15T12:38:05Z

Do not merge.

The change to deal with multi-targeting case has interactions with #870 and #828 that need to be discussed.

natidea · 2017-02-15T18:02:24Z

@dotnet-bot test this please

Fix dotnet#871

nguerrera · 2017-02-15T20:10:23Z

@dsplaisted @natidea @srivatsn I've now fixed the mulitargeting issue in a much more general way that:

also fixes Different RID behavior for multitargeted restore in VS and command line #871
does not regress the lessening of downgrade warnings in Skip import of Microsoft.NuGet.props and Microsoft.NuGet.targets #870
does not interact with the RID handling of Enable P2P's to determine their own compilation RID #828

Please review efbcaca, which does this and is squashed delta since sign-offs. I've also asked @emgarten to review.

I am also going to add some unit tests to check GetAllRuntimeIdentifiers directly and not just in the end-to-end scenario this PR sets out to address.

dsplaisted · 2017-02-16T00:08:09Z

src/Tasks/Microsoft.NET.Build.Tasks/buildCrossTargeting/Microsoft.NET.Sdk.targets

+    </ItemGroup>
+
+    <MSBuild Projects="$(MSBuildProjectFile)"
+             Condition="'$(TargetFrameworks)' != ''"


This condition doesn't seem to be strictly necessary, as this file should only be imported when TargetFrameworks is defined, right?

dsplaisted · 2017-02-16T00:12:06Z

test/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildADesktopExe.cs

+            buildCommand
+                .Execute()
+                .Should()
+                .Pass();


Should this be checking that the output is built using the correct RID, as is the case with It_handles_native_depdencies_and_platform_target?

nguerrera · 2017-02-16T00:55:55Z

@emgarten approved here #871 (comment)

He noted offline that rids are technically case sensitive, so I'll switch to DistinctWithCase().

I am still adding GetAllRuntimeIdentifiers unit tests and I'll add the coverage @dsplaisted requested above. I have to run to an appointment so I'll merge this later tonight once the extra tests are in.

nguerrera · 2017-02-16T07:30:01Z

Abandoning DistinctWithCase because MSBuild is already deduping case elsewhere. I think we have bigger problems if real RIDs ever differ only by case (e.g. path conflicts, endless confusion), so I think we can ignore it.

…805.9 (dotnet#847) - Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19405.9

Remove requirement for specifying a RID in desktop exe projects

8ad6b90

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

dsplaisted approved these changes Feb 9, 2017

View reviewed changes

natemcmaster mentioned this pull request Feb 10, 2017

Use SQLitePCL.raw aspnet/Microsoft.Data.Sqlite#320

Merged

ericsink mentioned this pull request Feb 10, 2017

Need <pack>false</pack> ericsink/SQLitePCL.raw#140

Closed

nguerrera added 4 commits February 13, 2017 11:30

PR feedback: fix comment typo

a1f6a81

PR feedback: improve method name and avoid GetExecutingAssembly

4107cb8

PR feedback: remove unused resource string

75744ed

Run xlf converter

3363efb

MattGertz added the Approved to merge label Feb 13, 2017

natidea added the Pending Shiproom Approval label Feb 13, 2017

natidea suggested changes Feb 15, 2017

View reviewed changes

nguerrera added 4 commits February 14, 2017 17:11

Play nicely with AppendRuntimeIdentifierToOutputPath

0292419

Change platform target sooner

7d80c51

Add satellite + multi-targeting coverage to desktop default RID

11e55cb

Merge branch 'master' into remove-rid-requirement

d25fe87

natidea approved these changes Feb 15, 2017

View reviewed changes

dsplaisted approved these changes Feb 15, 2017

View reviewed changes

nguerrera mentioned this pull request Feb 15, 2017

Skip import of Microsoft.NuGet.props and Microsoft.NuGet.targets #870

Merged

nguerrera mentioned this pull request Feb 15, 2017

Different RID behavior for multitargeted restore in VS and command line #871

Closed

nguerrera force-pushed the remove-rid-requirement branch from 5046d92 to cc62950 Compare February 15, 2017 19:55

Make all innner build RIDs visible to restore

efbcaca

Fix dotnet#871

nguerrera force-pushed the remove-rid-requirement branch from cc62950 to efbcaca Compare February 15, 2017 19:57

srivatsn approved these changes Feb 15, 2017

View reviewed changes

dsplaisted approved these changes Feb 16, 2017

View reviewed changes

PR feedback: remove unnecessary condition

11ebca3

Add unit test for GetAllRuntimeIdentifiers

497b67c

nguerrera force-pushed the remove-rid-requirement branch from 2effe62 to 497b67c Compare February 16, 2017 08:03

PR feedback: improve multi-targeting coverage

19732f1

nguerrera merged commit 1042b68 into dotnet:master Feb 16, 2017

nguerrera deleted the remove-rid-requirement branch February 16, 2017 08:46

nguerrera mentioned this pull request Feb 16, 2017

Make AppendRuntimeIdentifierToPath=true the default #873

Merged

natemcmaster mentioned this pull request Feb 17, 2017

Package with csproj and build as AnyCPU aspnet/MvcPrecompilation#86

Closed

livarcocc mentioned this pull request Jan 31, 2020

Remove RuntimeIdentifier added only for full framework during migration #7733

Closed

mmitche pushed a commit to mmitche/sdk that referenced this pull request Jun 5, 2020

Update dependencies from https://github.com/dotnet/arcade build 20190…

5f33bc7

…805.9 (dotnet#847) - Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19405.9

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

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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

dsplaisted 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!

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!

dsplaisted commented Feb 9, 2017

Uh oh!

mlorbetske commented Feb 10, 2017

Uh oh!

livarcocc commented Feb 11, 2017

Uh oh!

nguerrera commented Feb 13, 2017

Uh oh!

nguerrera commented Feb 13, 2017

Uh oh!

srivatsn commented Feb 13, 2017

Uh oh!

nguerrera commented Feb 13, 2017

Uh oh!

emgarten commented Feb 13, 2017

Uh oh!

nguerrera commented Feb 14, 2017

Uh oh!

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

Uh oh!

Uh oh!

nguerrera commented Feb 14, 2017

Uh oh!

natidea left a comment

Choose a reason for hiding this comment

Uh oh!

nguerrera commented Feb 15, 2017

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!

srivatsn commented Feb 15, 2017

Uh oh!

nguerrera commented Feb 15, 2017

Uh oh!

nguerrera commented Feb 15, 2017

Uh oh!

nguerrera commented Feb 15, 2017

Uh oh!

nguerrera commented Feb 9, 2017 •

edited

Loading

nguerrera commented Feb 14, 2017 •

edited

Loading

nguerrera commented Feb 16, 2017 •

edited

Loading