[WIP NO MERGE]: Add support for RuntimeFrameworkName to set the platform package for .NET Core 2.1 projects

[natemcmaster]

Part of dotnet/aspnetcore#3307

Changes:

• Introduce a new property, RuntimeFrameworkName, which defines the implicit package reference added when targeting .NET Core. The default is still 'Microsoft.NETCore.App'
• Add information about the default implicit version to use for ASP.NET Core metapackages to use (when selected as the runtime framework)
• Fail the build if an unrecognized value for RuntimeFrameworkName is specified in a .NET Core project.

---

Marking as "NO MERGE" because there is still ongoing discussion about when this should happen (if at all). I've opened this PR so we can take a look at the actual impact on the SDK when implementing this. We're meeting later this afternoon to take a closer look.

TODO: add automated tests to verify this works on aspnet core packages

cc @DamianEdwards @dsplaisted @nguerrera @Petermarcu @tmds @vijayrkn

[dsplaisted]

Isn't this supposed to be 2.1.1?

[JunTaoLuo]

@natemcmaster is this in anticipation for aspnet/Universe#1256?

[natemcmaster]

Bingo. We want to add a dependency from AspNetCore.App -> NETCore.App in 2.1.3. This implementation and aspnet/Universe#1256 go hand in hand. This is one of the tradeoffs we'll try to balance with @DamianEdwards. Doing so would mean users with the explicit PackageReference to AspNetCore.App would get downgrade warnings about NETCore.App. I think we will be okay with that since we want to help users move to the RuntimeFrameworkName approach, but we need to discuss.

[JunTaoLuo]

So does this mean there's no way for anyone to compose or use a different shared framework other than the ones we ship? I was under the impression that we want to support composable and custom shared frameworks at some point, maybe only for 3.0?

[nguerrera]

I'm in favor of locking this down until such a feature exists. It will be far easier to diagnose typos than generating a bogus packagereference with no version.

[natemcmaster]

This was more about users unintentionally attempting to use something as a shared framework package which isn't. Composable and custom frameworks is a 3.0 which I don't think we need to address in the 2.1 SDK.

[JunTaoLuo]

Ah good, I wasn't sure where that feature landed in terms of releases. Good to know it's only 3.0.

[nguerrera]

If we go with this change, I would wish this property didn't exist and there would only be RuntimeFrameworkName But that would be technically breaking.

[natemcmaster]

Yeah, I hope we can drop it in 3.0. I think we have to keep it for 2.1 because the source-build guys (cc @tmds) will need to toggle this separately from RuntimeFrameworkName to workaround dotnet/source-build#375

[nguerrera]

Does it make sense to ever use TFM version as AspNetCoreAllVersion?

[nguerrera]

Same applies to other lines for Latest instead of Default and AspNetCoreApp, but I won't comment everywhere.

[natemcmaster]

It will in 2.2 (assuming we don't botch that metapackage, too)

[nguerrera]

I think this is OK since it's the ultimate fallback.

[nguerrera]

I'm in favor of locking this down until such a feature exists. It will be far easier to diagnose typos than generating a bogus packagereference with no version.

[nguerrera]

I believe moving this out of .Common.targets to here will remove it from outer builds, which strictly speaking is a breaking change. I wouldn't normally be worried about something that is likely so obscure, but this is targeting a patch right now.

[natemcmaster]

Good to know, we should move it back then. Would there be any harm in hard coding this list to this?

<DefaultImplicitPackages>Microsoft.AspNetCore.All;Microsoft.AspNetCore.App;Microsoft.NETCore.App;NETStandard.Library</DefaultImplicitPackages>

[nguerrera]

TBH, I have no idea what this actually does. 🙃

Looking into it.

[nguerrera]

OK, it's the design time targets for the dependency node that consume this. Studying the code. If it's what I think the hard-coding above should be fine, but I'm still not 100% sure.

[nguerrera]

It seems to treat anything in this list as implicit but not look at IsImplicitlyDefined metadata. cc @abpiskunov

[natemcmaster]

Ok, I've moved this back to Microsoft.NET.Sdk.Common.targets with the aspnet list.

Btw, which branch should this PR change to? I don't see release/2.1.401 in this repo.

[nguerrera]

Ah, until now there was no dotnet/sdk changes for 2.1.401 (only dotnet/cli). it. Let's just leave it targeted at 2.1.4xx and if we decide to merge for 401, we can create the branch then.

[nguerrera]

The Publish=true is redundant now, right?

It feels like this should also be a conditional entry on the PackageReference. I'm having trouble following the rationale for Update in this case, but not for RuntimeFrameworkName != Microsoft.NETCore.App.

I suppose there could be a behavior change if user has explicit package ref and DisableImplict != true.

[natemcmaster]

The reason for this was that PrivateAssets="All" on NETCore.App only if OutputType == library, but for AspNetCore.App, PrivateAssets is always All (when using the implicit package version.)

I think the resolution to #2364 is to actually tweak this line so PrivateAsset="All" is always true unless PackageType== DotNetCliTool, but that's a separate change to make...didn't want to muddy the PR.

[nguerrera]

#2395 is out that I think would fix #2364. Since the pain is similar, I think there's a case to be made that we should pull in #2395 into the same release as this. It would end up making the code easier to follow. Something for us to consider, but not feedback for your change. cc @wli3

[natemcmaster]

Cool, glad that's going in to 2.2.

Also, yes, the Publish="true" is redundant. I can remove that line.

[nguerrera]

@abpiskunov @davkean I'm trying to understand the consequences of changing this.

This gets passed here:

sdk/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.PackageDependencyResolution.targets

Line 269 in d6ad245

DefaultImplicitPackages="$(DefaultImplicitPackages)"

And turned into a HashSet here:

sdk/src/Tasks/Microsoft.NET.Build.Tasks/PreprocessPackageDependenciesDesignTime.cs

Line 78 in f5db1af

ImplicitPackageReferences = GetImplicitPackageReferences(DefaultImplicitPackages);

And marks things as ImplicitlyDefined based on HashSet alone here:

sdk/src/Tasks/Microsoft.NET.Build.Tasks/PreprocessPackageDependenciesDesignTime.cs

Line 171 in f5db1af

dependency.IsImplicitlyDefined = ImplicitPackageReferences.Contains(dependency.Name);

What does dependency.IsImplicitlyDefined control in the Dependencies node? Does it impact anything else in the IDE?

It looks like this will mark explicit user package references to these packages as implicit, which feels wrong. Is that right?

[nguerrera]

At this point, my concerns are down to this (irrespective of which release this lands in):

1. Figure out the DefaultImplicitPackages consequences and if they're OK.
2. Make sure we agree on the metapackage chaining instead of having the SDK pick both metapackages. I still think I'm convinced that chaining is right, but @dsplaisted expressed doubts before our meeting just now and we didn't talk through them.

[davkean]

IsImplicitlyDefined causes it to get a different icon and you can cannot remove or attempt to remove the item.

[nguerrera]

IsImplicitlyDefined causes it to get a different icon and you can cannot remove it.

I think the (existing) code is wrong then. It should be based on IsImplicitlyDefined metadata on the package reference. NuGet uses that AFAIK in package manager. I'm going to play around with this and file a bug if I'm right.

[davkean]

I don't have context on above, but it respects it for all reference items: https://github.com/dotnet/project-system/search?q=IsImplicitlyDefined&unscoped_q=IsImplicitlyDefined.

[natemcmaster]

Figure out the DefaultImplicitPackages consequences and if they're OK.

I'm also not sure what this does. I know the IsImplicitlyDefined item metadata already works in VS, regardless of the value of DefaultImplicitPackages, so maybe let's just not touch the thing we don't understand, eh?

Make sure we agree on the metapackage chaining instead of having the SDK pick both metapackages.

Yes, I think this is a really thing to make sure we are clear on. The only reason we didn't metapackage chain in the first place was that we assumed there would always be two PackageReference items and that they would be kept in alignment through the implicit package version. But since many users are not actually using the implicit package version, they can put themselves into a place where their app will pull a standalone version of NETCore.App which is lower than what AspNetCore.App was compiled for. Although I hope this is rare, it can lead to runtime errors because of assembly version mismatch. I think the solution - one we should have done sooner - is to make sure Microsoft.AspNetCore.App declares in the package metadata its minimum NETCore.App version.

The tricky bit here is timing. If we decide to hold off on this until 2.1.500, then we can't do sharedfx package chaining. Adding package chaining without this SDK feature would mean users cannot actually reference the AspNetCore.App 2.1.3 package without disabling some package downgrade warnings...(unless we take a totally different SDK change that looks at AspNetCore.App package version and modifies the implicit package version of Microsoft.NETCore.App to match it.)

[nguerrera]

Although I hope this is rare, it can lead to runtime errors because of assembly version mismatch.

Microsoft.NETCore.App assembly versions change between patches? cc @steveharter

[natemcmaster]

Microsoft.NETCore.App assembly versions change between patches?

I don't think they do, but I don't want to rely on that always being true. Point is, running on a version of NETCore.App lower than what AspNetCore.App expects is never a good idea, and I believe our packages should codify this.

[nguerrera]

Re DefaultImplicitPackages: Here's what I see in VS:

1. With IsImplicitlyDefined=true metadata on PackageReference, but package name not in DefaultImplicitPackages:

• Package shows up under Dependencies -> NuGet AND Dependencies -> SDK
• You can press delete on the node in NuGet to remove the reference
• You cannot uninstall it in NuGet manager
• This is how Microsoft.ApsNetCore.App behaves with current templates and current web sdk handling

2. With IsImplicitlyDefined=true AND package name in DefaultImplicitPackages

• Package shows up only under Dependencies -> SDK

I think (2) is actually preferable so maybe the revert was premature, but it is beyond bizarre that the behavior is controlled by a hard-coded list of packages and not driven by metadata on the package ref.

[nguerrera]

Agree about version mismatch between dependent shared frameworks, I was mostly just curious if the assembly versions actually change.

[natemcmaster]

What happens when IsImplicitlyDefined=’false’ or emtpy AND package name is in DefaultImplicitPackages?

[nguerrera]

What happens when IsImplicitlyDefined=’false’ or emtpy AND package name is in DefaultImplicitPackages?

It behaves the same in Dependencies node as (2) above. Only under SDK, not NuGet node, and you can't delete it.

But it can now be uninstalled or updated in the NuGet UI.

This is just weird.

[davkean]

@abpiskunov Probably has context on how this stuff is supposed to work.

[natemcmaster]

One more thing to think about, and maybe we should address this in this PR.

In this PRs current design, we would need to change dotnet/aspnetcore#3307 at the same time. For users with an explicit PackageReference who attempt to update to AspNetCore.App 2.1.3, they would see this:

WebApp12.csproj : error NU1605: Detected package downgrade: Microsoft.NETCore.App from 2.1.3 to 2.1.0. Reference the package directly from the project to select a different version.
WebApp12.csproj : error NU1605:  WebApp12-> Microsoft.AspNetCore.App 2.1.3 -> Microsoft.NETCore.App (>= 2.1.3)
WebApp12.csproj : error NU1605:  WebApp12-> Microsoft.NETCore.App (>= 2.1.0)

I think we should also add a check that runs on CollectPackageReferences which looks for Microsoft.AspNetCore.App and Microsoft.AspNetCore.All as PackageReferences and can issue a warning or error like this:

warning: A PackageReference for 'Microsoft.AspNetCore.App' was included in your project. This will cause issues when upgrading to Microsoft.AspNetCore.App 2.1.3 and higher.
To upgrade to 2.1.3 or higher, remove this PackageReference and add the RuntimeFrameworkName property to $(MSBuildProjectFileName).

 <PropertyGroup>
    <RuntimeFrameworkName>Microsoft.AspNetCore.App</RuntimeFrameworkName>
 </PropertyGroup>

For more information, see https://aka.ms/sdkimplicitrefs

Thoughts @dsplaisted @nguerrera @DamianEdwards ?

[nguerrera]

I think that warning would make sense. We may not be able to get it localized if we do ship in patch.

[natemcmaster]

I've update the PR to issue a warning when there are explicit PackageReferences to AspNetCore.App/All.

I've also begun adding automated tests to verify the behavior we want. To make sure this works as expected, I'm using in tests a build of our metapackage which would align with the results of this change: aspnet/Universe#1261

[natemcmaster]

Updated the whitespace in the warning to be more VS friendly.

I'm thinking we should drop the code snippet from the warning. It doesn't render well anyways.

[nguerrera]

Agree on dropping the code snippet. We should just make sure the aka.ms link points to good docs on the subject.

[nguerrera]

We can drop the CDATA now, right?

[natemcmaster]

Yes. Removed that along with the code snippet.

[dsplaisted]

If we add this warning, we will need to update the https://aka.ms/sdkimplicitrefs link, or create a new one. The current content there doesn't directly apply to ASP.NET.

[natemcmaster]

Windows test failures don't appear to be related to my changes. They are complaining that the agent doesn't have .NET Framework 4.7.2 ref assemblies.

D:\\j\\workspace\\debug_windows---99f365cb\\.dotnet\\sdk\\2.1.400-preview-009090\\Microsoft.Common.CurrentVersion.targets(1179,5): error MSB3644: The reference assemblies for framework \".NETFramework,Version=v4.7.2\" were not found. To resolve this, install the SDK or Targeting Pack for this framework version or retarget your application to a version of the framework for which you have the SDK or Targeting Pack installed. Note that assemblies will be resolved from the Global Assembly Cache (GAC) and will be used in place of reference assemblies. Therefore your assembly may not be correctly targeted for the framework you intend. [D:\\j\\workspace\\debug_windows---99f365cb\\artifacts\\Debug\\tmp\\DesktopInBoxC---8981AF1F\\DesktopInBoxConflictResolution\\DesktopInBoxConflictResolution.csproj]se

I don't understand what would have caused the Ubuntu test failures. Are these known issues in 2.1.4xx?

• https://ci.dot.net/job/dotnet_sdk/job/release_2.1.4xx/job/release_ubuntu14.04_prtest/177/testReport/junit/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildANetCoreAppAndPassingALogger/It_collects_TargetFramework_version/
• https://ci.dot.net/job/dotnet_sdk/job/release_2.1.4xx/job/release_ubuntu14.04_prtest/177/testReport/junit/Microsoft.NET.Build.Tests/GivenThatWeWantToVerifyNuGetReferenceCompat/Nuget_reference_compat_referencerTarget____netcoreapp1_1____testDescription____Full____rawDependencyTargets____netstandard1_0_netstandard1_1_netstandard1_2_netst_______restoreSucceeds__True__buildSucceeds__True_/

[natemcmaster]

If we add this warning, we will need to update the aka.ms/sdkimplicitrefs link, or create a new one.

Agreed. Opened dotnet/docs#6482

[dsplaisted]

The .NET 4.7.2 reference assembly failures should be fixed by #2401. You can also try re-running those legs, it seems the agents aren't consistent on what is installed.

[dsplaisted]

oops, misclicked on close

[natemcmaster]

Per conversation in person, we are no longer pursuing this fix in 2.1.4xx.