serilog-sinks-console icon indicating copy to clipboard operation
serilog-sinks-console copied to clipboard

Add net6.0 Target

Open zachrybaker opened this issue 2 years ago • 7 comments

What issue does this PR address? Please add net6.0, net7.0 targets

Does this PR introduce a breaking change? No

Please check if the PR fulfills these requirements

  • [yes ] The commit follows our guidelines
  • [ n/a] Unit Tests for the changes have been added (for bug fixes / features)

Other information: Please list any other relevant information here

zachrybaker avatar Sep 06 '23 17:09 zachrybaker

while i dont object to this PR. i am curious as to why it is required. targeting net5 should be enough to support net6 and net7

SimonCropp avatar Sep 06 '23 23:09 SimonCropp

See also: https://github.com/serilog/serilog-sinks-file/pull/295

I think this in some ways comes down to a support issue; we do hit reports pretty much every week about this or similar concerns.

Serilog 3 is now targeting:

    <TargetFrameworks Condition=" '$(OS)' == 'Windows_NT'">net462;net471</TargetFrameworks>
    <TargetFrameworks>$(TargetFrameworks);netstandard2.1;netstandard2.0;net5.0;net6.0;net7.0</TargetFrameworks>

It might make the most sense to a) target these TFMs here (and in the other linked PR), b) bump the major package version to communicate the breaking change (dropping the oldest .NET Standard versions), and c) move to enshrine these in a standard .props file we share across all of the core Serilog packages.

(Edit: I should say - except for the Extensions ones, which target .NET versions 1:1 for other reasons.)

What do you think @SimonCropp @bartelink ?

nblumhardt avatar Sep 07 '23 00:09 nblumhardt

I don't see how this ever becomes manageable unless we figure out a unified policy and have a statement to refer people to.

I'd say the basics are:

  • there are cases where APIs / overloads added in newer TFMs enable things to be done more efficiently. We're all for taking advantage of that - if doing a variant targeting a specific TFM brings value
  • net6.0 feels like a reasonable place to draw a line; net5.0 was not an LTS release so will get people worrying unnecessarily. Sinks.File should get an upgrade as a result.
  • doing a custom net7.0 and net8.0 just bloats the package unless there's an actual new API being used. It also invites busywork the minute the net9.0 betas hit the street (not far off)
    • if there is a feature or perf need that could benefit from a net9.0 feature, we can add a dependency on it
    • if that happened, we can put in net10.0 alongside, and trim the net9.0 when the next major version rolls
    • we would not add net11.0 or net12.0 unless something actually needed and used a new API

In summary, my proposed policy for impls is:

  • anything that needed net5.0 triggers adding net6.0 when someone wants (NOTE this does not actually solve any problem)
  • when we roll a new major version
    • netstandard3.1, net45 and friends are eligible for removal unless a speciic package is providing downlevel support without any problems
    • netstandard2.0 always stays if it is supported
    • all non LTS requirements are replaced with the current LTS
      • so a new version now will move netstandard2.1, netcoreapp3.1, net5.0 to net6.0
      • a new version when net8.0 is released will replace net5.0 with net6.0, net7.0 with net8.0
    • min LTS requirement is not upgraded unless there is a reason (we have code that takes advantage of it)
      • if something targets net6.0, we don't remove that or add net8.0 or net10.0 or net12.0 just because net12.0 is the current LTS

Then we need specific rules about versioning:

  • the tests in dev should target all released FWs (i.e. definitely net7.0)
  • there comes a point when one almost released .NET version should also be eligible (e.g., targeting net8.0 now might be reasonable even if it's not actually released). net9.0 should not be considered until at least after net8.0 is released
  • the tests in main should not be expected to target EOL'd TFMs
    • so net5.0 is a gonner - replace with net6.0
    • net7.0 is eligible to add
    • net7.0 is eligible for removal when net8.0 is released
    • Q: can net8.0 be added before it's released? I assume if there are interesting changes there will be times when we want to do this. Probably no need to set an actual timeline though
  • at the point where we have sufficient compelling justification for a breaking change, we roll a new version
    • remove Obsolete methods
    • update all test dependencies (always allowed even without a major release, but forced by a major release)
    • TFMs for implementation projects should be reassessed when a new version is introduced
      • any net3/5 should be removed and replaced with net6.0 (but removal always needs to be a major version bump)
      • perhaps trim netstandard < 2.0 ?
      • are we trimming net45? (File has a variant for it)
      • no point trimming netstandard2.0
      • netstandard2.1 does not have many users - should consider removing in favor of a net6.0 dep
  • sometimes nothing has happened in a repo for ages. That should be considered normal for a Sink repo. Perhaps a prominent link to the download stats might a) give people an idea of just how much usage something has, as a way to inspire confidence b) make them appreciate that SO it the place to ask questions, rather than the issue tracker

Perhaps Nick could add a VERSIONING.md skeleton to Serilog core repo and we define the policy there, including a notes sections for warts that are in place but deprecated

So, specific things now:

  1. roll a new Serilog.Sinks.File version
  • atm impl targets net45;netstandard1.3;netstandard2.0;netstandard2.1;net5.0
  • 👍 could do a 5.0.1 that adds net6.0 to bring it into line with policy of replacing netstandard2.1, netcoreapp3.1, net5.0 with net6.0
    • a similar PR for net7.0 (or 9/11 when released) should be refused as not LTS (and no new APIs that enable anything)
    • I also think adding net8.0 should not be explicitly targeted (even when net6.0 EOLs)
  • 👎 could do a 6.0 that removes net5.0 and netstandard2.1 targeting, but there is nothing compelling and new ton justify it, and it's not broken.
    • net45 drives conditionals so ideally that can be removed to simplify things
    • would consider targeting netstandard2.0 only, unless there is a reason and value in supporting netstandard1.3 (or 2.1 or 5.0). If we get that wrong, new ones can always be re-added in a 6.1
  • tests target net48;net5.0
    • no need to keep targeting net5.0; that should change to net6.0. Can add net7.0, though (no restriction on non-LTS).
    • (See above, maybe add net8.0 even now, depending on whether we wait for RTM)
    • net48 makes sense as long as there is netstandard2.0 (or net452 or netstandard1.3)
  1. new Serilog.Sinks.Console release
  • targets `net45;netstandard1.3;netstandard2.0;net5.0
  • tests target netcoreapp2.1;netcoreapp2.2;netcoreapp3.0;netcoreapp3.1;net452;net462;net472;net48;net5.0
  • net5.0 opts in to using span
  • tests should target net48, net6.0 (and optionally net7.0, maybe net8.0)
  • impl should add a net6.0 target in line with policy above
  • 👍 should add a 4.2.0 that has a net6.0 variant in line with above policy of not targeting netstandard2.1/netcoreapp3/net5.0
  • 👎 no compelling reason for a 5.0.0, but if there was
    • remove obsoletions
    • stop targeting net5.0 (need to provide a net6.0 as it is actually being used)

addendum: see also this comment for more reasons to have a VERSIONING.md https://github.com/serilog/serilog-sinks-console/pull/145#discussion_r1318737211

bartelink avatar Sep 07 '23 09:09 bartelink

@nblumhardt I guess it's too late now, but maybe a 5.0.1 that trims the netstandard2.1, net5.0 and net7.0 targets might still make sense? https://github.com/serilog/serilog-sinks-console/pull/145#pullrequestreview-1691901895

bartelink avatar Nov 09 '23 08:11 bartelink

@bartelink thanks for the ping :+1:

I think the packages that just went out all align with the same TFMs as Serilog 3.1, definitely room for an update but not sure yet whether we should call that "3.2" or "4.0" ----- brewing some ideas :-)

nblumhardt avatar Nov 09 '23 10:11 nblumhardt

I think the packages that just went out all align with the same TFMs as Serilog 3.1,

True; I guess it's not a massive deal

For avoidance of doubt,Serilog.Sinks.File has not got the net7.0 added (and has not been released for a while; adding a net6.0 is the key thing a push will add). I'd suggest we don't add net7.0 (or anything) in the 5.x timeline. If we're looking to do a File 6.0.0, rather than a 5.1/5.0.1 then likely dropping netstandard2.1;net5.0 would make sense too at that point.

bartelink avatar Nov 09 '23 10:11 bartelink

Sounds good 👍

nblumhardt avatar Nov 09 '23 10:11 nblumhardt

Was this superceeded by #157 ?

Numpsy avatar Jun 20 '24 20:06 Numpsy

Thanks @zachrybaker and reviewers; everything should now be aligned with Serilog 4's TFMs, we got there in the end! :-)

nblumhardt avatar Jun 21 '24 09:06 nblumhardt

Huzzah!

On Fri, Jun 21, 2024 at 4:05 AM Nicholas Blumhardt @.***> wrote:

Thanks @zachrybaker https://github.com/zachrybaker and reviewers; everything should now be aligned with Serilog 4's TFMs, we got there in the end! :-)

— Reply to this email directly, view it on GitHub https://github.com/serilog/serilog-sinks-console/pull/145#issuecomment-2182335561, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACDSMSFLLEIDTQPKW5IEVTZIPUFBAVCNFSM6AAAAABJUV5V7SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBSGMZTKNJWGE . You are receiving this because you were mentioned.Message ID: @.***>

zachrybaker avatar Jun 21 '24 15:06 zachrybaker