Xabaril / AspNetCore.Diagnostics.HealthChecks

Enterprise HealthChecks for ASP.NET Core Diagnostics Package
Apache License 2.0
4.06k stars 793 forks source link

Consider adding explicit target framework versions to all packages #2180

Closed DamianEdwards closed 6 months ago

DamianEdwards commented 6 months ago

Issues like #2084 and #2163 are caused by the fact that the packages shipped from this repo only include a netstandard2.0 assembly. While the guidance on this isn't explicit, adding explicit net6+ targets for packages that contain a netstandard2.0 target and have package dependencies, helps to avoid this class of problem.

We just discovered that the Aspire components are hitting this issue which is causing health checks for some components to fail at runtime due to assembly version mismatches.

unaizorrilla commented 6 months ago

Hi @DamianEdwards

I would try add the explicit target fx this a soon as posible!

thanks

eerhardt commented 6 months ago

FYI - @adamsitnik @viktorhofer @joperezr @ericstj @terrajobst

I wonder if that guidance doc needs updating.

DO include a net6.0 target or later if you require new APIs introduced in a modern .NET.

This statement should be stronger. The case here is that technically these libraries need new APIs introduced in a modern .NET (IAsyncEnumerable). But since we shipped the Microsoft.Bcl.AsyncInterfaces nuget, it makes it seem like those APIs work just fine in netstandard. But the issue is that the library (ex. System.Text.Json) that is bringing in the reference to Microsoft.Bcl.AsyncInterfaces is dropping that dependency in the net8.0 TFM. Which allows this situation to happen. The HealthChecks.* libraries are compiled against the 8.0.0 version of Microsoft.Bcl.AsyncInterfaces, but the nuget dependency is getting dropped in a net8.0 app, which allows other dependencies to bring in lower versions of Microsoft.Bcl.AsyncInterfaces.

At this point, I think that guidance doc should just say:

✔️ DO start with including a net6.0 target.

and go from there.

ericstj commented 6 months ago

What we've recommended to folks is to have an explicit reference to AsyncInterfaces if you need it. Don't rely on it coming transitively. It's unfortunately difficult to enforce that.

adamsitnik commented 6 months ago

@DamianEdwards @eerhardt who is going to provide the fix? I am very busy with BinaryFormatter removal now. I suggest somebody from the Aspire Team provides the fix, I review the PR and @unaizorrilla releases new packages.

unaizorrilla commented 6 months ago

Ok, let me know to help if necessary

eerhardt commented 6 months ago

I am going to work on fixing this.

terrajobst commented 3 months ago

At this point, I think that guidance doc should just say:

✔️ DO start with including a net6.0 target.

I don't think that's the right fix because there is nothing specific to .NET 6 here. The .NET Framework case is different due to .NET Framework 4.6.1 not really having .NET Standard 2.0 support due to missing facades/type forwards.

Your issue seems to be caused by relying on transitive dependencies which is fragile specifically for the reason you cited.

eerhardt commented 3 months ago

I don't think that's the right fix because there is nothing specific to .NET 6 here.

My point was: we should be recommending to start with net6.0, and only add netstandard2.0 support if your library needs to run on .NET Framework. Then you would be multi-targeting net6.0;netstandard2.0 all the time when you needed to support both .NET 6+ and .NET Framework.

Your issue seems to be caused by relying on transitive dependencies which is fragile specifically for the reason you cited.

But nothing in our tooling flags this as a problem.

In general, just targeting netstandard2.0 is not a pit of success. If your library needs to run on both, you should be multi-targeting to all 3: net6.0;netstandard2.0;net462 (assuming you want other netstandard2.0 libraries to reference you). That will give you the most success. (it's what we do in all our libraries from runtime and aspnetcore.)

terrajobst commented 3 months ago

My point was: we should be recommending to start with net6.0, and only add netstandard2.0 support if your library needs to run on .NET Framework.

Ah. That's indeed a fair point. Let me chew on this.

But nothing in our tooling flags this as a problem.

That is also fair albeit just one of the many ways our tooling is lacking :-(