serilog / serilog

Simple .NET logging with fully-structured events
https://serilog.net
Apache License 2.0
7.26k stars 798 forks source link

Medium term strategy for Target Framework Monikers #1970

Closed bartelink closed 7 months ago

bartelink commented 1 year ago

Atm Serilog 3.X targets net462;net471;netstandard2.1;netstandard2.0;net5.0;net6.0;net7.0 and will continue to do so.

This was arrived at organically. Going forward, the TL;DR of the policy of what to target is:

This implies that Serilog 4.X will likely target the following TFMs: net462;net471;netstandard2.0;net6.0.


Reasoning

This issue replaces #1966 and comment threads such as https://github.com/serilog/serilog-sinks-console/pull/145#issuecomment-1709862581.

There's been requests to support net8.0 etc, inferring based on the above broad range. If we have a clear strategy in the core package and repo, the other sinks can follow. [Longer version]

Production assemblies

For production assemblies, the following rules apply for picking TFMs, the following rules should apply:

For this repo, that means V4 should:

For the Console sink, the above rules would imply:

Test assemblies

Numpsy commented 11 months ago

it may target net6.0 if and only if there is something that can not be achieved via netstandard2.0.

Not sure how much it effects sinks as you might usually only include the ones you're actually using, but -

The .NET 8 docs at https://learn.microsoft.com/en-us/dotnet/core/compatibility/sdk/8.0/trimming-unsupported-targetframework document some changes in behavior when trimming apps that contain libs targetting .NET Framework or .NET Standard which suggest that it's benefficial for trimming purposes to have .NET 6.0 TFMs for the libraries even if they aren't using any .NET 6 specific functionality?

bartelink commented 11 months ago

it's beneficial for trimming purposes to have .NET 6.0 TFMs for the libraries even if they aren't using any .NET 6 specific functionality?

πŸ‘ interesting; the rules do specify having a net6.0 TFM regardless, and netstandard2.0 where possible (it's net5.0 and netstandard2.1 that will just get removed).

That IsTargetFrameworkCompatible predicate also looks like a useful technique in general (though the general explicit PropertyGroups per TFM works fine for now)

andrewlock commented 11 months ago

Just wanted to clarify (warn?), that by adding a reference to System.Diagnostics.DiagnosticSource, which references System.Runtime.CompilerServices.Unsafe, you have effectively dropped support for .NET Core 2.x, due to the way Microsoft started adding .targets files that explicitly error when the netstandard2.0 parts are used on < .NET Core 3.1 TFMs:

This is an example from our CI /project/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.

I'm really only flagging it because this dropping of TFMs was added in a minor version bump AFAICT, so I would argue is very non-semver πŸ™ˆ

If you're not aware of the issue, I wrote about it last year: https://andrewlock.net/stop-lying-about-netstandard-2-support/. The onus is firmly on MS on this one IMO, but library authors don't seem to realise the implications of adding some of these libraries.

For what it's worth StackExchange.Redis similarly added a dependency recently and accidentally dropped support, as described here.

Just to be clear, I'm not at-all saying you should support .NET Core 2.1 just that "Support" for .NET Standard 2.0 doesn't really get you much IMO. Basically Xamarin and that's it. And it's misleading in NuGet because it looks like you should be able to use it without issues on .NET Core 2.x without issue. So just consider this a heads up πŸ˜„

bartelink commented 11 months ago

Firstly, thanks for taking the time to note this.

this dropping of TFMs was added in a minor version bump AFAICT,

No, this is about trimming of specific TFMs when a major bump happens; no such trims have taken place.

On the pending trim list, there's:

One minor wart is that Serilog.Sinks.Console which has just had a 5.0.1 release targets identical TFMs to the core; ideally net5.0, netstandard2.1 and net7.0 would have been trimmed. (Doing so would have been a good dry run)

@andrewlock Perhaps you might be aware of a good reason not to avoid dropping netstandard2.1 for a package there are netstandard2.0 and net6.0 specific builds?


I guess the next question is what someone in your position should be expected to do...

The driving assumption is that given 2.x, 3.x and 5.x are out of support, consumers can either a) use older versions that supported them or b) update to a supported STS/LTS. Is that viable in your case, or do you see another way to resolve the forces?

andrewlock commented 11 months ago

No, this is about trimming of specific TFMs when a major bump happens; no such trims have taken place.

Sorry, my comment is a bit of a side-point really, I'll move to a separate issue, I just thought it might be pertinent πŸ˜„

Perhaps you might be aware of a good reason not to avoid dropping netstandard2.1 for a package there are netstandard2.0 and net6.0 specific builds?

I mean, you can use Span<T> in <net6.0 is the main benefit πŸ€·β€β™‚οΈ But personally I would have favoured netcoreapp3.1 over netstandard2.1 in the first place πŸ˜„

I guess the next question is what someone in your position should be expected to do...

The driving assumption is that given 2.x, 3.x and 5.x are out of support, consumers can either a) use older versions that supported them or b) update to a supported STS/LTS. Is that viable in your case, or do you see another way to resolve the forces?

My case is a special one, because on the Datadog .NET tracer team I'm just testing libraries against a bunch of TFMs. Both of your options are viable and reasonable IMO.

The problem IMO is that the "dropping of support for TFMs" (which already happened when you added System.Diagnostics.DiagnosticSource) should only happen in a major version bump!

bartelink commented 11 months ago

Perhaps you might be aware of a good reason not to avoid dropping netstandard2.1 for a package there are netstandard2.0 and net6.0 specific builds?

I mean, you can use Span in <net6.0 is the main benefit πŸ€·β€β™‚οΈ But personally I would have favoured netcoreapp3.1 over netstandard2.1 in the first place πŸ˜„

Not talking about the specific APIs that are only available in a given (newer) TFM. I'm wondering if there's a benefit to offering a TFM-specific build independent of whether one would make use of APIs from it, i.e.

But personally I would have favoured netcoreapp3.1 over netstandard2.1 in the first place πŸ˜„

Thankfully I've never had to think about that, and there's no chance of main needing to worry about it either!

The problem IMO is that the "dropping of support for TFMs" (which already happened when you added System.Diagnostics.DiagnosticSource) should only happen in a major version bump!

Yes; I can appreciate that this is the effect. I'm pretty sure this impact was not something that people were conscious of. The 3.0 release was a cleanup release, and 3.1 is focused on otel integration.

The general philosophy is that the current release (and main branch) should focus on supporting as many "current platforms" as practical. The problem is that the assumption that "if you can compile against netstandard2.0 you cover a lot of consumers" has been proven false.

It would seem that a possible resolution is:

  1. put up a notice about the netstandard2.0 being subject to the "as long as we're not talking .NET core 1.0-3.1" priviso for now...
  2. log an issue to #ifdef out the DiagnosticSource stuff in the netstandard2.0 (and netstandard2.1) builds ; when that's done, the notice can go

Nick (who added the otel integration) will be along in due course and will likely have more complete answers in due course...

nblumhardt commented 11 months ago

πŸ‘‹ I've replied RE the breaking change in #1983 so that we can keep this ticket focused on what our TFM list for vNext will be like. I didn't note the (2) #ifdef option over there - thanks @bartelink! - I'll add a note on why I'm not a fan, although it's worth keeping up our sleeves.

andrewlock commented 11 months ago

I'm wondering if there's a benefit to offering a TFM-specific build independent of whether one would make use of APIs from it... netstandard2.0 and netstandard2.1 also build with identical flags so the plan is to drop netstandard2.1 specific builds in the next major. I'm asking if you, as someone who has much better appreciation of it all, are aware of a problem that removing it might cause.

To my knowledge, there's absolutely no benefit to netstandard2.1 in this case, and it should be completely non-breaking πŸ™‚

It would seem that a possible resolution is:

I agree on both of those, and I think option 1 is the way to go for the sake of everyone's sanity and the longer-term health of the project πŸ˜„

nblumhardt commented 8 months ago

We had some discussion in another thread that might cause us to tweak this strategy slightly; quoting @bartelink:

net6.0 + supported LTS FWs at time of release of any major version, but we trim STS versions, and unsupported LTSes

I'm just whipping up an initial tiny PR to set dev to 4.0; this ticket could be the next one to follow.

Numpsy commented 8 months ago

Q. about TFMs in other bits of the infrastructure - I started a PR at https://github.com/serilog/serilog-enrichers-environment/pull/55 a while back with the idea that we could agree on the TFMs for one of the enrichers and then change the others (part of the idea being that having them target things like .NET Standard 1.x seems a waste of time in the main lib doesn't support them any more). Note sure if those calls should be made based on what Serilog 3 does, or what 4 will do?

bartelink commented 8 months ago

In general, the same principles apply

The above sort of factors are my reasoning for applying the KISS principle as mentioned in my review comment on core: https://github.com/serilog/serilog/pull/2016#discussion_r1501365476

TL;DR the simplest and most reasonable thing is for core to have netstandard2.0, net6.0 only, and only add anything else when it actually provides a significant benefit. For enrichers and other things, starting off with net6.0 only is a reasonable default (esp if its not envisaged that people would use them in an environment that only supports netstandard2.0 and/or oyu'd be doing conditional compilation or any other form of shimming)


For test assemblies, that's entirely different, you want the min .NET FW, the latest .NET FW, and all supported LTS and STS releases to be covered. For instance, that validates that net14.0, net15.0, net16.0 all function well despite the core assembly only having e.g. netstandard2.0 and net6.0 specific builds.