Add net6.0 Target

[zachrybaker]

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

[SimonCropp]

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

[nblumhardt]

See also: serilog/serilog-sinks-file#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 ?

[bartelink]

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)

2. 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 #145 (comment)

[bartelink]

per the skeleton policy in the comment, I would not add net7.0 (and would also not add net8.0 in a few months either)

Suggested change

	<TargetFrameworks>net45;netstandard1.3;netstandard2.0;net5.0;net6.0;net7.0</TargetFrameworks>
	<TargetFrameworks>net45;netstandard1.3;netstandard2.0;net5.0;net6.0</TargetFrameworks>

[zachrybaker]

FWIW I think signaling support of .Net8.0 now does make sense. I just submitted a PR yesterday for another project that makes Grpc work with named pipes, that already had .Net8.0. Seems to be in the mix. Just be aware that some folks may not have the SDK on their box for whatever reason and some may not be able to add it yet for IT policy reasons.

[bartelink]

If we have a VERSIONING.md, we can point people to that.
It will mention that the default assumption is that the world uses Serilog and if it is not happy on the latest FW release or preview, that's a bug that needs fixing immediately, with a link to the download counts so people can stop relying on inferring support based on the presence of a variant that requires a TFM
It will also mention that just because there is only a net6.0 target in the package does not mean that it can't be used with 8/9/10/11/12 etc

For me the important part of supporting net8.0 is to have the test suite use it

This means two things

• people forking the repo need it on their box
• the CI needs to support it (if its -latest images, that's covered)
  For gRPC stuff, it would not surprise me if they have a dependency on new stuff in 7.0 or 8.0

In that case, they will likely no longer support 6.0, which is a breaking change which we would signal with a major version bump
If Serilog added a net8.0 target for a reason (some newer API means we can implement a feature or do something quicker or with less memory), then the CI would need a new SDK, and the package would get a new TFM - but there is zero impact on devs using the package - they can upgrade and a net6 SDK will safely ignore the new package (and use the most relevant impl that's already in the package)

cc @nblumhardt @SimonCropp

[bartelink]

Sorry, realised the actual point you were making about 'IT policy reasons'; IMO:

• main should never use non-RTM TFMs (i.e. baseline stance is no point releasing a final version that relies on a thing in preview)
• dev and PRs can use previews if developing a new feature or tuning that requires preview - the critical bit is the CI env having them; we would not merge without that anyway
• because CI will cover the preview SDK requirement, people contributing PRs can do a local edit to temporarily e.g. skip testing net8.0

Bottom line is that this situation will be very rare, as we're not talking about constantly being on tenterhooks to get some new null release out adding a new Target to the package whenever MS are about to drop a release of anything

[bartelink]

per my skeleton policy, I'd remove the long dead core versions from the test matrix
Not sure if net452, 62, 72 have value, but net45;netstandard1.3 need coverage (I'm assuming net48 covers netstandard2.0)

Suggested change

	<TargetFrameworks>netcoreapp2.1;netcoreapp2.2;netcoreapp3.0;netcoreapp3.1;net452;net462;net472;net48;net5.0;net6.0;net7.0</TargetFrameworks>
	<TargetFrameworks>net452;net462;net472;net48;net6.0;net7.0</TargetFrameworks>

[zachrybaker]

@nblumhardt thoughts? Leaner makes sense to me, and this is a test project after all, so targeting standards is not needed.

[bartelink]

net7 was added here but not in File; we should be consistent one way or the other (I'm fine with adding it in both or neither). Though it seems the most reasonable thing is to add it into .File

[bartelink]

Thanks for doing the work - in general it all makes sense

I added comments re minor things that don't fit my policy proposal.

Others will review in due course (and/or I'll adjust this review in line with agreed changes to the policy)

[bartelink]

Aligning with policy/coverage in serilog/serilog:

Suggested change

	<TargetFrameworks>netcoreapp2.1;netcoreapp2.2;netcoreapp3.0;netcoreapp3.1;net452;net462;net472;net48;net5.0;net6.0;net7.0</TargetFrameworks>
	<TargetFrameworks>net472;net48;net5.0;net6.0;net7.0</TargetFrameworks>

[bartelink]

LGTM; would like to see us either have net7.0 included in File test suite or removed from this one for consistency though
(and in both places, if the CI can support it, no harm to validate test suite with net8.0)

[bartelink]

@zachrybaker Needs rebasing now. As I see it:

• Medium term strategy for Target Framework Monikers serilog#1970 documents the policy
• dev has 6.0 in all places, tests are at 7.0 (i.e. your PR has effectively been cherry-picked)
• the impl package right now targets net462;net471;netstandard2.1;netstandard2.0;net5.0;net6.0;net7.0
• there has not been a final 5.0.0 release yet

I believe the correct thing to do now is to follow the policy, and trim the targeted TFMs for the implementation package in Serilog.Sinks.Console.csproj to net462;net471 + netstandard2.0;net6.0

Once that's merged and the dev build has been validated, we'd be clear for a 5.0.0 final release

[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?
#145 (review)

[nblumhardt]

@bartelink thanks for the ping 👍

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 :-)

[bartelink]

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.

[nblumhardt]

Sounds good 👍

[Numpsy]

Was this superceeded by #157 ?

[nblumhardt]

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

[zachrybaker]

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 <#145 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACDSMSFLLEIDTQPKW5IEVTZIPUFBAVCNFSM6AAAAABJUV5V7SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBSGMZTKNJWGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>