Target Microsoft.ApplicationInsights.AspNetCore to netcoreapp3.1 and remove package references to deprecated nugets Microsoft.AspNetCore.* #2860
Conversation
Remove package references to nugets Microsoft.AspNetCore.* because they are deprecated. For AspNetCore -framework reference to Microsoft.AspNetCore.App is added. microsoft#2811
…on #if NET452 with #if NETFRAMEWORK The condition is not specific to NET452, but generic to all NETFRAMEWORKs.
…et462 This is required, because WorkerService depends on TelemetryChannel, which in its turn targets net452;netstandard2.0 and has conditional compilation on net framework.
|
@cijothomas @TimothyMothra, could you please take a look at these changes and say if they are OK with your considerations about still supporting legacy frameworks? |
| @@ -3,9 +3,9 @@ | |||
|
|
|||
| <PropertyGroup> | |||
| <AssemblyName>Microsoft.ApplicationInsights.AspNetCore</AssemblyName> | |||
| <TargetFrameworks>netstandard2.0</TargetFrameworks> | |||
| <TargetFrameworks>netcoreapp3.1</TargetFrameworks> | |||
There was a problem hiding this comment.
won't removing netstandard2.0 lose support for asp.net core 2.1 a on .NET framework?
https://dotnet.microsoft.com/en-us/platform/support/policy/dotnet-core#servicing
or is that already unsupported?
There was a problem hiding this comment.
ASP.NET Core 2.1 is the de facto version that is already deprecated and no longer supported.
Removing dependency to deprecated ASP.NET Core 2.1 nuget packages is the goal of this PR.
There was a problem hiding this comment.
its a bit more complex... ASP.NET Core 2.1 is deprecated, however, ASP.NET Core 2.1 on .NET Framework is not...
I believe this is still a supported version in this repo, so won't be possible to remove it.
Also, given almost all of new investments are occurring only in OpenTelemetry, it may not be desired to spend much time on this repo. Of course, if there are security issues, it should be promptly addressed.
There was a problem hiding this comment.
You are correct. ASP.NET Core 2.1.x is supported on .NET Framework.
Given that MS split the support for ASP.NET Core on .NET Core and .NET Framework, maybe a better approach is to also split the targets of AI.AspNetCore - for net462 and netcoreapp3.1:
- For net462, AI.AspNetCore will use nuget packages for ASP.NET Core 2.1.x
- For netcoreapp3.1, AI.AspNetCore will use framework reference to Microsoft.AspNetCore.App.
This way AI.AspNetCore will continue to support ASP.NET Core 2.1.x on .NET Framework through the nuget package.
How does this sound to you?
There was a problem hiding this comment.
That might be correct. Unfortunately, I do not have enough cycles to think through all the implications of that.
@TimothyMothra can make a final call if we want to proceed with this.
@antymon4o Apologies in advance, if we chose to not proceed with this. Almost all investments are occurring in OpenTelemetry based solutions, given it is the future for instrumentation.
There was a problem hiding this comment.
@cijothomas thank you for you comments!
I understand that the focus and the future is in the OpenTelemetry.
But since the changes in this PR are only on targeted frameworks and nuget packages, and there is no behavior change, I can not understand what resources or investments are required to review and complete this PR. The work is completed. It is not needed for someone else to do/code something more.
Is it possible at least for the PR checks to be run just to see if the builds and the test are passing on GitHub too?
| @@ -3,7 +3,7 @@ | |||
|
|
|||
| <PropertyGroup> | |||
| <AssemblyName>Microsoft.ApplicationInsights.WorkerService</AssemblyName> | |||
| <TargetFrameworks>netstandard2.0</TargetFrameworks> | |||
| <TargetFrameworks>net462;netstandard2.0</TargetFrameworks> | |||
There was a problem hiding this comment.
It seems that there isn't, but after the changes to AI.AspNetCore project some tests for AI.WorkerService.Tests for framework 4.* failed.
There isn't direct dependency between AI.AspNetCore and AI.WorkerService, But...
There was runtime error when executing the tests with this exception:
System.IO.FileLoadException: 'Could not load file or assembly 'System.Security.Principal.Windows, Version=4.1.1.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' or one of its dependencies. The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x80131040)' at Microsoft.ApplicationInsights.WindowsServer.TelemetryChannel.Shared.Implementation.WindowsIdentityProvider.Microsoft.ApplicationInsights.WindowsServer.TelemetryChannel.Shared.Implementation.IIdentityProvider.GetName() in X:\github\ApplicationInsights-dotnet\BASE\src\ServerTelemetryChannel\Managed\Shared\Implementation\WindowsIdentityProvider.cs:line 15 at Microsoft.ApplicationInsights.WindowsServer.TelemetryChannel.Implementation.ApplicationFolderProvider.CreateTelemetrySubdirectory(DirectoryInfo root) in X:\github\ApplicationInsights-dotnet\BASE\src\ServerTelemetryChannel\Implementation\ApplicationFolderProvider.cs:line 271
To fix the tests, adding net462 to AI.WorkerService was required.
And it seems logical to have this specific target for AI.WorkerService, because AI.WorkerService depends on TelemetryChannel which has two target frameworks net452, netstandard2.0 and has conditional compilation statements dependent on NET452/NETFRAMEWORK.
I cannot give an explanation why the tests were passing before. Maybe it was coincidence because this assembly System.Security.Principal.Windows was copied as a transitive dependency of Microsoft.AspNetCore.* in test output folder without directly depending, even though the code required it.
…of IWebHostEnvironment
When targerting net462, add nuget package references to Microsoft.AspNetCore.* 2.1.x, otherwise use framework reference to Microsoft.AspNetCore.App Add public api files for net462
Fix Issue #2811 .
Changes
Checklist