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

Medium term strategy for Target Framework Monikers #1970

Closed
bartelink opened this issue Oct 21, 2023 · 11 comments
Closed

Medium term strategy for Target Framework Monikers #1970

bartelink opened this issue Oct 21, 2023 · 11 comments

Comments

@bartelink
Copy link
Member

bartelink commented Oct 21, 2023

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:

  • we support net462, net471 and netstandard2.0 where possible.
  • Where there are compelling features that dictate targeting newer TFMs, we favor the lowest even numbered TFM >= net6.0.
  • Odd-numbered target frameworks get trimmed on major version releases (netstandard2.1 and net5.0 included).

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 serilog/serilog-sinks-console#145 (comment).

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:

  • In general, if netstandard2.0 is insufficient, we target the lowest even numbered netX.0 TFM that will work. i.e. a light sink would target net6.0 (as opposed to targeting net12.0 just because it's there)
  • non-LTS TFMs should not be targeted unless there's a reason:
    • e.g. if .NET 9.0 adds a new API that lets us do something significantly faster etc
  • on a major version bump, we trim outdated non-LTS targets (e.g. if net10.0 is out, we remove net7.0 and/or net9.0 targeting
  • net462, net471 and netstandard <= 2.0 won't be aggressively trimmed
  • netstandard2.1 and net5.0 will be considered as non-LTS (so will be replaced with net6.0 on next major)

For this repo, that means V4 should:

  • remove netstandard2.1, net5.0 (ideally they would have been removed in V3 but the ship has sailed)
  • if V 3.10 had added a net9.0 target (because it needed a new API that was not in net8.0), V4 would remove that and replace it with net10.0

For the Console sink, the above rules would imply:

  • it should target netstandard2.0.
  • it should not target netstandard2.1, netcoreapp3.1, net5.0
  • it may target net6.0 if and only if there is something that can not be achieved via netstandard2.0.
  • bu, targeting net6.0 does not imply supporting net7.0 or net8.0 or any other later one.
  • if net9.0 added API that's the only way to deliver a compelling new feature and/or significant perf increase, it could be added as part of the feature PR for that

Test assemblies

  • out of support frameworks do not need to be tested and can be trimmed any time (i.e. can trim net5.0 any time)
  • At least one in date version should be in the targets at all times
  • non-LTS coverage is optional (i.e. when net9.0 is available, we can add it to the list, but there is no SLA demanding it)
@Numpsy
Copy link
Member

Numpsy commented Nov 14, 2023

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
Copy link
Member Author

bartelink commented Nov 14, 2023

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
Copy link

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
Copy link
Member Author

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:

  • netstandard2.1, net5.0 (both considered to not provide any specific value over supporting netstandard2.0 and net6.0)
  • net7.0 (there's not been any specific benefit identified for providing a specific build for it)

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
Copy link

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
Copy link
Member Author

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.

  • In Serilog core, the net7.0 build does not do anything different to net6.0 so it can go
  • 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.

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
Copy link
Member

👋 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
Copy link

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
Copy link
Member

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.

nblumhardt added a commit to nblumhardt/serilog that referenced this issue Feb 24, 2024
@Numpsy
Copy link
Member

Numpsy commented Feb 24, 2024

Q. about TFMs in other bits of the infrastructure - I started a PR at serilog/serilog-enrichers-environment#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
Copy link
Member Author

bartelink commented Feb 24, 2024

In general, the same principles apply

  • TFM-specific builds for netstandard2.0, net6.0
  • other supported LTS releases can be added, but will be trimmed with every major release of any given component (but see note below about making sure there is a reason for it)
  • if core had a net9.0 dep because something new was being used, that does not imply the same is required for any other lib
  • similarly, just because core happens to have net8.0 does not mean a new enricher must have a net8.0 build if that's not supported at the time it's being released
  • in general, the more TFM specific builds, the more busywork, and it needs to be for a reason. A 3 liner enricher really doesnt need to be compiled 5 times to produce the same IL. Also the more TFM specific builds that are addded and removed over time, the more churn in terms of things that previously resolved to net8.0 suddenly binding to net10.0 or net6.0 etc

The above sort of factors are my reasoning for applying the KISS principle as mentioned in my review comment on core: #2016 (comment)

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.

nblumhardt added a commit to nblumhardt/serilog that referenced this issue Feb 25, 2024
nblumhardt added a commit that referenced this issue Mar 1, 2024
* Update TFMs for #1970

* 'Policy' wording - review feedback

* Update src/Serilog/Serilog.csproj

Co-authored-by: Ruben Bartelink <ruben@bartelink.com>

---------

Co-authored-by: Ruben Bartelink <ruben@bartelink.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants