Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove target frameworks net452;net46; because they are no longer sup… #2854

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

antymon4o
Copy link

…ported #2850

use frameworkreference for Microsoft.AspNetCore.App instead of icrosoft.AspNetCore.* nuget packages, because they are now deprecated and end of life.

Fix Issue #2850.

Changes

Remove target frameworks net452;net46; and netcoreapp3.1; because they are no longer supported.
Affected projects are: DependencyCollector, TelemetryChannel, Perf, WindowsServer, Microsoft.ApplicationInsights.
Use frameworkreference for Microsoft.AspNetCore.App instead of icrosoft.AspNetCore.* nuget packages, because they are now deprecated and end of life.
Microsoft.ApplicationInsights.AspNetCore targets net6 to enable framework reference to Microsoft.AspNetCore.App.

Checklist

  • I ran Unit Tests locally.
  • CHANGELOG.md updated with one line description of the fix, and a link to the original issue if available.

The PR will trigger build, unit tests, and functional tests automatically. Please follow these instructions to build and test locally.

Notes for authors:

  • FxCop and other analyzers will fail the build. To see these errors yourself, compile localy using the Release configuration.

…ported microsoft#2850

use frameworkreference for Microsoft.AspNetCore.App instead of icrosoft.AspNetCore.* nuget packages, because they are now deprecated and end of life.
Copy link
Member

@TimothyMothra TimothyMothra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I would love to see this change, we have a requirement to continue supporting these frameworks for same legacy systems.

@antymon4o
Copy link
Author

antymon4o commented Mar 5, 2024

Thank you for you comment, @TimothyMothra.
I perfectly understand that dropping support for specific framework version is not an easy decision to make.
In the company I work, we have been using .NET Framework since version 1.1 for the systems we are developing. And as you can imagine, we have tons of legacy code. This is pretty normal for enterprise software that is evolving for decades. During the years we have been upgrading to the proper supported version of .NET Framework and .NET Core.
But there is a difference between having legacy code and running it on unsupported platform. The former is normal, but the latter is unacceptable if only for security and compliance reasons.
Of course, all the versions of ApplicationInsights packages up to 2.22 will always support net452 and net46. And thus if any legacy systems need them, they can use them.
As we cannot tolerate dragging dependencies of unsupported and legacy frameworks and packages, the need is for a version of ApplicationInsights that is dependent on current, supported packages and frameworks.
Given that there is no code change, and thus no behavior change in the library, what is the problem to release this new version?
Will it be more acceptable if the new version of ApplicationInsights is set to 3.0 or even 6.0 to indicate major change in supported frameworks?

@antymon4o antymon4o requested a review from TimothyMothra March 5, 2024 22:53
@MichaCo
Copy link
Contributor

MichaCo commented Mar 12, 2024

Although I would love to see this change, we have a requirement to continue supporting these frameworks for same legacy systems.

@TimothyMothra I'd suggest a major version release across the board (3.0).

There is a huge difference between AspNetCore 2.x/3.x and the latest versions under NET6/8 in terms of what nugets to reference vs using Framework dependencies...

Everything prior NET6 is long out of LTS anyways, so, why not just clean up the mess and publish a clean version for the new .NET 6/8 versions?

…net6.0

change conditional compilation expressions #if NET452 to #if NETFRAMEWORK

microsoft#2850
…e specifics in behavior coded in classes in project

In TelemetryChannel.ApplicationFolderProvider add #if NETFRAMEWORK in IsWindowsOperatingSystem method.

microsoft#2850
@antymon4o
Copy link
Author

In Microsoft.ApplicationInsights project, there are a lot of comments like:

// .NET 4.5.2 have a custom implementation of RichPayloadEventSource

And in RichPayloadEventSource.cs there is a conditional compilation #if !NET452.

The question is:
Is this implementation specific to NET452 only or is it specific to all framework versions in general?

If this implementation should be used in all framework versions, then this condition should be changed.

@pharring
Copy link
Member

@antymon4o I believe the conditional in RichPayloadEventSource.cs is correct. That's because support for anonymous types in EventSource events was introduced in .NET 4.6. There's a comment starting on line 393 of that file explaining it.

@antymon4o
Copy link
Author

@TimothyMothra can you please have another look at the PR and tell if the support for the deprecated and LTS out frameworks is critical.
Is there any chance the proposed changes to be merged in a new major version 3.0 of ApplicationInsigts?

@cijothomas
Copy link
Contributor

Is there any chance the proposed changes to be merged in a new major version 3.0 of ApplicationInsigts?

Sorry, there are no plans for a 3.0 Application Insights! However, dropping target framework can be done without major version bump! I am not suggesting we merge this PR and do normal release - I'll defer to @TimothyMothra to decide if this is acceptable or not.

It maybe worth exploring/fixing Microsoft.ApplicationInsights.AspNetCore package alone to fix the original issue described in #2850, more precisely raised in #2811

@antymon4o
Copy link
Author

antymon4o commented Mar 27, 2024

It is possible to fix only ApplicationInsights.AspNetCore package, but it would be a somewhat partial fix.
Actually, the fix for this package is part of this PR. If it is useful I can post only it as a part of a new PR.
I just need to know if it is definite that this PR is rejected? Who can confirm this and the accepted/approved approach to fix the issues?

@antymon4o
Copy link
Author

I have created e new PR #2860 with only the changes to Microsoft.ApplicationInsights.AspNetCore package and linked it to #2811.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants