search

getsentry / sentry-dotnet

Sentry SDK for .NET
https://docs.sentry.io/platforms/dotnet
MIT License
604 stars 206 forks source link

Consider dropping support for netcoreapp2.0, netcoreapp2.1, and netcoreapp3.0 #1343

Closed SimonCropp closed 2 years ago

SimonCropp commented 3 years ago

many new MS nugets have dropped support for netcoreapp2.0, netcoreapp2.1, and netcoreapp3.0 eg

This is problematic for us since we consume many of these transitively in tests

This will be an increasing problem going forward as more nugets update to newer versions of ms packages and release.

The error we get is also misleading, since it errors for the transitive package, not the consuming package:

Severity    Code    Description Project File    Line    Suppression State
Error       System.CodeDom doesn't support netcoreapp3. Consider updating your TargetFramework to netcoreapp3.1 or later.

For context here are the support expiries:

SimonCropp commented 3 years ago

the announcement from microsoft about the nugets dropping support for runtimes https://github.com/dotnet/announcements/issues/190

bruno-garcia commented 3 years ago

netcoreapp2.0, netcoreapp2.1 are used in tests and samples. Without much verification we can agree to drop those from samples but tests need to have if the target library has them.

We target netcoreapp3.0 from lib as it's the initial target we had (before 3.1 LTS came out) and there was no reason to target 3.1 from 3.0 other than breaking any user running 3.0.

IMHO the deprecation from .NET itself isn't a signal for us to drop support while we have customers on those and there is usage in all versions. That said, if this will introduce issues with transient dependencies, we will need to plan for deprecating things. We don't have direct dependency though, do those you named (perhaps System.Runtime.CompilerServices.Unsafe)?

Maybe the best way forward is to get a matrix of library and TFMs and we discuss what targets to drop?

Sentry net6.0;net5.0;netcoreapp3.0;netstandard2.1;netstandard2.0;net461
Sentry.Extensions.Logging
Sentry.AspNetCore
Sentry.AspNetCore.Grpc
Sentry.Google.Cloud.Functions
Sentry.AspNet
Sentry.EntityFramework
Sentry.Serilog
Sentry.Log4Net
Sentry.NLog
Sentry.DiagnosticSource
SimonCropp commented 3 years ago

In libs the only direct reference we have System.Diagnostics.DiagnosticSource. we are likely to have more as more dependencies move forward

We have some indirect refs in tests. For now i have worked around this https://github.com/getsentry/sentry-dotnet/blob/main/test/Directory.Build.props#L8. as long as we have no api that conflict that will get us unstuck for test runs.

I think, if we want to be certain that we support certain netcore* targets, we need to explicitly add them in the TargetFrameworks for both libs and tests. If we dont then it is too easy to update a ref, have a clean buil+test run, and do a deploy not knowing that we broke something.

Note that this will get more difficult to support over time as tooling and libs drop support for older TFWs. eg the current beta of xunit 3 does not support netcore3.0 or netcore2.x

SimonCropp commented 3 years ago

I did some more research. the following would cause issues if we updated to the current stable

bruno-garcia commented 3 years ago

@SimonCropp We should not go for a single System.Text.Json. I believe it's best to do what we did here:

https://github.com/getsentry/sentry-dotnet/blob/ad9f5ff4ebeb1620d42d781b15d0e9f7edec441d/src/Sentry.Extensions.Logging/Sentry.Extensions.Logging.csproj#L13-L31

Lets keep what we have already since it's shipped, on net5.0: https://github.com/getsentry/sentry-dotnet/blob/ad9f5ff4ebeb1620d42d781b15d0e9f7edec441d/src/Sentry/Sentry.csproj#L46-L51

But going forward, lets add TFMs and bump dependencies only on those new ones. What do you think?

SimonCropp commented 3 years ago

We should not go for a single System.Text.Json

the approach we current use is problematic. lets say someone is using net48. So they will get Microsoft.Extensions.Http 2.1 from us. But Microsoft.Extensions.Http 2.1 was released in 2018. if the app is under active development they will likely be using a newer version. So now (for example) the sentry expects the the Microsoft.Extensions.Http api from 4 major versions ago. but the one being used is the current. So there is a change of a api mis-match. if this occurs an unhelpful exception will be thrown (TypeNotFound, MethodNotFound, etc)

SimonCropp commented 2 years ago

closing this one for now. there are still non trivial numbers of sentry users on older frameworks