search

serilog / serilog-extensions-hosting

Serilog logging for Microsoft.Extensions.Hosting
Apache License 2.0
141 stars 34 forks source link

Transitive dependencies on MS.Extensions.* should align with the major version shipped with the SDK #84

Closed damianh closed 9 months ago

damianh commented 9 months ago

Hello,

Spotted this on https://www.nuget.org/packages/Serilog.Extensions.Hosting

net6.0
Microsoft.Extensions.DependencyInjection.Abstractions (>= 8.0.0)
Microsoft.Extensions.Hosting.Abstractions (>= 8.0.0)
Microsoft.Extensions.Logging.Abstractions (>= 8.0.0)
Serilog (>= 3.1.1)
Serilog.Extensions.Logging (>= 8.0.0)

net7.0
Microsoft.Extensions.DependencyInjection.Abstractions (>= 8.0.0)
Microsoft.Extensions.Hosting.Abstractions (>= 8.0.0)
Microsoft.Extensions.Logging.Abstractions (>= 8.0.0)
Serilog (>= 3.1.1)
Serilog.Extensions.Logging (>= 8.0.0)

net8.0
Microsoft.Extensions.DependencyInjection.Abstractions (>= 8.0.0)
Microsoft.Extensions.Hosting.Abstractions (>= 8.0.0)
Microsoft.Extensions.Logging.Abstractions (>= 8.0.0)
Serilog (>= 3.1.1)
Serilog.Extensions.Logging (>= 8.0.0)

We have a .net6 and .net7 projects that had compilation errors after upgrading to v8 of this library. Specifically things like:

Error   CS0121  The call is ambiguous between the following methods or properties: 'Microsoft.Extensions.DependencyInjection.OptionsBuilderExtensions.ValidateOnStart<TOptions>(Microsoft.Extensions.Options.OptionsBuilder<TOptions>)' and 'Microsoft.Extensions.DependencyInjection.OptionsBuilderExtensions.ValidateOnStart<TOptions>(Microsoft.Extensions.Options.OptionsBuilder<TOptions>)'    MyType.MyMethod

There are likely breaking changes in these MS.Ext packages between major versions so I wouldn't expect v8 versions to be compatible on net7.0 and net6.0 projects without conflicts (either compile time or runtime).

What I would expect to see for a library that tries to be compatible across the major TFM, and have MS.Ext dependencies, would to have such dependencies locked to the major version shipped with that SDK.

For example:

net6.0
Microsoft.Extensions.DependencyInjection.Abstractions (>= 6.0.0)
Microsoft.Extensions.Hosting.Abstractions (>= 6.0.0)
Microsoft.Extensions.Logging.Abstractions (>= 6.0.0)
Serilog (>= 3.1.1)
Serilog.Extensions.Logging (>= 6.0.0)

net7.0
Microsoft.Extensions.DependencyInjection.Abstractions (>= 7.0.0)
Microsoft.Extensions.Hosting.Abstractions (>= 7.0.0)
Microsoft.Extensions.Logging.Abstractions (>= 7.0.0)
Serilog (>= 3.1.1)
Serilog.Extensions.Logging (>= 7.0.0)

net8.0
Microsoft.Extensions.DependencyInjection.Abstractions (>= 8.0.0)
Microsoft.Extensions.Hosting.Abstractions (>= 8.0.0)
Microsoft.Extensions.Logging.Abstractions (>= 8.0.0)
Serilog (>= 3.1.1)
Serilog.Extensions.Logging (>= 8.0.0)

Here is an example project that does it in a way that works: https://www.nuget.org/packages/Duende.AccessTokenManagement and the corresponding csproj: https://github.com/DuendeSoftware/Duende.AccessTokenManagement/blob/main/src/Duende.AccessTokenManagement/Duende.AccessTokenManagement.csproj

Cheers!

nblumhardt commented 9 months ago

Hey Damian! Thanks for the notes.

This is something we've previously spent some time grappling with; the Duende approach was what this package used originally, but that also has some downsides as dependency graphs get more complex.

If you're targeting .NET 6 and the v6 dependencies, you will need the 5.0.1 version of this package (since we switched versioning strategies at the .NET 7 release); in the future, whatever major version of Microsoft.Extensions.Hosting you're targeting, you can rely on the same major version of this package being compatible.

damianh commented 9 months ago

Thanks for quick response.

Understood, if that's the approach then I suggest limiting the supported frameworks to just the lasted major TFM only. Because the current version declares compatibility with net6.0 and net7.0, folks using dependabot and ide package managers will be steered to upgrade on net6.0 and net7.0 project which then breaks things on them with incompatible transitive dependencies being pulled in.

I suggest an 8.0.1 package with net8.0 TFM support only and delisting of the 8.0.0 package(s).

nblumhardt commented 9 months ago

Thanks for the follow-up!

The Microsoft.Extensions.* 8.x packages do actually target those downlevel platforms, and some people use them successfully that way; here's the MEL framework support list, which includes essentially the same TFMs as we use here.

https://www.nuget.org/packages/Microsoft.Extensions.Logging

The path Serilog.Extensions. follows is to just do exactly what those packages do, so we don't add or remove any compatibility problems. The trick really is just targeting the the Serilog.Extensions. package versions that match the Microsoft.Extensions.* versions that your app uses.

damianh commented 9 months ago

I can detect that you don't desire making the adjustments here. That's cool, please close 😎

It is a broken experience, but it'll go away for me when my particular apps are on .NET8 (until the same thing happens when .NET9 comes around).

nblumhardt commented 9 months ago

The feedback's definitely welcome - thanks for raising this, all the same 😄. It's an area that's already had a lot of time put into it, chasing different compat problems over the years, so not super keen to reopen the can of worms - but keeping an eye on it 👍