fix(csharp): build net472 on all platforms so the Testing package ships it - #4605
Conversation
…ps it Apache.Arrow.Adbc.Testing guarded its net472 target framework behind IsWindows. Packaging runs in a Linux container (compose.yaml uses mcr.microsoft.com/dotnet/sdk:10.0), so TargetFrameworks collapsed to net8.0;net10.0 there and the package published to nuget.org has been missing its lib/net472 folder. It is the only test project on the release push list in dev/release/post-07-csharp.sh, so this affects consumers targeting .NET Framework. Target net472 unconditionally instead, matching what the src/Drivers projects already do, so the package contents no longer depend on which platform packs it. net472 still cannot be *run* off Windows, as there is no .NET Framework test host there, so csharp_test.sh now runs the other target frameworks individually on Linux and macOS. The list is read back from the project rather than hardcoded, so adding a target framework later cannot silently drop it from CI. xunit.runner.visualstudio 3.1.5 is used for the .NET targets and 2.8.2 for net472, selected by condition in Directory.Packages.props. Two related CI fixes: - csharp.yml requested a single SDK per matrix leg while the project multi-targets net8.0 and net10.0. The .NET 8 SDK cannot build net10.0, so that leg only worked because the runner image happened to preinstall .NET 10. Install both SDKs in every job and drop the now-redundant dotnet axis, which was producing identical jobs. - Benchmarks is packable by default and produced a Benchmarks.nupkg that was built into the release artifacts but never published. Mark it IsPackable=false. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes the C# Apache.Arrow.Adbc.Testing NuGet package contents so it consistently includes the net472 build regardless of the OS used for packing (important because release packaging runs in Linux containers), and updates CI/scripts accordingly to avoid attempting to run net472 tests on non-Windows.
Changes:
- Make
Apache.Arrow.Adbc.Testingtargetnet472unconditionally solib/net472is always produced during pack. - Update C# test script to run per-target-framework on Linux/macOS while skipping execution for
net472. - Adjust C# CI to install both .NET 8 and .NET 10 SDKs in every job and prevent
Benchmarksfrom producing an unpublished.nupkg.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| csharp/test/Apache.Arrow.Adbc.Tests/Apache.Arrow.Adbc.Testing.csproj | Unconditionally multi-targets net8.0;net10.0;net472 so packing always emits lib/net472. |
| csharp/Directory.Packages.props | Selects xunit.runner.visualstudio version by TFM to keep net472 compatible. |
| csharp/Benchmarks/Benchmarks.csproj | Marks benchmarks project as not packable to avoid generating unused release artifacts. |
| ci/scripts/csharp_test.sh | Skips running net472 tests on non-Windows while still running other TFMs from the project’s declared list. |
| .github/workflows/csharp.yml | Installs both .NET 8 and .NET 10 SDKs in each matrix job and removes redundant dotnet-version axis. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Darn it 😅 Should we validate this in the release verification? I've been thinking it might be more important to validate that all the binaries are present and working |
Yeah :/.
Yeah, maybe. arrow-dotnet has a nice thing it does (courtesy of @kou, I think) where it rebuilds the test projects using PackageReferences instead of ProjectReferences and runs the tests against the generated nupkg files. I'll look at porting that over here in the next two weeks. |
|
I think the CI failures are incidental. |
Well that's a relief. I was pretty stumped. |
|
It was done by @adamreeve :-) |
Apache.Arrow.Adbc.Testing guarded its net472 target framework behind IsWindows. Packaging runs in a Linux container (compose.yaml uses mcr.microsoft.com/dotnet/sdk:10.0), so TargetFrameworks collapsed to net8.0;net10.0 there and the package published to nuget.org has been missing its lib/net472 folder. It is the only test project on the release push list in dev/release/post-07-csharp.sh, so this affects consumers targeting .NET Framework.
Target net472 unconditionally instead, matching what the src/Drivers projects already do, so the package contents no longer depend on which platform packs it.
net472 still cannot be run off Windows, as there is no .NET Framework test host there, so csharp_test.sh now runs the other target frameworks individually on Linux and macOS. The list is read back from the project rather than hardcoded, so adding a target framework later cannot silently drop it from CI.
xunit.runner.visualstudio 3.1.5 is used for the .NET targets and 2.8.2 for net472, selected by condition in Directory.Packages.props.
Two related CI fixes:
csharp.yml requested a single SDK per matrix leg while the project multi-targets net8.0 and net10.0. The .NET 8 SDK cannot build net10.0, so that leg only worked because the runner image happened to preinstall .NET 10. Install both SDKs in every job and drop the now-redundant dotnet axis, which was producing identical jobs.
Benchmarks is packable by default and produced a Benchmarks.nupkg that was built into the release artifacts but never published. Mark it IsPackable=false.
Closes #4604