search

NuGet / Home

Repo for NuGet Client issues
Other
1.5k stars 252 forks source link

Treat TargetFramework(s) values as aliases #5154

Open emgarten opened 7 years ago

emgarten commented 7 years ago

NuGet should treat the values from TargetFramework and TargetFrameworks as aliases.

When setting the property TargetFramework=(alias) the actual target framework value can be evaluated from TargetFrameworkIdentifier and TargetFrameworkVersion.

See for examples and more details: https://github.com/dotnet/project-system/issues/1938

Update by @nkolev92

NuGet in 5.8 and .NET 5.0 SDK now treats TargetFramework as an alias, however duplicate framework entries are not supported yet (ex. You can't target .NETCoreApp,Version=3.0 twice.)

davkean commented 7 years ago

Yes, see this guideline I just wrote: https://github.com/dotnet/project-system/blob/master/docs/repo/coding-conventions.md#data

natidea commented 7 years ago

RE: https://github.com/dotnet/project-system/issues/615 Nominate currently passes both TF and TFM. TF is the key used to identify each TargetFrameworkInfo object (though the api calls it "TargetFrameworkMoniker"), and TFM is stored in the Properties bag. I'll close https://github.com/dotnet/project-system/issues/615 since we are already passing all the info NuGet needs to make this switch.

nkolev92 commented 4 years ago

@rrelyea we should discuss this and tackle it for .NET 5.0.

zkat commented 4 years ago

Moving under the .NET 5 epic (#9090), as this is probably something we want to look into prioritizing for that. It'll require a lot of partner work/sync. Most likely the next step for this is to collect some partner/user stories to get a sense of the impact the lack of this feature is having, so we can better prioritize it in our roadmap. (so if you're reading it, feel free to tell us how this impacts you!)

nkolev92 commented 4 years ago

As me, @zkat, @zivkan and others discussed during the sync-up, I think this is larger than a week's work and thus not quality week appropriate.

Affected areas:

cc @anangaur

nkolev92 commented 4 years ago

Another affected component is the solution explorer dropdown menu.

See: https://github.com/NuGet/NuGet.Client/pull/3392#discussion_r430934193

ViktorHofer commented 4 years ago

so if you're reading it, feel free to tell us how this impacts you!

In dotnet/runtime we overload the TargetFramework and TargetFrameworks properties in our libraries to allow multiplexing not just on the TargetFramework but on other properties like the runtime (OS part) or the architecture as well.

Example: https://github.com/dotnet/runtime/blob/6347980e2f5490f4c3a3effd2417912f8ce248bf/src/libraries/System.Runtime/src/System.Runtime.csproj#L5

Note that us overloading the TargetFramework property with an OS portion isn't related to the "net5.0-Platform" feature. NuGet should not read or interpret these suffixes and ignore them during restore. This currently causes restore operations inside Visual Studio to fail for projects like the one linked above and there is no known way to work around that. For restore operations on the CLI, we strip the suffixes away: https://github.com/dotnet/arcade/blob/9811f06184cd2adae554f013ece07bece2a6c50e/src/Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk/src/build/Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk.targets#L12-L14.

It would really help us if we could get a rough ETA of a) when this "feature" will be implemented and b) in which Visual Studio version it will be available.

cc @ericstj @Anipik

zkat commented 4 years ago

We might be able to get away in net5.0 with just doing https://github.com/nuget/home/issues/9756, which isn't this whole feature, but it makes some necessary work happen towards making aliases happen.

ViktorHofer commented 4 years ago

@zkat can you please provide more details what #9756 covers and more interestingly what it doesn't cover? thanks

ViktorHofer commented 4 years ago

ping @zkat @nkolev92

nkolev92 commented 4 years ago

iirc, allowing duplicate frameworks will be the thing that #9756 won't cover.

That requires us to change the lock file & assets file, and lots of internal logic that'd be a big scope creep to try to get finish in 16.8.

zivkan commented 4 years ago

@ViktorHofer I believe that the intent/minimum requirements for #9756 is to support the .NET SDK passing default TargetPlatformVersion when the TargetFramework omits it. That's it. For example, the project's TargetFramework is net5.0-windows, but NuGet doesn't know what version of windows it is. The .NET SDK creates the TargetPlatformVersion, which Nuget has to read, so we know it means net5.0-windows10.0.1234 or whatever, instead of parsing the TargetFramework as we do today. We're going to read TFI, TFV, TPI and TPV (and maybe TargetProfile to support projects still using that?) from MSBuild, rather than just the TargetFramework and TPV. But it won't be full alias support.

This means everything else is out of scope of that issue, such as:

This issue (#5154) is more of an epic to get all these features working.

Note that pack will probably never support projects where the canonical TFM is not unique, because we need to use the "official" short folder name in the lib/, ref/ and other folders. We can't use the aliases while also supporting backwards compatibility for old NuGet clients.

nkolev92 commented 4 years ago

Given that now NuGet treats frameworks as aliases, I propose we close this and open a tracking issue for allowing duplicate frameworks?

Thoughts @zkat @JonDouglas @aortiz-msft

JonDouglas commented 4 years ago

I read through the entire thread & relevant issues. I don't personally understand the value of having duplicated framework entries. I think creating an issue to track that is worthwhile as I'm not necessarily sure what scenarios that covers & what value that brings to support that.

nkolev92 commented 4 years ago

I don't personally understand the value of having duplicated framework entries

It's definitely not a common scenario. A few asks we've come across.

zivkan commented 4 years ago

Another scenario where it could be useful is wanting to test the behavior of different versions of the same package on the same framework. This could be a big help for integration tests.

Also benchmarks.

ViktorHofer commented 4 years ago

The runtime repo would be the biggest benefactor to a feature like that, given that they have to build for many different platforms. @ViktorHofer might be able to speak on that in more detail, but there's lots about the runtime repo that's custom and not super relevant for the average user.

AFAIK it's not just us. I believe @clairernovotny, partner teams and community members have tried something similar in the past. NuGet might not even be the reason for multi-targeting (with an identical pivot).

ViktorHofer commented 4 years ago

The runtime repo would be the biggest benefactor to a feature like that, given that they have to build for many different platforms. @ViktorHofer might be able to speak on that in more detail, but there's lots about the runtime repo that's custom and not super relevant for the average user.

@nkolev92 after discussing this with @JonDouglas offline I realized that to unblock us in the short-term it might be enough to allow duplicates and distinct them so that the de-duplicated TFM/TPM tuples are restored. We currently don't use different PackageReferences when multi-targeting for the same TFM/TPM tuple (ie net5.0-browser, net5.0-linux). I believe that with static graph restore enabled (which we depend on), duplicates are already allowed today (cc @jeffkl) and only VS complains.

Just to be clear, we would still want the full feature so that we can have targetframeworks with identical TFM/TPM tuples that target different packages / package versions / etc. But the highest priority for us right now is to fix the broken VS experience as reported numerous times in our repo, i.e.: https://github.com/dotnet/runtime/issues/20411#issuecomment-697142018.

zivkan commented 4 years ago

@ViktorHofer We already support restoring multiple net5.0 TFMs, as long as the canonical TFM is unique. For .NET 5, the canonical TFM is the combination of TargetFrameworkMoniker and TargetPlatformMoniker. For example, this restores and builds correctly:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFrameworks>net5.0;net5.0-windows</TargetFrameworks>
  </PropertyGroup>

</Project>

The issue is that for platforms identifiers that the SDK doesn't know about, such as net5.0-linux, the SDK doesn't populate the TargetPlatformMoniker property, so the canonical TFM is identical to net5.0 (no platform). I can get restore to work by doing this:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFrameworks>net5.0;net5.0-windows;net5.0-linux</TargetFrameworks>
  </PropertyGroup>

  <PropertyGroup Condition=" '$(TargetFramework)' == 'net5.0-linux' ">
    <TargetPlatformMoniker>linux,Version=0.0</TargetPlatformMoniker>
  </PropertyGroup>

</Project>

But then the SDK fails to build the project because linux is not a supported platform. I don't know how dotnet/runtime creates packages, but if it's using NuGet's msbuild pack task, then even if you use some trick to get the build to work, having this fake net5.0-linux TFM in the assets file will cause assemblies to be packed in the nupkg. But even once TFM alias with duplicates support is added, we still won't support packing these projects. Packing will always require unique canonical TFMs.

The assets file and the classes that represent the in-memory model of it, currently uses the canonical TFM as the key in several JSON objects. It will require a breaking change in our APIs, and probably a breaking change to the assets file json schema as well. So, I can't imagine a quick fix. I think the full effort of adding support for duplicate canonical TFMs is needed.

ViktorHofer commented 4 years ago

I don't know how dotnet/runtime creates packages, but if it's using NuGet's msbuild pack task, then even if you use some trick to get the build to work, having this fake net5.0-linux TFM in the assets file will cause assemblies to be packed in the nupkg.

We don't use nuget's pack task, we use our custom packaging library which calls into NuGet's apis: https://github.com/dotnet/arcade/tree/master/src/Microsoft.DotNet.Build.Tasks.Packaging/src.

But then the SDK fails to build the project because linux is not a supported platform.

Can you point me to that logic? Is it possible to teach the SDK how to handle the custom TPM?

We already support restoring multiple net5.0 TFMs, as long as the canonical TFM is unique.

Right. I was curious if we could lift that restriction during restore by ignoring duplicates until the full feature is implemented?

nkolev92 commented 4 years ago

Can you point me to that logic? Is it possible to teach the SDK how to handle the custom TPM?

@dsplaisted will know that.

Right. I was curious if we could lift that restriction during restore by ignoring duplicates until the full feature is implemented?

That's hard and risky to lift. If we remove that, P2P, lock file generation & pack will fail/behave weird.
Now you might say you're not using those, but that's just the obvious failures :) Lots of things in NuGet assume framework uniqueness, and look-up things based on that assumption.

Allowing duplicates is the full implementation unfortunately. It's more about managing the corner cases so that they fail with a controlled NuGet rather than a system error. :)