serilog / serilog-sinks-file

Write Serilog events to files in text and JSON formats, optionally rolling on time or size
Apache License 2.0
334 stars 117 forks source link

Please add net6.0 as a target framework, or remove net5.0 target #294

Closed zachrybaker closed 1 year ago

zachrybaker commented 1 year ago

I have a client who is up in arms because this package "does not support net6.0."

Yes, I know there's a target of NetStandard2.1 being used.

Net5.0 is out of support since May of last year. So I would imagine you can fall in line and swap net5.0 to net6.0.

Thanks.

zachrybaker commented 1 year ago

Or just remove the .net5.0 target. I mean if we aren't going to stay aligned with frameworks, at least fall back to .NetStandard2.1?

bartelink commented 1 year ago

Have you tried to build it that way? It may be that there is something that can be done more efficiently in net5.0 or later - if that's the case, then its unlikely that net5.0 would be removed

The policy for Core Serilog seems to be to add explicit targeting per dotnet version https://github.com/serilog/serilog/blob/dev/src/Serilog/Serilog.csproj#L7

The solution is likely going to be one of:

  1. remove net5.0 (treat it as a bug; only if there turned out to be no special case code benefiting from that)
  2. add net6.0 and net7.0 (not sure if 6 and 7 are both supported due to different code/new APIs, or to get explicit testing for the newer FWs, with the shipping of those specific packages only a side-effect)

It might also be useful to ask on SO; I'm sure there are others that have walked this path with similar cleanliness/correctness concerns

(if it was my personal lib, my stance would be that targeting net5.0 and it being out of date is irrelevant - it only defines a minimum APIset requirement. It's not like changing it to net6.0 solves the problem for any interesting length of time if support timespan is going to decide things)

zachrybaker commented 1 year ago

Yes, hopefully we can add .Net6.0 and get us back to "supported" as 5.0 is EOL. Sounds like this package really just needs to be maintained. I've done all the legwork actually, here's the PR.

@bartelink your stance doesn't fly with operations of the companies I work with. Obviously different orgs have different perspectives. I have pointed out that officially MS-supported is one thing, but staleness, deprecated is probably more important since it leads to packages more likely to have known or unknown vulnerabilities. Yet many companies really value the comfort that using officially-supported frameworks provides and also want the easiest path to knowing they don't have code reliant on stale runtimes. This particular client is scanning for vulnerabilities, and is also pointing JetBrains DotPeek at the deployment folders and seeing that it finds this package as a .net5.0 package (because that's the newest target framework that it supports).

bartelink commented 1 year ago

I don't particularly get your point - here's how I see it:

I happen to target netstandard2.0 or net6.0 for most libs I produce

For an app, on the other hand:

Based on the above, I'm saying:

In some really tinfoil hat scenarios, doing null updates but compiling with a new SDK version (upping the ver in the build actions and/or global.json) can be important - but even then, the important things is bugfixes in the compiler, not that the IL was produced for "an in-date FW" - it's just IL and that hasnt changed for a heck of a long time

There has to be a good document that lays this out somewhere, as the notion that you need to endlessly target new FWs the minute they release until your nuget contains 50 variants makes no sense and wastes a lot of people's time...

zachrybaker commented 1 year ago

@bartelink where is this comign from? are you the guy who has to approve my PR, or not?

bartelink commented 1 year ago

I've made some very minor contributions to Serilog and am a reviewer for a different sink but am not a maintainer of this one.

However I do develop .NET libraries, so it's not just wacky ideas. One of the maintainers will drop by in due course, but please be patient - ultimately there's no actual benefit or risk reduction that doing a change to up specifically target something other than net6.0 will bring - ultimately even the netstandard1.1 variant will work perfectly fine too. There's plenty more consumers of this lib though, so don't worry about this issue being left to rot for weeks/months either

This is not to say that the tests shouldn't be extended to prove that it works for all current FWs as you'e done - my core pint is about there being no actual benefit to targeting something beyond net5.0, regardless of any warm fuzzies it may give somone.

have you referred the client to https://www.nuget.org/packages/Serilog.Sinks.File/5.0.0#supportedframeworks-body-tab ? The TL;DR is that it just defines a minimum and they're all implicitly forward compatible. slight tangent but e.g.: https://github.com/serilog/serilog-sinks-async/issues/87

nblumhardt commented 1 year ago

Aligning the target frameworks with Serilog 3's seems like a reasonable update to make šŸ‘

As @bartelink points out, there's a difference between targeting .NET 5's API set, and actually executing .NET 5 code.

The former case, because .NET 5's API surface is a subset of .NET 6's, is a reasonable place to be. You can think of .NET 5 in this case as an updated .NET Standard: at runtime, you'll still be running .NET 6 or whatever TFM your app entrypoint targets - there should be no EOL, stale, or deprecated .NET 5 code involved.

Unfortunately, the current crop of dependency scanners seem to have limited understanding of how .NET works, so very conservative false-positive results seem to be the norm. May be one to discuss in more detail with your client, as you'll hit this issue with other packages regardless of what we do here.

Thanks for the PR nonetheless, will take a look.

zachrybaker commented 1 year ago

@bartelink Preaching to the choir my friend. You can move along now.

zachrybaker commented 1 year ago

Unfortunately, the current crop of dependency scanners seem to have limited understanding of how .NET works, so very conservative false-positive results seem to be the norm.

Yes @nblumhardt I have already explained via email to the persons charged with keeping operations aligned with not-EOL-ed frameworks like .Net5.0 that thereā€™s a false sense of security being presented with DotPeek where it flags the latest target that it sees (in this case .Net5.0, erroneously called .NetCoreApp5.0 in DotPeek) and that we canā€™t interpret that to mean that it is therefore requiring the .Net5.0 runtime or that it is inherintly now stale or deprecated. Iā€™ve also communicated the arguably much more interesting and concerning issue of dealing with deprecated packages or outdated packages as they are more likely where actual potential vulnerability lies. Explained the difference between a dll targeting a framework or standard as IL and an executable targeting a framework (runtime) but not a standard. Explained how if eliminating what DotPeek sees as Net5.0 is the goal, that they will burn man hours to get there that they may should spend moving away from deprecated packages instead. No feedback yet, but the ship doesnā€™t turn really fast, and meanwhile they are likely feeling skittish b/c the pain of removing the .Net5.0 runtime and breaking some production apps apps that had some peculiar dependencies that did not in fact work in .Net6.0 once the .Net5.0 runtime was removed.

OTOH I imagine they would argue that this package should only be targeting the standards it adheres toā€¦.especially if its not going to be kept up-to-date with MSā€™s RTM cycle. I guess there could be some optimizations Iā€™m unaware of when targeting .Net5.0 rather than the standard that donā€™t come into play when the IL is generated according to standard but do when targeting .Net5/6/7?

bartelink commented 1 year ago

I appreciate you're in a heck of a busywork loop, and I don't envy your position. But, ultimately, there is nuance to this, and adding new TFMs till the end of time is not sustainable.

@bartelink Preaching to the choir my friend. You can move along now.

I'd suggest a different formula of words net time you're attempting to move a conversation 'along now' in future. There are better places to kick the cat than github

Any implementation TFM variant addition needs to be for a reason.

It's critical that we in this symphony get to a clear policy and as part of that, it's important to not oversimply things.

  1. test TFM lists and impl TFM lists are entirely different things
    • one defines what is running and/or where we know it works. That can have stuff added and trimmed willy nilly. Removing EOL'd framework is fair game. If the CI can be rigged, targeting pre-RTM is potentially OK
    • the other defines a set of APIs that we have a unique story for
      • adding more increases compile time and package size
      • adding newer ones does not necessarily even change/fix the IL (see SDK point)
      • sometimes targeting a newer one allows one to reduce the set of package reference dependencies - that's definitiely valuable. But each such instance should be justified and mentioned in a comment next to it. e.g. its not clear the net5.0 here ever had a purpose, whereas in the Console sink it seems it did
    • net46, netstandard2.1, netcoreapp3.1, netcoreapp2 etc have many warts and should be avoided
    • netstandard2.0 is the default way to support the full FW, but older ones can sometimes suffice
  2. the SDKs we use to build are important - they should be relatively in-date. Whether a package was build 5 years ago with a dead compiler is the problem, not that the implementation does not lean on any APIs added in the last 5 years.

See longer version attempting to define a full policy https://github.com/serilog/serilog-sinks-console/pull/145#issuecomment-1709862581

especially if its not going to be kept up-to-date with MSā€™s RTM cycle.

MS roll out a new stack of aspnet dlls with every FW release. There is no need for an arbitrary external package to do the same.

Either the IL needs to be different (a compiler fix, or an impl fix), or leave it alone. Again: a library targets an API set and is built with the compiler from a SDK. The TFM its built against is only relevant if that changes the packages it references

If I'm upgrading from net6.0 to net10.0 runtime, the ideal is for me to be using the exact same version of the same TFM of the same Serilog. Having anything change while I'm changing the runtime my app is using wins me nothing - just more things that I need to eliminate from my inquiries when troubleshooting

When I'm NOT updating the app target FW, a release of Serilog that REMOVES TFMs makes my life simpler, and I'd be looking to take it (e.g. if it used to have a variants for net5.0, netstandard1.3 and now has only a single netstandard2.0 variant, I'm delighted, as more people are using the same bits as me (as long as nobody that needed netstandard1.3 is suffering))

I guess there could be some optimizations Iā€™m unaware of when targeting .Net5.0 rather than the standard that donā€™t come into play when the IL is generated according to standard but do when targeting .Net5/6/7?

While possible, I suspect this is very rare. Far more likely is that a newer compiler (via a newer SDK) would deliver a different IL encoding that the JITter in a runtime would do a more efficient job with. i.e. if you add a net8.0 TFM to a lib and the perf gets better, the same win may also apply to an existing net6.0 target that is being compiled with the newer compiler.

zachrybaker commented 1 year ago

@bartelink Iā€™ll own poor word choice of ā€˜move alongā€™ my apologies. There can be a fine line between wanting to understand why people need/want the proposed change, and suggesting they must be doing it wrong. Sometimes we humans just need a context switch.

And I can appreciate your thoroughness and desire to stop the madness of staying on top of so many target frameworks with the serilog packages.