dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.1k stars 4.7k forks source link

Proposal: Unlist and deprecate the System.Runtime.Loader NuGet package #38503

Open teo-tsirpanis opened 4 years ago

teo-tsirpanis commented 4 years ago

A leftover from the .NET Core 1.0 days, the System.Runtime.Loader NuGet package is especially dangerous (in addition to bloating the package dependency tree) because it "gives" the .NET Core-only AssemblyLoadContext class to .NET Standard libraries.

Directly using the package from an unsupported framework like the .NET Framework does not import any such type, but consider the (very realistic) scenario of having a .NET Framework or Xamarin app, importing a .NET Standard library and "using" AssemblyLoadContext. The app will crash, but most importantly, the illusion that AssemblyLoadContext is supported in .NET Standard is maintained at compile-time.

Even first-party libraries have fallen into the trap, like FSharp.Compiler.Service and there are other libraries that misuse the package even more by referencing it from .NET Core. And it has 55k downloads per day!

Because of this false compatibility with .NET Standard, I propose to unlist this package and deprecate it, suggesting the .NET Standard consumers to retarget to .NET Core and the .NET Core ones to simply drop it.

Of course if any other of these .NET Standard packages also falsely provides .NET Core-only features, it should get the same treatment.

ghost commented 4 years ago

Tagging subscribers to this area: @ViktorHofer Notify danmosemsft if you want to be subscribed.

ViktorHofer commented 4 years ago

cc @ericstj

ericstj commented 4 years ago

importing a .NET Standard library and "using" AssemblyLoadContext.

To be clear, this is a library that never tested it's behavior on that platform which is dubious. We reccomend that any netstandard library still test all the frameworks it supports.

We can consider deprecation of this package. I'm curious what deprecation does for transitive dependencies. Would we break packages that are only netcore and depend on this? //cc @nkolev92

I can't think of too many other packages in this boat, for most we built a version that throws PNSE when they aren't supported.

nkolev92 commented 4 years ago

Deprecation has no affect on transitive libraries. Currently the consumers get a warning in the Package Manager UI whenever they consume the package:https://docs.microsoft.com/en-us/nuget/nuget-org/deprecate-packages#client-experience-for-deprecated-packages

Smartisa commented 2 years ago

What method can be used to automatically know whether this package supports the actual installed platform?

teo-tsirpanis commented 2 years ago

@Smartisa you could use Type.GetType("System.Runtime.Loader.AssemblyLoadContext") != null, but that's not the best option since it uses reflection and is not compatible with AOT. If you target modern .NET, the answer to your question is always a "yes" and you shouldn't bother with this package at all.

@ericstj, is there something that prevents this issue from moving forward? It's been open for more than a year.

Smartisa commented 2 years ago

@Smartisa you could use Type.GetType("System.Runtime.Loader.AssemblyLoadContext") != null, but that's not the best option since it uses reflection and is not compatible with AOT. If you target modern .NET, the answer to your question is always a "yes" and you shouldn't bother with this package at all.

@ericstj, is there something that prevents this issue from moving forward? It's been open for more than a year.

Thanks for your answer. How can I automatically know whether any package is compatible with the actual installed platform?

teo-tsirpanis commented 2 years ago

In general, a NuGet package should work on all frameworks it targets. This package is an exception and you should avoid it using it in all cases. If you target modern .NET and you want to use AssemblyLoadContext, you don't need this package, the type is already there.

If you target .NET Standard, this package is lying to you, and I strongly recommend you to target to .NET 6.

Smartisa commented 2 years ago

In general, a NuGet package should work on all frameworks it targets. This package is an exception and you should avoid it using it in all cases. If you target modern .NET and you want to use AssemblyLoadContext, you don't need this package, the type is already there.

If you target .NET Standard, this package is lying to you, and I strongly recommend you to target to .NET 6.

We want to check the compatibility of any package with the actual installed platform, but we haven't found a practical method yet. Thanks for your help

ericstj commented 2 years ago

@ericstj is there something that prevents this issue from moving forward? It's been open for more than a year.

It's not getting traction because it's not a blocking issue and it's assigned to future. There's also uncertainty around the fallout of leveraging the deprecated feature for existing consumers.

It could be interesting a case for trying out deprecation. All of the frameworks that actually needed it are now out of support so the only consumers of it today are getting the false promise of this API's availability on .NETStandard. The risk here is that we somehow break a scenario for one of our consumers who didn't care about the lack of availability.

With respect to other questions about checking the compatibility of packages, https://docs.microsoft.com/en-us/dotnet/standard/analyzers/platform-compat-analyzer can help for newer packages that use it. That doesn't help this scenario though, since the package is older than that feature. That feature also doesn't represent framework gaps. Our recommendation has always been to test on the concrete frameworks you want to support.

This package is really just a relic of 1.0 where support was expressed differently (matching ref/lib). What this package did, at the time, would fail when restored on a framework that didn't resolve the lib, but later Nuget removed that failure and this package was left behind. We would never build this package this way today, but it exists and we haven't had any precedent for removing or deprecating old packages (yet).

@mkArtakMSFT is driving an un-listing effort in 7.0 -- perhaps we can deprecate this one as part of that effort. cc @agocke whose team owns the API that was defined in this package.