Azure / azure-sdk-for-net

This repository is for active development of the Azure SDK for .NET. For consumers of the SDK we recommend visiting our public developer docs at https://learn.microsoft.com/dotnet/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-net.
MIT License
5.25k stars 4.59k forks source link

Target netstandard2.0 and net461 #32597

Open heaths opened 1 year ago

heaths commented 1 year ago

We need to target both netstandard2.0 and net461 so that .NET Framework (net461+) apps don't pull down a bunch of shim assemblies they don't need.

/cc @schaabs

heaths commented 1 year ago

FWIW, the guidelines also call this out: https://learn.microsoft.com/en-us/dotnet/standard/library-guidance/cross-platform-targeting#:~:text=CONSIDER%20adding%20a%20target%20for%20net461%20when%20you%27re%20offering%20a%20netstandard2.0%20target.

heaths commented 1 year ago

This does create a bit of a problem: testing with Key Vault - which the solution also contains Core and Identity - a number of problems were identified:

  1. ECDsa is supported on net47 and newer, but defined in netstandard2.0. Using reflection-based proxies and no-inlining hints, I was able to have a single code base for KV. But by targeting net461 explicitly now, various types like ECParameter, ECCurve, etc., don't exist. That said, this does present a good opportunity to eliminate reflection and use multi-targeting, but should probably target net47 as well.
  2. Identity takes advantage of spans, which aren't defined for net461 without pulling down netstandard2.0 assemblies. Doing so means the code works (at least for RSA; see ECDsa problems above) but no longer would if we multi-target net461 without pulling down necessary netstandard2.0 assemblies. Maye we just do for span to keep thinks from regressing?

/cc @jsquire @schaabs

heaths commented 1 year ago

Talking with @ericstj, once a .NET Framework TFM is targeted (in the nupkg), netstandard* will never be used. Thus, for cryptpgraphy types related to ECDsa and others found in https://github.com/dotnet/standard/blob/release/2.0.0/netstandard/src/ApiCompatBaseline.net461.txt, we might want to consider multi-targeting both net461 + net47 (or whatever is needed; net47 adds ECDsa support).

As for spans, we could target net461 but still reference System.Memory, which might still offer better performance and maintainability than #ifdef'ing to use a string, char[], or whatever we need.

heaths commented 1 year ago

This tool may also be of use: https://nugettools.azurewebsites.net/6.0.0/framework-precedence

Romfos commented 1 year ago

What about .NET 6 as tfm? Would be good to support Nullable Reference Types for modern dotnet versions

heaths commented 1 year ago

We don't need to support net6.0 TFM to use nullable types. The nullable attributes need only be defined as the same namespace and type sans the assembly strong name. We do this in Azure.Core. However, adding nullable type annotations is a breaking change which is why we haven't done it. That is, if we add them now, a lot of code that was already compiling fine would break. This is the reason we haven't added them.