serilog / serilog-settings-configuration

A Serilog configuration provider that reads from Microsoft.Extensions.Configuration
Apache License 2.0
446 stars 129 forks source link

Using different versions of Microsoft.Extensions.DependencyModel base… #339

Closed hypdeb closed 1 year ago

hypdeb commented 1 year ago

Using different versions of Microsoft.Extensions.DependencyModel based on target framework. This resolves warnings when using package as part of solution where another package requires a more recent version of Microsoft.Extensions.DependencyModel.

SimonCropp commented 1 year ago

IMO we should just update to Microsoft.Extensions.DependencyModel v7. with no conditions. if people want to use older versions they can use older nugets

nblumhardt commented 1 year ago

I'm coming around to that point of view too, @SimonCropp.

To do that well, we'd need to synchronize major version numbers here with those in Serilog.Extensions.Logging, .Hosting, and Serilog.AspNetCore - i.e. make it consistent across all of the packages tied to the app model. Can you think of anything that's likely to spoil this kind of plan?

sungam3r commented 1 year ago

if people want to use older versions they can use older nugets

This is a wrong interpretation. People do not want to use old versions, sometimes they are forced to use them due to various restrictions. It’s simply pointless to increase the major version of dependency without a good reason.

SimonCropp commented 1 year ago

how will this work if someone has to use Microsoft.Extensions.DependencyModel v3 on net7

hypdeb commented 1 year ago

Anyways, it doesn't seem possible to support .net7.0 directly currently due to AppVeyor's Visual Studio images not supporting it. I reverted those changes.

SimonCropp commented 1 year ago

also how will this work if someone has to use Microsoft.Extensions.DependencyModel v7 on net4.6.2

hypdeb commented 1 year ago

also how will this work if someone has to use Microsoft.Extensions.DependencyModel v7 on net4.6.2

The same thing that is happening now for all framework versions: you get a warning that there's a conflict that can't be resolved between 3.0.0 required by this library and 7.0.0 required by you or some other library.

Moreover, trying to use Microsoft.Extensions.DependencyModel 7.0.0 in a net462 app will produce another warning that the library doesn't support this target framework / has not been tested with it.

kavun commented 1 year ago

@hypdeb, you should be able to add net7.0 back now 🥳 https://www.appveyor.com/updates/ image

hypdeb commented 1 year ago

@kavun thanks, sorry for the delay to apply this change. But I get one failing test locally with .net 6 and .net 7. I will try to have a look tonight.

And .NET 7 is not in the Ubuntu image so it fails there. Therefore this will have to wait some more :)

glen-84 commented 1 year ago

Will this be merged? Seeing a wall of warnings during builds because of this.

hypdeb commented 1 year ago

Sorry I kind of forgot about this because the tooling used to build this project did not support the latest version of .NET. I'll try to have another look now.

hypdeb commented 1 year ago

Sadly some tests still fail. It seems some non-backwards compatible change was made in DependencyModel.

image
glen-84 commented 1 year ago

Seems like only one test – MixedMinimumLevelCorrectOneIsEnabledOnLogger.

hypdeb commented 1 year ago

Trying to understand how this package is used. Any input is welcome as I don't really have a clue haha.

glen-84 commented 1 year ago

This is actually a recently added test by @HexHunter97. Maybe they can help.

0xced commented 1 year ago

The error in the MixedMinimumLevelCorrectOneIsEnabledOnLogger test is unrelated to the Microsoft.Extensions.DependencyModel package version.

Only adding net6.0 in the TargetFrameworks of Serilog.Settings.Configuration.csproj is enough to produce this error.

I don't understand why (yet)!

hypdeb commented 1 year ago

@0xced Thanks for your help! I was already digging in the wrong direction.

HexHunter97 commented 1 year ago

I'll try take a look in a few hours once free from work to confirm, but my suspicion is this:

It probably has something to do with the preprocessor directives in the ConfigurationReader and ConfigurationReaderTests not matching. At the time I wrote this piece of code in 2020, there was only NETSTANDARD, NET452 and NET461 versions of the code and didn't think of this potential conflict and didn't know about MS's plans to fully switch to .NET.

https://github.com/serilog/serilog-settings-configuration/blob/ab8764198c7fc784f1a02f3c2d02e53532204941/src/Serilog.Settings.Configuration/Settings/Configuration/ConfigurationReader.cs#L23

https://github.com/serilog/serilog-settings-configuration/blob/ab8764198c7fc784f1a02f3c2d02e53532204941/test/Serilog.Settings.Configuration.Tests/ConfigurationReaderTests.cs#L225

A fix would be to add NET as the preprocessor directive in ConfigurationReader.

So as said, I'll confirm this in a few hours.

0xced commented 1 year ago

Very good point!

I always try to make conditional compilation future proof. See https://github.com/spectreconsole/spectre.console/pull/563 for an example how to achieve this goal. The same principles should be applied in Serilog.Settings.Configuration too.

0xced commented 1 year ago

Yep, that's it! Replacing #if NETSTANDARD || NET461 with #if !NET451 everywhere fixes the failing test when adding the .NET 6 target framework.

HexHunter97 commented 1 year ago

This one's on me completely. But you live & learn. 😄 Apologies for the headaches guys!

hypdeb commented 1 year ago

Indeed, you could also add || NET6_OR_GREATER

hypdeb commented 1 year ago

But @0xced 's solution is more explicit.

hypdeb commented 1 year ago

All green now! Can someone give it a final review and merge it?

0xced commented 1 year ago

I'm still not understanding why this change is needed. I could not reproduce the warning you are mentioning in the original issue.

Here's what I've tried.

Program.cs

using System;
using System.Reflection;
using Microsoft.Extensions.DependencyModel;

var version = typeof(DependencyContext).Assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()?.InformationalVersion;
Console.WriteLine($"Microsoft.Extensions.DependencyModel version: {version}");

TestWithScrutor.csproj

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net6.0</TargetFramework>
    <Nullable>enable</Nullable>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Scrutor" Version="4.2.1" />
    <PackageReference Include="Serilog.Settings.Configuration" Version="3.4.0" />
  </ItemGroup>

</Project>

dotnet run produces the following output, no restore nor compilation warning at all:

Microsoft.Extensions.DependencyModel version: 6.0.0+4822e3c3aa77eb82b2fb33c9321f923cf11ddde6

TestWithEFCoreSqlite.csproj

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net6.0</TargetFramework>
    <Nullable>enable</Nullable>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.EntityFrameworkCore.Sqlite.Core" Version="7.0.3" />
    <PackageReference Include="Serilog.Settings.Configuration" Version="3.4.0" />
  </ItemGroup>

</Project>

dotnet run produces the following output, no restore nor compilation warning at all:

Microsoft.Extensions.DependencyModel version: 7.0.0+d099f075e45d2aa6007a22b71b45a08758559f80

Serilog.Settings.Configuration declares the dependency as >= 3.0.0, Scrutor declares the dependency as >= 6.0.0 and Microsoft.EntityFrameworkCore.Sqlite.Core declares it as >= 7.0.0. The results here are totally expected, that's simply how the NuGet dependency resolution works.

Could you please share a sample project where warnings occur and share exactly what the warnings are?

0xced commented 1 year ago

OK now I see that Microsoft.Extensions.Configuration.Binder 2.0.0 and Microsoft.Extensions.DependencyModel 3.0.0 have been deprecated as part of the .NET Package Deprecation effort.

So I'd be in favour of simply dropping the net451 target framework, bumping both Microsoft.Extensions.Configuration.Binder and Microsoft.Extensions.DependencyModel to version 6.0.0 and removing all conditional compilation.

I just proposed this alternative solution in #351 which should supersede this pull request in my humble opinion.

hypdeb commented 1 year ago

@0xced Yes please, I didn't know this was an option!

glen-84 commented 1 year ago

@0xced

For us it started happening when we moved our tests to a separate project, and that test project referenced the main application project.

image

I need to check the list of dependencies again.