Restore compatibility with .NET 9.0#208
Conversation
Since appveyor doesn’t have .NET 10 we need to also publish for .NET 9 This commit makes sure we are building and testing against both everywhere. To make this simpler I have removed the flags from the `Makefile` and let the target versions be defined canonically in the `csproj` files. The only change to make .NET 9.0 work was specifying `<LangVersion>14.0</LangVersion>` when building the tests. .NET 9.0 uses CSharp version 13.0, which is missing some of the assertion features we need in the tests (probably since dropping NUnit). But I think this should be fine because Appveyor will be using the packaged code, not compiling it from source.
See #202 for changes
There was a problem hiding this comment.
Pull request overview
This PR aims to restore build/test/publish compatibility on environments that don’t have a .NET 10 SDK available (notably AppVeyor) by multi-targeting .NET 9 in addition to .NET 10, and simplifying build commands to rely on canonical target frameworks in the project files.
Changes:
- Multi-target
GovukNotifyandGovukNotify.Testsfornet9.0andnet10.0. - Remove explicit framework overrides in the
Makefilesodotnet build/test/packuses the TFMs defined in*.csproj. - Update changelog with a new
9.0.0entry describing restored .NET 9 support.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/GovukNotify/GovukNotify.csproj | Adds net9.0 to TFMs and bumps package version to 9.0.0. |
| src/GovukNotify.Tests/GovukNotify.Tests.csproj | Adds net9.0 to TFMs and forces LangVersion to 14.0. |
| Makefile | Removes -f net10.0 / custom TargetFrameworks overrides so commands build all TFMs by default. |
| CHANGELOG.md | Adds 9.0.0 entry describing restored .NET 9 support. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| build-release: ## Build release version | ||
| dotnet build -c=Release -f=net10.0 | ||
| dotnet build -c=Release | ||
|
|
||
| .PHONY: build-package | ||
| build-package: build-release ## Build and package NuGet | ||
| dotnet pack -c=Release ./src/GovukNotify/GovukNotify.csproj /p:TargetFrameworks=net8,net9,net10 -o=publish | ||
| dotnet pack -c=Release ./src/GovukNotify/GovukNotify.csproj -o=publish |
There was a problem hiding this comment.
With the projects now multi-targeting net9.0;net10.0, running dotnet build/dotnet pack without specifying a framework will attempt to build/pack all TFMs. This will break environments that don’t have a .NET 10 SDK available (which is the motivation for this PR). Consider adding a Makefile variable (e.g., TFM?=net9.0) to allow building/packing just net9.0 on constrained CI, or ensuring CI installs/pins the required SDK so the default targets work everywhere.
There was a problem hiding this comment.
No. We want to build all TFMs
| <TargetFrameworks>net10.0;net9.0</TargetFrameworks> | ||
| <Version>9.0.0</Version> |
There was a problem hiding this comment.
TargetFrameworks now includes net10.0, but the repo’s AppVeyor pipeline runs dotnet test/dotnet pack without installing a .NET 10 SDK (see appveyor.txt / publish.cmd). On a machine with only .NET 9 (or earlier) installed this will fail with NETSDK1045 (cannot target .NET 10). Consider making net10.0 conditional on the SDK version/CI environment, or installing/pinning a .NET 10 SDK via global.json/CI setup, so AppVeyor can still build/test/pack.
| <TargetFrameworks>net10.0</TargetFrameworks> | ||
|
|
||
| <TargetFrameworks>net10.0;net9.0</TargetFrameworks> | ||
| <LangVersion>14.0</LangVersion> |
There was a problem hiding this comment.
Setting LangVersion to 14.0 will require a compiler/SDK that understands C# 14. If CI/AppVeyor is limited to the .NET 9 SDK (as noted in the PR description), this is likely to fail at compile time with an invalid language version. Prefer removing the override, or making it conditional on the installed SDK version; if specific tests depend on newer syntax, it would be safer to rewrite those assertions to be C# 13-compatible.
| <LangVersion>14.0</LangVersion> |
There was a problem hiding this comment.
It’s fine, this is test code so we’re not shipping it to consumers
|
@quis I assume this has got stuck on dependencies which is fine but is there any update on a likely release date? My understanding is that you don't need to update every dependent application to use the latest so could it be released so compatible applications can be patched and then release a patch release later for the apps that were stuck on old versions of .NET? Basically allowing people to patch .NET 10 now and patch .NET 9 and below later? |
Since appveyor doesn’t have .NET 10 we need to also publish for .NET 9
This commit makes sure we are building and testing against both everywhere. To make this simpler I have removed the flags from the
Makefileand let the target versions be defined canonically in the*.csprojfiles.The only change to make .NET 9.0 work was specifying
<LangVersion>14.0</LangVersion>when building the tests. .NET 9.0 uses CSharp version 13.0, which is missing some of the assertion features we need in the tests (probably since dropping NUnit). But I think this should be fine because Appveyor will be using the packaged code, not compiling it from source.