Fix Microsoft.Bcl.AsyncInterfaces issue

[sungam3r]

fixes #1975

Actually this fix should be done for all packages. The core issue is Microsoft.Extensions.DependencyInjection dependency.

TODO:

1. Add/ensure net6 tfm for all packages
2. Use Microsoft.Extensions.DependencyInjection v6 for net6 tfm and v7 for any other tfm.

Rel: #1720

ping @NielsPilgaard

#1720 have fixed this issue in wrong way, alas.

[sungam3r]

In other words net6-targeted app SHOULD NOT use Microsoft.Extensions.DependencyInjection > v6.

[NielsPilgaard]

Would you like a hand with this PR? :)

[sungam3r]

I would like to hear considerations. I'm going to change all deps.

[NielsPilgaard]

Where is the dependency on Microsoft.Extensions.DependencyInjection coming from?

[sungam3r]

Many CI failed for net6.0 with error System.EntryPointNotFoundException : Entry point was not found. after I removed <PackageReference Include="Microsoft.Bcl.AsyncInterfaces" Version="7.0.0" Condition="'$(TargetFramework)' != 'net7.0'" /> line from Directory.Build.props.

I narrowed it down to --collect "XPlat Code Coverage" argument.

dotnet test ./test/HealthChecks.Gremlin.Tests/HealthChecks.Gremlin.Tests.csproj -f net6.0 works dotnet test ./test/HealthChecks.Gremlin.Tests/HealthChecks.Gremlin.Tests.csproj --collect "XPlat Code Coverage" -f net6.0 fails.

If I return <PackageReference Include="Microsoft.Bcl.AsyncInterfaces" Version="7.0.0" Condition="'$(TargetFramework)' != 'net7.0'" /> back everything works again but I don't understand why this package is still an issue. For net6 I aligned package versions to <7.

Need a bit assistance here.

[sungam3r]

@NielsPilgaard From our code only 1 place (abstractions though)

[sungam3r]

@NielsPilgaard Ah, I misunderstood you. It came from Microsoft.Extensions.Diagnostics.HealthChecks that is used by each HC package.

[sungam3r]

Looks like an issue with coverlet.collector, I would like to deal with it before making further changes here.

[NielsPilgaard]

@NielsPilgaard Ah, I misunderstood you. It came from Microsoft.Extensions.Diagnostics.HealthChecks that is used by each HC package.

I only see Microsoft.Extensions.DependencyInjection.Abstractions as a dependency to Microsoft.Extensions.Diagnostics.HealthChecks?

How do we know Microsoft.Extensions.DependencyInjection is the underlying issue here?

Perhaps a stupid question, but do we really need code coverage?

[sungam3r]

I only see Microsoft.Extensions.DependencyInjection.Abstractions as a dependency to Microsoft.Extensions.Diagnostics.HealthChecks?

Microsoft.Extensions.Diagnostics.HealthChecks -> Microsoft.Extensions.Hosting.Abstractions -> Microsoft.Bcl.AsyncInterfaces

Perhaps a stupid question, but do we really need code coverage?

Code coverage is a widely used feature that helps to observe the level of testability.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (73abc7a) 64.11% compared to head (30e1183) 64.86%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1980      +/-   ##
==========================================
+ Coverage   64.11%   64.86%   +0.75%     
==========================================
  Files         248      264      +16     
  Lines        8359     8583     +224     
  Branches      586      613      +27     
==========================================
+ Hits         5359     5567     +208     
- Misses       2852     2860       +8     
- Partials      148      156       +8     

Flag	Coverage Δ
ApplicationStatus	26.66% <ø> (ø)
ArangoDb	26.50% <ø> (ø)
Aws.S3	14.28% <ø> (ø)
Aws.SecretsManager	14.54% <ø> (ø)
Aws.Sns	14.77% <ø> (ø)
Aws.Sqs	15.28% <ø> (ø)
Aws.SystemsManager	14.54% <ø> (ø)
Azure.IoTHub	12.82% <ø> (ø)
AzureApplicationInsights	14.70% <ø> (ø)
AzureBlobStorage	25.17% <ø> (?)
AzureDigitalTwin	35.76% <ø> (ø)
AzureEventHubs	16.54% <ø> (?)
AzureFileStorage	25.17% <ø> (?)
AzureKeyVault	29.24% <ø> (ø)
AzureQueueStorage	25.17% <ø> (?)
AzureSearch	16.10% <ø> (ø)
AzureServiceBus	71.34% <ø> (ø)
AzureTableStorage	26.66% <ø> (?)
Consul	24.32% <ø> (+1.74%)	:arrow_up:
CosmosDb	28.10% <ø> (ø)
Dapr	14.50% <ø> (?)
DocumentDb	14.64% <ø> (ø)
DynamoDb	12.12% <ø> (ø)
Elasticsearch	39.49% <ø> (ø)
EventStore	62.08% <ø> (ø)
EventStore.gRPC	25.51% <ø> (ø)
Gcp.CloudFirestore	12.10% <ø> (ø)
Gremlin	25.00% <ø> (+1.92%)	:arrow_up:
Hangfire	10.97% <ø> (ø)
IbmMQ	30.76% <ø> (ø)
InfluxDB	15.00% <ø> (ø)
Kafka	22.03% <ø> (ø)
Kubernetes	41.54% <ø> (ø)
MongoDb	31.77% <ø> (ø)
MySql	31.63% <ø> (ø)
Nats	72.77% <ø> (+2.91%)	:arrow_up:
Npgsql	42.71% <ø> (+15.61%)	:arrow_up:
OpenIdConnectServer	39.28% <ø> (ø)
Oracle	60.60% <ø> (ø)
Prometheus.Metrics	29.80% <ø> (ø)
Publisher.ApplicationInsights	14.76% <ø> (ø)
Publisher.CloudWatch	19.02% <ø> (ø)
Publisher.Datadog	15.00% <ø> (ø)
Publisher.Prometheus	18.75% <ø> (ø)
Publisher.Seq	40.30% <ø> (ø)
RabbitMQ	47.67% <ø> (ø)
RavenDb	70.74% <ø> (ø)
Redis	65.71% <ø> (ø)
SendGrid	11.92% <ø> (ø)
SignalR	24.22% <ø> (ø)
SqlServer	28.57% <ø> (ø)
Sqlite	26.38% <ø> (ø)
System	42.56% <ø> (ø)
UI	65.68% <ø> (-0.17%)	:arrow_down:
Uris	0.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[sungam3r]

@NielsPilgaard Hi! A lot of time has passed since I was doing this. Now I am gradually returning to the support of this project. I resolved conflicts and see that now the tests are successful, although I have not changed coverlet version. Perhaps, NET8 SDK is the case. No matter how it was, we are returning to the initial question about which versions of Microsoft.Extensions.DependencyInjection to use for different TFMs. Bug with Microsoft.Bcl.AsyncInterfaces is rather annoying and I would like to make changes in all projects to fix it once and forever.

This PR is an example of what I expect to do - retarget packages to use different Microsoft.Extensions.DependencyInjection versions so each package on netX TFM uses Microsoft.Extensions.DependencyInjection vX. In case of netstandard2.0/2.1 TFMs I think it's worth to use the latest Microsoft.Extensions.DependencyInjection, i.e. 8, but I'm not sure.

ping @adamsitnik @unaizorrilla for advice.

[NielsPilgaard]

This PR is an example of what I expect to do - retarget packages to use different Microsoft.Extensions.DependencyInjection versions so each package on netX TFM uses Microsoft.Extensions.DependencyInjection vX. In case of netstandard2.0/2.1 TFMs I think it's worth to use the latest Microsoft.Extensions.DependencyInjection, i.e. 8, but I'm not sure.

Good plan, I agree. Also, using v8 for netstandard2.0/2.1 seems correct, I can't see a scenario where that would break anything.

[sungam3r]

Oh, I will need to check about 70 projects.

[NielsPilgaard]

Oh, I will need to check about 70 projects.

How can I help? If you want help that is 😊

[sungam3r]

If you want you may post a PR into my branch adding all that stuff in csproj files. I will not deal with this PR today or tomorrow but can review/merge.

[NielsPilgaard]

While working on that PR I saw that Microsoft.Bcl.AsyncInterfaces has been included in Microsoft.Extensions.Diagnostics.HealthChecks since v8.0.0, when targetting anything other than net8.0 - Since that's the case we should just be able to update to v8.0.0 across the board, and the bug should be resolved, no?

[sungam3r]

I'm not sure. Yes, it is included but it has version 8, so net6/net7 apps end up with v8 instead of v6. There are 2 questions - backward compatibility and consistency across platforms. I tend to be consistent and to not use v8 in net6/7 TFMs.

[NielsPilgaard]

Personally I prefer being able to stay on the latest version, and not have to stick with a specific one matching my current TFM.

[sungam3r]

In ideal world everyone wants this but these are cross restrictions on versions used in app which is combined of all packages in dependency tree.

[NielsPilgaard]

So you think we should go with the approach you outlined earlier?

[sungam3r]

I think so. https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/issues/1975#issuecomment-1837925404 should be applied not only for our clients but also for us because we are the clients of underlying MS packages.

[sungam3r]

Well, actually https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/issues/1975#issuecomment-1837925404 may say that v8 versions of health checks should work ONLY for net8 and for net6 one should use previous series of packages. @adamsitnik please clarify. If so we can completely separate targeting to different maintenance branches - v6, v7, v8. It is rather serious decision and subsequent work.

[sungam3r]

I end up with 3 options for versioning strategy.

If not care too much about platform compatibility:

Option 1. Always target the latest MS packages in master. For now 8.

If care about platform compatibility:

Option 2. Multitarget packages in master to use v6/v7/v8 series of MS packages, so our v8 supports v8 AND v7 AND v6, etc

Option 3. Target packages in master to use ONLY v8 series of MS packages, our v8 supports v8 (+netstandard). Target packages in v7 branch to use ONLY v7 series of MS packages, our v7 supports v7 (+netstandard). Target packages in v6 branch to use ONLY v6 series of MS packages, our v6 supports v6 (+netstandard).

Option 3 IMO looks more attractive from a client point of view. It copies MS versioning strategy for .NET packages. The drawback is that we need to backport changes between branches. This is tiring work.

Option 2 is almost where we are for now but further changes required to be fully compatible with such strategy.

In any case chosen option should be explicitly documented in README.

[NielsPilgaard]

To reduce complexity and maintenance load I prefer option 1, but I agree with your analysis from the consumer's point of view.

I'll hold off on making a PR until a decision has been made :)

[adamsitnik]

I am going to think out loud here.

1. Each health check package depends on Microsoft.Extensions.DependencyInjection (and maybe some other Microsoft.Extensions packages) and the client library package(s). The first follow .NET versioning scheme, the latter sometimes do (example: Npgsql), but most of them don't.
2. Microsoft.Extension packages should not introduce breaking API changes (old code should compile just fine), but they may drop older TFM support (for now it's very rare, but one day .NET Team is going to stop supporting .NET Standard).
3. Why users may want to update to latest version of Health Check packages: a) get a bug fix for a bug in the health check package. b) update to new .NET and stop using two different versions of Microsoft.Extensions packages c) get a bug fix for MS.E or client library package (the users can just add the reference to newer version in explicit way and everything is going to work fine).

If every package targets different MS.E version depending on the TFM, we don't need to backport bug fixes to older versions, but we may need to use #if when using new features for new TFMs. It may also require changes to the README.md files for cases when we recommend using new features like today with keyed DI to register multiple instances of the same type. If every package targets only latest TFM, we should backport every bug fix to previous version. It's time consuming and requires extra work. But it's rare.

We could give it a try and change all the packages to do the following:

<PackageReference Include="Microsoft.Extensions.DependencyInjection" Version="$(MicrosoftExtensionsVersion)" />

And in Directory.Build.props:

  <!-- out-of-band packages that are not included in NetCoreApp have TFM-specific versions -->
  <Choose>
    <When Condition="'$(TargetFramework)' == 'net6.0'">
        <PropertyGroup>
            <MicrosoftExtensionsVersion>6.0.1</MicrosoftExtensionsVersion>
        </PropertyGroup>
    </When>
    <When Condition="'$(TargetFramework)' == 'net7.0'">
        <PropertyGroup>
            <MicrosoftExtensionsVersion>7.0.0</MicrosoftExtensionsVersion>
        </PropertyGroup>
    </When>
    <When Condition="'$(TargetFramework)' == 'net8.0'">
        <PropertyGroup>
            <MicrosoftExtensionsVersion>8.0.0</MicrosoftExtensionsVersion>
        </PropertyGroup>
    </When>
  </Choose>

But I am not sure how to handle .NET Standard TFM here. I guess it should target the latest version??

If we are lucky, the update to .NET 9 would require:

<When Condition="'$(TargetFramework)' == 'net9.0'">
    <PropertyGroup>
        <MicrosoftExtensionsVersion>9.0.0</MicrosoftExtensionsVersion>
    </PropertyGroup>
</When>

But.. introducing such change would be a braking change for all the users who target older TFMs, updated to our 8.0.0 packages and now would update to 8.1.0 which would change the version of referenced MS.E packages. But I don't think that it's common and that it would cause any actual trouble.

I think that we could try to apply these changes to 10 most popular packages and see how it goes. Then based on the complexity of required changes decide whether we want to introduce such changes.