Build ServiceDiscovery library and tests against netstandard2.0 #10470
Conversation
twsouthwick
force-pushed
the
servicediscovery-fx
branch
from
adb78e1 to
aa30c55
Compare
This enables scenarios to use the library on .NET Framework. This is mostly done by using C# 14 new extension syntax so very little code changes were made but rather the APIs that didn't exist are added into FrameworkExtensions classes to light them up on .NET Core builds.
twsouthwick
force-pushed
the
servicediscovery-fx
branch
from
aa30c55 to
17f3a62
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR enables the ServiceDiscovery library and its tests to build and run on .NET Framework by multi-targeting the projects and providing polyfills for newer APIs via C# 14 extension syntax.
- Multi-target key projects and tests against net462/net472 alongside existing frameworks.
- Added a shared
FxPolyfillsfolder with extension methods to surface missing .NET Core APIs on .NET Framework. - Updated test and service discovery projects to conditionally include additional references and imports.
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Shared/TemplatesTesting/Aspire.Shared.TemplatesTesting.targets | Added IncludeTestUtilities property and removed direct test utilities reference |
| tests/Microsoft.Extensions.ServiceDiscovery.Tests/…Tests.cs | Suppressed a type conflict warning around the fake resolver factory |
| tests/**/*.csproj | Switched to multi-targeting net472 on Windows and added framework-specific package imports |
| tests/Aspire.TestUtilities/Aspire.TestUtilities.csproj | Multi-target net472 and import polyfills targets for framework support |
| src/Shared/FxPolyfills/*.cs | Introduced extension polyfills for various System and threading APIs on .NET Framework |
| src/Shared/FxPolyfills/FxPolyfills.targets | Configured inclusion/exclusion of polyfill sources based on targeting |
| src/Microsoft.Extensions.ServiceDiscovery*.csproj | Multi-target net462, conditional AOT flag, reference Bcl packages, and import polyfills |
| src/Microsoft.Extensions.ServiceDiscovery/ServiceDiscoveryHttpClientBuilderExtensions.cs | Wrapped gRPC-disable filter in #if NET for proper cross-target behavior |
| eng/Versions.props & Directory.Packages.props | Added and updated version properties for newly referenced packages |
Comments suppressed due to low confidence (3)
tests/Shared/TemplatesTesting/Aspire.Shared.TemplatesTesting.targets:18
- The ProjectReference to
Aspire.TestUtilitieswas removed, which may break resolution of theRequiresDockerattribute and other test utilities. Please re-add or replace this reference so that the shared test utilities are included in the build.
</ItemGroup>
src/Shared/FxPolyfills/Task.cs:20
- Add unit tests for
FxPolyfillTask.ConfigureAwaitcovering eachConfigureAwaitOptionspath to ensure the polyfill behaves identically to its .NET Core counterpart.
public async Task ConfigureAwait(ConfigureAwaitOptions options)
src/Shared/FxPolyfills/Task.TimeProvider.cs:12
- Consider adding tests for
FxPolyfillTask.WaitAsyncto validate timeout and cancellation behavior on .NET Framework scenarios.
public Task WaitAsync(CancellationToken token)
|
These polyfills are excellent. I wish there was an official MS polyfill for net4x, to prevent multiple copies of classes/code in each dll. |
...ions.ServiceDiscovery.Abstractions/Microsoft.Extensions.ServiceDiscovery.Abstractions.csproj
Outdated
Show resolved
Hide resolved
|
|
||
| <!-- Include all Framework Extension files from the shared location, excluding test-specific ones --> | ||
| <ItemGroup Condition="$(IsTargetingNetFramework)"> | ||
| <Reference Include="System.Net.Http" /> |
There was a problem hiding this comment.
NIT: should this go directly into projects that need it as opposed to globally? It helps with readability when references are all in the same project file.
There was a problem hiding this comment.
I put it here because these are usings that are already automatically added to the projects for .net 8/9, so it continues to be transparent to the consuming library.
|
|
||
| <PropertyGroup> | ||
| <TargetFramework>$(DefaultTargetFramework)</TargetFramework> | ||
| <TargetFrameworks>$(DefaultTargetFramework);net472</TargetFrameworks> |
There was a problem hiding this comment.
Why some are net462 and some net472?
There was a problem hiding this comment.
net462 is the minimum supported framework for the runtime libraries so makes sense to continue supporting that. However, xunit and other testing utilities have a minimum of net472
| <PropertyGroup> | ||
| <TargetFramework>$(DefaultTargetFramework)</TargetFramework> | ||
| <TargetFrameworks>$(DefaultTargetFramework)</TargetFrameworks> | ||
| <TargetFrameworks Condition="$([MSBuild]::IsOSPlatform('Windows'))">$(TargetFrameworks);net472</TargetFrameworks> |
There was a problem hiding this comment.
Why do we condition this here but not in the other test project?
There was a problem hiding this comment.
Because this is an exe and won't run on non-windows while the other is just a build time time. If there's a way to remove it from test runs on non-windows, we can still build it for net472, I've just always done it this way.
|
Most of the comments are nits, but this looks really good otherwise, thanks @twsouthwick! I don't love the name of FxPollyfills, but I also don't have a good alternative 😄 |
I mentioned in the original comment, but DNS requires quite a bit more for the polyfills so my plan was to make sure the pattern was OK before including that. YARP only supports .NET 6+, so it can't be supported there |
|
why net462 and not just 48? |
I would suggest targeting the lowest version of netfx still in support, which is net462 for now. |
|
I realized we could just target netstandard2.0 which can help with migration efforts even more. I've pushed a change that removes explicitly targeting net462 with netstandard2.0. I'm not sure what the current recommended practice for multi-targeting is, so I can add the net462 if there is a reason to be explicit about it. |
twsouthwick
changed the title
twsouthwick
changed the title
I think it would be good to bring it back. We have public docs somewhere that w recommend library devs to consider also targeting netfx explicitly when targeting ns2.0 mainly to help avoid binding redirect issues. I somehow can't find those docs now, but for that reason it might be good to add it back. |
There was a problem hiding this comment.
Other than adding back the netfx target, this LGTM. Thanks @twsouthwick and sorry for the wait!
An add on from dotnet#10470 that added support for netstandard2.0, this adds an explicit net472 target which is part of the recommendation for multi-targetd libraries.
An add on from dotnet#10470 that added support for netstandard2.0, this adds an explicit net472 target which is part of the recommendation for multi-targetd libraries.
An add on from dotnet#10470 that added support for netstandard2.0, this adds an explicit net462 target which is part of the recommendation for multi-targetd libraries.
An add on from dotnet#10470 that added support for netstandard2.0, this adds an explicit net462 target which is part of the recommendation for multi-targetd libraries.
An add on from dotnet#10470 that added support for netstandard2.0, this adds an explicit net462 target which is part of the recommendation for multi-targetd libraries.
An add on from #10470 that added support for netstandard2.0, this adds an explicit net462 target which is part of the recommendation for multi-targeted libraries.
An add on from #10470 that added support for netstandard2.0, this adds an explicit net462 target which is part of the recommendation for multi-targeted libraries.
Description
This enables scenarios to use the library on .NET Framework. This is mostly done by using C# 14 new extension syntax so very little code changes were made but rather the APIs that didn't exist are added into FrameworkExtensions classes to light them up on .NET Core builds.
NOTE: This updates the abstractions and main ServiceDiscovery library, but does not enable the DNS nor YARP. YARP isn't supported on framework, so that isn't necessary. The DNS has a larger change due to more APIs it's using that don't exist, so once this PR gets in I can push that up if this pattern works.
Checklist
<remarks />and<code />elements on your triple slash comments?doc-ideatemplatebreaking-changetemplatediagnostictemplate