[Core] Remove netcoreapp2.1 and net5.0 targets

[m-redding]

• [Core] Remove netcoreapp2.1 target #39424

[azure-sdk]

API change check

APIView has identified API level changes in this PR and created following API reviews.

Azure.Core
Azure.Core.Expressions.DataFactory

[jsquire]

YAY!

Aren't there some #if NETCOREAPP2_1 checks in Azure Core that we can remove with the target going away? I'm thinking around diagnostics and for AOT?

[christothes]

Aren't there some #if NETCOREAPP2_1 checks in Azure Core that we can remove with the target going away? I'm thinking around diagnostics and for AOT?

yeah - https://grep.app/search?q=NETCOREAPP2_1&filter[repo][0]=Azure/azure-sdk-for-net

[m-redding]

Oh yes I forgot about those - thank you, I will add them right now @christothes @jsquire

[heaths]

Should we consider - at least for a time - to keep net5.0 targets in our tests, if we had them to begin with? I appreciate we don't support those TFMs - unsupported means unsupported - but we also said we wouldn't intentionally break customers. If even just a single commit to sanity check across platforms (I know KV has some platform-dependent features) and then revert the commit in this PR, that would be something.

[m-redding]

@heaths It looks like the test targets are only net462, net6.0, and net7.0, when you say

even just a single commit to sanity check across platforms

do you mean add a net5.0 and netcoreapp2.1 target to the test .csproj to run the pipelines once? Sorry I don't understand exactly what you mean there

[heaths]

@heaths It looks like the test targets are only net462, net6.0, and net7.0, when you say

even just a single commit to sanity check across platforms

do you mean add a net5.0 and netcoreapp2.1 target to the test .csproj to run the pipelines once? Sorry I don't understand exactly what you mean there

That's correct. Even if we just test it as part of the PR then revert that comment. That way, there's no surprises with existing code.

[jsquire]

do you mean add a net5.0 and netcoreapp2.1 target to the test .csproj to run the pipelines once? Sorry I don't understand exactly what you mean there
That's correct. Even if we just test it as part of the PR then revert that comment. That way, there's no surprises with existing code.

I don't think it's that easy. You'd have to also alter the eng sys configuration around the text platform maatrix and probably add something to manually install the unsupported frameworks - if we are even able to do so.

I'm not opposed to doing a test - but I don't feel strongly that we need to do so. At best, I'd expect it would help us add some notes to the change log about what may no longer work. If we decide to test, it may be easier to do locally rather than through the pipelines.

[annelo-msft]

Please add an entry to the CHANGELOG so it can be checked in as part of this PR. Many thanks!

[heaths]

@jsquire said,

If we decide to test, it may be easier to do locally rather than through the pipelines.

Yeah, that's a good point. You likely already have the target runtimes installed and, if not, you can quickly get them from https://dot.net.

[lmolkova]

YAY!!!

After it's merged, would it be possible to generate a nightly nuget so I can test if it still works with AppInsights?

[m-redding]

@heaths @jsquire I added the targets to Azure.Core.Tests.csproj locally to try and run the test suite. I got the following error when building:

C:...Azure.Core.Tests.csproj : error NU1701: Warning As Error: Package 'NUnit3TestAdapter 4.4.2' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8, .NETFramework,Version=v4.8.1' instead of the project target framework '.NETCoreApp,Version=v2.1'. This package may not be fully compatible with your project.

I hid that one and then got the following:

C:\Users\mredding.nuget\packages\system.runtime.compilerservices.unsafe\6.0.0\buildTransitive\netcoreapp2.0\System.Runtime.CompilerServices.Unsafe.targets(4,5): error : System.Runtime.CompilerServices.Unsafe doesn't support netcoreapp2.1. Consider updating your TargetFramework to netcoreapp3.1 or later. [C:\Users\mredding\source\repos\azure-sdk-for-net\sdk\core\Azure.Core\tests\Azure.Core.Tests.csproj::TargetFramework=netcoreapp2.1]

Which I'm 90% sure is coming from having removed:

  <!-- Keep System.Diagnostics.DiagnosticSource 4.6.0 for netcoreapp2.1 support. -->
  <ItemGroup Condition=" '$(TargetFramework)' == 'netcoreapp2.1' ">
    <PackageReference Include="System.Diagnostics.DiagnosticSource" VersionOverride="4.6.0" />
  </ItemGroup>

Because that was the error that was causing me to need to keep that additional version in the first place. I think this means that with this change we would be breaking customers using netcoreapp2.1.

I removed the netcoreapp2.1 target and ran the Azure.Core.Tests for net5.0, which had three failures:

This one for both async/sync, with the failure: System.InvalidCastException : Unable to cast object of type 'System.Net.Http.HttpClientHandler' to type 'System.Net.Http.SocketsHttpHandler'.

#if NETCOREAPP
        [Test]
        public void UsesSocketsClientTransportOnNetCoreApp()
        {
            var transport = (HttpClientTransport)GetTransport();
            HttpClient transportClient = transport.Client;
            var handler = (SocketsHttpHandler)GetHandler(transportClient);
            Assert.AreEqual(TimeSpan.FromSeconds(300), handler.PooledConnectionLifetime);
            Assert.GreaterOrEqual(handler.MaxConnectionsPerServer, 50);
        }
#else
        [Test]
        public void UsesHttpClientTransportOnNetStandart()
        {
            var transport = (HttpClientTransport)GetTransport();
            HttpClient transportClient = transport.Client;
            var handler = (HttpClientHandler)GetHandler(transportClient);
            Assert.AreEqual(50, handler.MaxConnectionsPerServer);
        }
#endif

And this one with Expected: True But was: False

        public void CreateOperationCanceledExceptionPassesTokenOnSupportedVersions()
        {
            using var cts = new CancellationTokenSource();
            cts.Cancel();
            var ex = (TaskCanceledException) CancellationHelper.CreateOperationCanceledException(new Exception("test"), cts.Token);

#if NETCOREAPP2_1_OR_GREATER
            Assert.IsTrue(ex.CancellationToken.IsCancellationRequested);
#else
            Assert.IsFalse(ex.CancellationToken.IsCancellationRequested);
#endif

I think both of these tests seem to be a case of the test just not picking up on the correct target, since these tests haven't been run with a net5.0 target in... awhile? It doesn't seem like the behavior is incorrect since the outcome fits the else cases.

Do either of you have thoughts about if this impacts the work in this PR?

[jsquire]

I think this means that with this change we would be breaking customers using netcoreapp2.1.

I've no issue with this. netcoreapp2.1 reached end-of-life in August, 202 (src). It's reasonable to have this break.

And this one with Expected: True But was: False

        public void CreateOperationCanceledExceptionPassesTokenOnSupportedVersions()
        {
            using var cts = new CancellationTokenSource();
            cts.Cancel();
            var ex = (TaskCanceledException) CancellationHelper.CreateOperationCanceledException(new Exception("test"), cts.Token);

#if NETCOREAPP2_1_OR_GREATER
            Assert.IsTrue(ex.CancellationToken.IsCancellationRequested);
#else
            Assert.IsFalse(ex.CancellationToken.IsCancellationRequested);
#endif

Not entirely sure what that test is doing, but the name of "OnSupportedPlatforms" makes me think we should change the #if to screen for net 6 and above.

This one for both async/sync, with the failure: System.InvalidCastException : Unable to cast object of type 'System.Net.Http.HttpClientHandler' to type 'System.Net.Http.SocketsHttpHandler'.

#if NETCOREAPP

This one seems to be related the fallback to the net461 target. We should probably make that #if check for net 6 or higher.

Do either of you have thoughts about if this impacts the work in this PR?

[m-redding]

@jsquire I'm not really sure what the test is doing either, but it doesn't seem to indicate a behavioral change or change in support to me. I think since the change doesn't impact the current test targets, I think I'll leave those preprocessor targets for now. I'll update the changelog to more clearly indicate that netcoreapp2.1 isn't supported anymore.