launchdarkly / dotnet-logging

Logging abstraction used by LaunchDarkly .NET-based libraries
Other
1 stars 2 forks source link

Conditional Microsoft.Extensions.Logging.Abstractions inclusion based on netcoreapp2.1 targeting #10

Open Eric-Bonneau opened 2 years ago

Eric-Bonneau commented 2 years ago

Hi there,

We're having some issues right now with a netcoreapp2.2 app with Microsoft.NET.Sdk.Web as project using LaunchDarkly.ServerSdk version 6.3.2. This server SDK version uses LaunchDarkly.Logging version 1.0.1. From what I can see on this line, it looks like you're explicitly including Microsoft.Extensions.Logging.Abstractions at version 3.1.9 for apps using netcoreapp2.1. I'm not exactly sure why this is affecting my app that is using netcoreapp2.2, but nevertheless, the error that I'm seeing is:

Severity    Code    Description Project File    Line    Suppression State
Error   NU1107  Version conflict detected for Microsoft.Extensions.Logging.Abstractions. Install/reference Microsoft.Extensions.Logging.Abstractions 3.1.9 directly to project MyProject to resolve this issue. 
 MyProject -> LaunchDarkly.ServerSdk 6.3.2 -> LaunchDarkly.Logging 1.0.1 -> Microsoft.Extensions.Logging.Abstractions (>= 3.1.9) 
 MyProject -> Microsoft.AspNetCore.App 2.2.0 -> Microsoft.Extensions.Logging.Abstractions (>= 2.2.0 && < 2.3.0).        

If I add Microsoft.Extensions.Logging.Abstractions version 3.1.9 directly to my project, I get this warning when building:

warning NU1608: Detected package version outside of dependency constraint: Microsoft.AspNetCore.App 2.2.0 requires Microsoft.Extensions.Logging.Abstractions (>= 2.2.0 && < 2.3.0) but version Microsoft.Extensions.Logging.Abstractions 3.1.19 was resolved.

This builds, but ultimately leads to runtime errors.

From what I've understood, these Microsoft.Extensions.* packages are versioned in line with the current .NET Core (and now .NET) versioning scheme. So I would expect that if you're doing conditional targeting for netcoreapp2.1, the package version should be the latest within the 2.1. line (or just use 2.1. within the .csproj to get the latest version).

Am I misunderstanding something?

I know you don't officially support netcoreapp2.2, but I feel like there must be some way to have conditional targeting to allow for 2. versions of the Microsoft.Extensions.Logging.Abstractions package. It would really help to unblock me, and possibly others in the 2. version line of .NET Core.

I suspect this issue will mostly go away in .NET Core 3.* and up since it seems like you no longer need to add these packages explicitly; they are added automatically as part of the Microsoft.NET.Sdk.Webproject.

Please let me know what you think, or if there is anything I can do to help.

Thanks!

eli-darkly commented 2 years ago

To answer the easiest question first, even though it's not the main one:

I'm not exactly sure why this is affecting my app that is using netcoreapp2.2

Target frameworks and dependencies in the project file for a package apply to that package, not to the application that's using it. The conditional stuff in our project file that refers to netcoreapp2.1 is talking about that target framework build of our package, not of your application. We don't have a 2.2 build, we have a 2.1 build, and a 2.2 application will end up using our 2.1 build because that is the next best compatible one. That's just how .NET compatibility works— there is no need to "officially support netcoreapp2.2" because the versions are forward-compatible.

eli-darkly commented 2 years ago

As for why we have a reference to 3.1.9 there, rather than to a 2.x version— I'm not sure. It might have been accidental, that is, maybe it simply picked up a recent compatible version at the time our project was created and we kept that.

It's not the case that M.E.L.A. 3.x only works with .NET Core 3.1; if you look at the Frameworks tab here, its target framework is .NET Standard 2.0 which can be used by all .NET Core versions and most other frameworks as well. The problem isn't the runtime framework but, as you said, the specific package Microsoft.AspNetCore.App which has a stricter version constraint. And off the top of my head unfortunately I can't think of a good workaround. In .NET Framework you would use a binding redirect, but you can't do that in .NET Core.

So I think we would probably need to do a new release to fix this. Technically .NET Core 2.x is already EOL and so we will be completely dropping support for it in a future SDK release, but our current SDK version that was 2.x-compatible (or at least was meant to be) is still supported and so it's still our policy to fix any issues it currently has.

Right now I'm trying to think through what the right approach is to versioning such a fix. I think it depends on whether this could have ever worked for .NET Core 2.x apps. If it could not have (i.e. you're the first to have tried), then that's just a bugfix and so it's easy: we can put out a patch version that updates that one dependency. However... since the problem (I think) is more specifically with the dependency constraints of ASP.NET Core, it might be that a .NET Core 2.x app that did not use ASP.NET could have worked with our current package dependencies, in which case we would be breaking such an app if we changed our dependency version... so, by semantic versioning rules, we would need to have that be a major version release of LaunchDarkly.Logging. I think we can get away with having it only be a patch release of LaunchDarkly.ServerSdk though.

eli-darkly commented 2 years ago

it might be that a .NET Core 2.x app that did not use ASP.NET could have worked with our current package dependencies

In fact that is definitely the case, since our current CI builds include one that runs in .NET Core 2.1. So, the rest of what I said above is applicable.

Eric-Bonneau commented 2 years ago

Target frameworks and dependencies in the project file for a package apply to that package, not to the application that's using it. The conditional stuff in our project file that refers to netcoreapp2.1 is talking about that target framework build of our package, not of your application. We don't have a 2.2 build, we have a 2.1 build, and a 2.2 application will end up using our 2.1 build because that is the next best compatible one. That's just how .NET compatibility works— there is no need to "officially support netcoreapp2.2" because the versions are forward-compatible.

Okay, thanks for explaining that. It makes it a lot clearer. 👍

In fact that is definitely the case, since our current CI builds include one that runs in .NET Core 2.1. So, the rest of what I said above is applicable.

Yes, it does seem to only show warnings for me with ASP.NET Core projects (2.1 and 2.2). I tried making a minimal project to try to reproduce the runtime issues I'm encountering, but I haven't been able to. So there might be some other kind of package incompatibilities that I'm encountering in my larger project.