Fix Microsoft.Bcl.AsyncInterfaces issue #1980
Conversation
|
In other words net6-targeted app SHOULD NOT use |
|
Would you like a hand with this PR? :) |
|
I would like to hear considerations. I'm going to change all deps. |
|
Where is the dependency on |
|
Many CI failed for net6.0 with error I narrowed it down to
If I return Need a bit assistance here. |
|
@NielsPilgaard From our code only 1 place (abstractions though) |
|
@NielsPilgaard Ah, I misunderstood you. It came from |
|
Looks like an issue with coverlet.collector, I would like to deal with it before making further changes here. |
I only see How do we know Perhaps a stupid question, but do we really need code coverage? |
Microsoft.Extensions.Diagnostics.HealthChecks -> Microsoft.Extensions.Hosting.Abstractions -> Microsoft.Bcl.AsyncInterfaces
Code coverage is a widely used feature that helps to observe the level of testability. |
# Conflicts: # AspNetCore.Diagnostics.HealthChecks.sln # Directory.Build.props # build/versions.props # src/HealthChecks.AzureStorage/HealthChecks.AzureStorage.csproj # src/HealthChecks.Consul/HealthChecks.Consul.csproj # src/HealthChecks.EventStore.gRPC/HealthChecks.EventStore.gRPC.csproj # src/HealthChecks.Gremlin/HealthChecks.Gremlin.csproj # src/HealthChecks.Nats/HealthChecks.Nats.csproj
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ 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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@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 This PR is an example of what I expect to do - retarget packages to use different ping @adamsitnik @unaizorrilla for advice. |
sungam3r
added
dependencies
Good plan, I agree. |
|
Oh, I will need to check about 70 projects. |
How can I help? If you want help that is 😊 |
|
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. |
|
While working on that PR I saw that |
|
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. |
|
Personally I prefer being able to stay on the latest version, and not have to stick with a specific one matching my current TFM. |
|
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. |
|
So you think we should go with the approach you outlined earlier? |
|
I think so. #1975 (comment) should be applied not only for our clients but also for us because we are the clients of underlying MS packages. |
|
Well, actually #1975 (comment) 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. |
|
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 If care about platform compatibility: Option 2. Multitarget packages in Option 3. Target packages in 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. |
|
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 :) |
|
I am going to think out loud here.
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 We could give it a try and change all the packages to do the following: <PackageReference Include="Microsoft.Extensions.DependencyInjection" Version="$(MicrosoftExtensionsVersion)" />And in <!-- 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 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. |
|
The problem was fixed in #2185 |
fixes #1975
Actually this fix should be done for all packages. The core issue is
Microsoft.Extensions.DependencyInjectiondependency.TODO:
Microsoft.Extensions.DependencyInjectionv6 for net6 tfm and v7 for any other tfm.Rel: #1720
ping @NielsPilgaard
#1720 have fixed this issue in wrong way, alas.