serilog / serilog-extensions-hosting

Serilog logging for Microsoft.Extensions.Hosting
Apache License 2.0
139 stars 30 forks source link

CreateBootstrapLogger no longer available in .NET Standard 2.1 since 8.0.0 #82

Open botder opened 10 months ago

botder commented 10 months ago

Hello there,

in the 8.0.0 release, the target framework netstandard2.1 was removed (commit f22e3fd700ee2b0fd55fca625b94ef6af5221c48), and in turn, support for a reloadable bootstrap logger was removed for any .NET Standard 2.1 project using this library. See also the code snippet below:

https://github.com/serilog/serilog-extensions-hosting/blob/2b8e22185718393e4cdcfb00a8d9fef85930762f/src/Serilog.Extensions.Hosting/Serilog.Extensions.Hosting.csproj#L28-L30

bartelink commented 10 months ago

Do you have a netstandard2.1 lib that needs to use this functionality?

Can you retarget it to net6.0 (or later) ?

In other words, can you share some more context as to why this is a must-have for you please?

Ultimately if the diff to re-add it is reasonable, and there is a solid case for why a significant cohort of people would not be able to do something desirable, reinstatement is not necessarily off the table - the general principles here are outlined in https://github.com/serilog/serilog/issues/1970 but they are guidelines/principles, not absolutes

nblumhardt commented 10 months ago

Just to add to @bartelink's reply; this package and the other .Extensions-related ones use a different versioning policy to the rest of the Serilog packages, following their Microsoft.* dependencies exactly (or so the hope goes):

https://github.com/serilog/serilog-extensions-hosting#versioning

This came out of many and varied confusing issues supporting anything different, so while I think serilog/serilog#1970 is the way forward for the rest of serilog/, the Serilog.Extensions.Logging, Serilog.Extensions.Hosting, Serilog.AspNetCore, and Serilog.Settings.Configuration packages will ideally stay as they are (until .NET 9 and whatever TFMs the matching .NET packages target).

Unfortunately this does lead to issues like this one, sorry this was disruptive!

botder commented 10 months ago

Do you have a netstandard2.1 lib that needs to use this functionality? Can you retarget it to net6.0 (or later) ? In other words, can you share some more context as to why this is a must-have for you please?

Well yeah, it's in the title and description: I am using CreateBootstrapLogger to set up a basic logger for a generic host in a shared C# library (-> .NET Standard). I am not sure if I actually need this functionality (to automatically reload the logger after runtime/host configuration) at all, but it is the only library in my project, that bumps the target framework from .NET Standard 2.0 to 2.1 (you disable reloading for .NET Standard 2.0 in this project). I mean, if I can avoid it, I try not to target serveral frameworks if I can choose .NET Standard in my project - single compilation & less headaches.

bartelink commented 10 months ago

Thanks for the update.

I think Nick's policy makes sense so if you can work around it somehow, that's for the best.

The thinking behind my questioning was to explore:

And I can definitely appreciate not having to add a TFM to multi-target, https://github.com/serilog/serilog/issues/1970 and this lib are all seeking to limit that, both from a compile time/storage space perspective, but equally from the point of view of limiting how much egregious stuff you need to wade through to troubleshoot scenarios

Assuming you can find a reasonable workaround, can the issue be closed on the basis Nick outlined?

botder commented 10 months ago

Are you reading my message? I stated precisely why I am using .NET Standard 2.1 instead of 2.0 there.

I am using CreateBootstrapLogger, which creates a ReloadableLogger: https://github.com/serilog/serilog-extensions-hosting/blob/2b8e22185718393e4cdcfb00a8d9fef85930762f/src/Serilog.Extensions.Hosting/LoggerConfigurationExtensions.cs#L37-L40

The ReloadableLogger class is only available, when NO_RELOADABLE_LOGGER is false: https://github.com/serilog/serilog-extensions-hosting/blob/2b8e22185718393e4cdcfb00a8d9fef85930762f/src/Serilog.Extensions.Hosting/Extensions/Hosting/ReloadableLogger.cs#L15

NO_RELOADABLE_LOGGER is non-false for .NET Standard 2.0, but that was not the case for .NET Standard 2.1. https://github.com/serilog/serilog-extensions-hosting/blob/2b8e22185718393e4cdcfb00a8d9fef85930762f/src/Serilog.Extensions.Hosting/Serilog.Extensions.Hosting.csproj#L28-L30


Also, .NET Standard 2.1 (the one you dropped) is supported by .NET (Core) 3.0, 3.1, 5.0, 6.0, 7.0, 8.0 and it worked fine in 7.0.0. Is there any reason to explicitly target any of those frameworks directly in this project? And if that reloadable logger works in .NET Standard 2.0 for you, you could also just target .NET Standard 2.0 alone.

bartelink commented 10 months ago

Are you reading my message?

I absolutely understand the mechanics of it. And I appreciate that your code used to work as it was and does not now. And that it will not unless your code changes or the Serilog supported TFMs in V8 change.

There is a primary maintainer of this package.

He's doing a decent job by some standards: https://www.nuget.org/stats/packages/Serilog.Extensions.Hosting?groupby=Version

Nick's response is the overriding concern here. Please consider the reasons he gave for leaving it as it is in formulating a response that's useful for him in deciding whether there is a real unavoidable thing that's been missed which would justify changing the policy here. Again: I am not the maintainer of this package and there's no point beating points into my stupid head or 'winning' a debate with me.

botder commented 10 months ago

Microsoft.Extensions.Hosting 8.0.0 targets netstandard2.1, netstandard2.0 and some others. The Serilog version (3.1.1) this project uses, still supports .NET Standard 2.1. There is no reason to drop .NET Standard 2.1 right now.

https://github.com/dotnet/runtime/blob/ff6b26f0474698ee2fafd3639069c5ef0cc2d9e5/src/libraries/Microsoft.Extensions.Hosting/src/Microsoft.Extensions.Hosting.csproj#L4

If you don't re-add support for .NET Standard 2.1, then please go ahead and remove this comment: https://github.com/serilog/serilog-extensions-hosting/blob/2b8e22185718393e4cdcfb00a8d9fef85930762f/src/Serilog.Extensions.Hosting/Serilog.Extensions.Hosting.csproj#L8-L10

nblumhardt commented 10 months ago

Ah, looks like my mistake, here! Seems as though netstandard2.1 is targeted directly by the current crop of *.Extensions projects (wonder if that changed, temporarily, during the previews?). We should add this back across affected packages.

BrettJaner commented 3 months ago

I have a similar issue where my app needs to target .NET framework (net481 to be exact) in order to reference older third party dlls built against netfx. Is there any reason why we can't open the Bootstrap/Reloadable Logger up to apps targeting .NET framework as well?

jawztherevenge commented 3 months ago

Ah, looks like my mistake, here! Seems as though netstandard2.1 is targeted directly by the current crop of *.Extensions projects (wonder if that changed, temporarily, during the previews?). We should add this back across affected packages.

Does that mean CreateBootstrapLogger() support will available for netstandard2.0 and/or netstandard2.1?

I also have a logging library shared across .NET Framework and .NET 6/8 projects that wraps a bunch of common setup during app composition. It's the difference between being able to do something like MySharedLogger.CreateStartupLogger() at app startup vs Log.Logger = MySharedLogger.CreateStartupLoggerConfig().CreateBootstrapLogger().

Not a huge deal, but it is more convenient. And it does make the stated support for netstandard2.0 feel a little untrue...I was really scratching my head over why "LoggerConfiguration does not contain a definition for CreateBootstrapLogger"...

nblumhardt commented 3 months ago

Does that mean CreateBootstrapLogger() support will available for netstandard2.0 and/or netstandard2.1?

Likely 2.1, not 2.0 (which doesn't support default interface implementations). At this stage it looks like no one's had a chance to put together a PR so there's no current ETA.

I think the main reason for the lukewarm activity on this ticket might be that for a library, netstandard2.1 doesn't really enable anything that's not otherwise possible: if you can target it with netstandard2.1 you can alternatively add TFMs to your project for whatever specific platforms you need, with minimal extra effort in those cases.

PR definitely welcome, though.