Add .NET 10 target framework support#325
Conversation
Co-authored-by: sensslen <3428860+sensslen@users.noreply.github.com>
Co-authored-by: sensslen <3428860+sensslen@users.noreply.github.com>
Co-authored-by: sensslen <3428860+sensslen@users.noreply.github.com>
Co-authored-by: sensslen <3428860+sensslen@users.noreply.github.com>
…dback Co-authored-by: sensslen <3428860+sensslen@users.noreply.github.com>
|
@copilot Make the .net 10 integration unconditional and make sure it gets installed to the CI pipelines where needed. .net 10 will land soon and I want the repository to be ready. There's no need for this complex approach |
Co-authored-by: sensslen <3428860+sensslen@users.noreply.github.com>
Implemented in commit ae48825. Changes made:
The project now unconditionally targets .NET 10, and all CI pipelines will install the .NET 10 SDK. |
…eworks Co-authored-by: sensslen <3428860+sensslen@users.noreply.github.com>
709a166 to
54287be
Compare
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis pull request extends .NET 10.0 framework support across the project and related infrastructure. Target frameworks were updated from net472;net8.0;net9.0 to include net10.0 in all relevant project files. GitHub workflows were consolidated to use single unified setup steps handling multiple .NET versions (8.0.x, 9.0.x, 10.0.x) instead of separate version-specific blocks. A global.json file was added specifying .NET SDK version 10.0.100-rc.2 with test runner configuration. Package dependencies were updated, including Microsoft.Build.Locator upgraded to version 1.10.12 and McMaster.Extensions.CommandLineUtils removed. Code changes include refactoring MsBuildAbstraction.cs to use a lazy-initializing method instead of a property, conditional logic updates for net10.0 or later runtimes, and EnableNUnitRunner property additions to test project files. Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
|
@coderabbitai please review |
|
@sensslen: I'll review the changes in this pull request for .NET 10 support. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/release.yml (1)
35-47: Add net10.0 tests and migrate from VSTest action to dotnet test with Microsoft.Testing.Platform.Two verified concerns:
Missing net10.0 tests: NuGetUtility.Test supports net10.0 (targets: net472;net8.0;net9.0;net10.0) but the workflow only tests net8.0 and net9.0. Net10.0 binaries are published without test coverage.
VSTest incompatibility with .NET 10: The workflow uses
josepho0918/vstest-action, which relies on VSTest. However,global.jsonconfigures"runner": "Microsoft.Testing.Platform", and .NET 10 SDK no longer supports VSTest without explicitly opting in. The action will fail or produce incorrect results when running on .NET 10.Recommended fix:
- Replace the VSTest action calls with
dotnet testcommand, which natively supports Microsoft.Testing.Platform on .NET 10.- Add test step for NuGetUtility.Test on net10.0.
- Note: NuGetUtility.Test.UrlToLicenseMapping only targets net9.0, so it cannot be tested on net10.0.
🧹 Nitpick comments (7)
global.json (1)
6-6: Consider SDK version stability.The SDK version
10.0.100-rc.2.25502.107is a release candidate. WhilerollForward: latestMajorprovides flexibility, be aware that RC versions may have stability or compatibility issues in production environments.src/NuGetUtility/Wrapper/MsBuildWrapper/MsBuildAbstraction.cs (1)
40-48: Refactoring from property to method is functionally correct.The lazy initialization pattern remains intact, just moved from a property to a private method. This makes the initialization more explicit and avoids potential property side-effect concerns.
If thread safety is a concern, consider using
Lazy<T>:-private ProjectCollection? _projects; +private readonly Lazy<ProjectCollection> _projects = new(() => InitializeProjectCollection()); private ProjectCollection GetProjectCollection() { - if (_projects is null) - { - _projects = InitializeProjectCollection(); - } - - return _projects; + return _projects.Value; }.github/workflows/action.yml (5)
33-39: Workflow setup approach is simplified but wastes some resources.The consolidated setup-dotnet step now installs all three SDK versions (8.0.x, 9.0.x, 10.0.x) globally for every job, even when specific jobs only need one or two versions. While this is simpler and aligns with "unconditional .NET 10 support," consider optimizing by using conditional setup based on
matrix.dotnetVersionfor jobs that use framework matrices.For jobs with
matrix.frameworkand correspondingmatrix.dotnetVersion(e.g.,test,test_windows,check_licenses,check_version_command), use:- name: Setup dotnet uses: actions/setup-dotnet@v5 with: dotnet-version: ${{ matrix.dotnetVersion }}For single-version jobs (e.g.,
test_file_license_matching,test_url_to_license_mapping), you may keep the multiline format since they need multiple versions or simplify to just the needed version.
61-66: Redundant SDK installations for single-framework test jobs.Jobs
test_file_license_matchingandtest_url_to_license_mappinginstall both 9.0.x and 10.0.x, but per PR objectives, these test projects target only net9.0. This wastes CI resources.Optimize these jobs to install only the required version:
- name: Setup dotnet uses: actions/setup-dotnet@v5 with: dotnet-version: "9.0.x"The test commands (lines 72 and 92) will correctly execute against the single target framework without needing
-fflag.Also applies to: 79-84
112-118: Setup is redundant for test_windows job when net472 test uses msbuild.The setup-dotnet installs all three .NET versions (8.0.x, 9.0.x, 10.0.x) for every matrix iteration, including when
matrix.framework == net472. For the net472 case, these .NET SDKs are unnecessary since msbuild on Windows already provides the .NET Framework tooling.Optimize by using conditional setup based on framework:
- name: Setup dotnet uses: actions/setup-dotnet@v5 with: dotnet-version: ${{ matrix.dotnetVersion }} if: matrix.framework != 'net472'Or use inline conditions in the setup step.
138-143: Unnecessary multi-version setup for check_code_format.The
check_code_formatjob installs both 9.0.x and 10.0.x, butdotnet formattypically uses the first available SDK. Since this is a code format check (not framework-specific), simplify to:- name: Setup dotnet uses: actions/setup-dotnet@v5 with: dotnet-version: "9.0.x"
210-215: Unnecessary SDK setup for .NET Framework jobs.The jobs
check_licenses_net472(lines 210-215) andcheck_version_command_net472(lines 303-308) install 9.0.x and 10.0.x SDKs, but since these jobs build only net472 using msbuild/Visual Studio, the .NET SDKs are not used. Remove these setup steps or optimize to install only what's needed (e.g., for dotnet CLI restore fallback):- name: Setup dotnet uses: actions/setup-dotnet@v5 with: dotnet-version: "9.0.x" if: false # Not needed for net472-only msbuild jobsOr remove the setup step entirely if msbuild handles everything.
Also applies to: 303-308
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/action.yml(8 hunks).github/workflows/release.yml(2 hunks)NuGetUtility.sln(1 hunks)global.json(1 hunks)src/NuGetLicense/NuGetLicense.csproj(1 hunks)src/NuGetLicenseCore/NuGetLicenseCore.csproj(1 hunks)src/NuGetUtility/NuGetUtility.csproj(3 hunks)src/NuGetUtility/Wrapper/MsBuildWrapper/MsBuildAbstraction.cs(2 hunks)src/SPDXLicenseMatcher/JavaLibrary/ParseInstruction.cs(1 hunks)src/SPDXLicenseMatcher/SPDXLicenseMatcher.csproj(1 hunks)tests/NuGetLicense.Test/NuGetLicense.Test.csproj(2 hunks)tests/NuGetUtility.Test.Extensions/NuGetUtility.Test.Extensions.csproj(1 hunks)tests/NuGetUtility.Test/NuGetUtility.Test.csproj(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test_file_license_matching (macos-latest)
🔇 Additional comments (14)
src/SPDXLicenseMatcher/SPDXLicenseMatcher.csproj (1)
4-4: LGTM! Target framework extension is correct.The addition of
net10.0to the target frameworks aligns with the PR objective to add .NET 10 support.src/NuGetLicenseCore/NuGetLicenseCore.csproj (1)
5-5: LGTM! Target framework extension is correct.The addition of
net10.0enables the dotnet tool to run on .NET 10 runtime.tests/NuGetUtility.Test.Extensions/NuGetUtility.Test.Extensions.csproj (1)
4-4: LGTM! Test project target framework extension is correct.Adding
net10.0will enable running tests on .NET 10 runtime.NuGetUtility.sln (1)
30-30: LGTM! Good practice to include global.json in Solution Items.Making
global.jsonvisible in the solution helps developers discover and understand the SDK configuration..github/workflows/release.yml (2)
17-20: LGTM! Multi-version dotnet setup is correct.The consolidated setup installing .NET 8.0.x, 9.0.x, and 10.0.x versions is appropriate for multi-targeting.
61-62: LGTM! net10.0 publish step added correctly.The publish command for .NET 10 follows the same pattern as net8 and net9.
src/NuGetUtility/Wrapper/MsBuildWrapper/MsBuildAbstraction.cs (1)
27-27: LGTM! Method invocation is correct.The call to
GetProjectCollection().LoadProject(projectPath)correctly uses the new method.global.json (1)
1-9: Test runner configuration is complete and properly configured.The verification confirms that test projects already have
<EnableNUnitRunner>true</EnableNUnitRunner>configured, which works correctly with the Microsoft.Testing.Platform specified in global.json. This configuration successfully addresses the CI error about VSTest incompatibility with .NET 10.src/SPDXLicenseMatcher/JavaLibrary/ParseInstruction.cs (1)
646-656: I'll verify the key claims in this review comment about C# language features and the dotnet format warning.Remove the critique about unnecessary conditional compilation.
The review comment is incorrect. Null-conditional assignment (?. and ?[]) on reference types is a feature introduced in C# 14, which means it is only available when targeting .NET 10. Beginning in C# 14, assignment is permissible with a null conditional access expression (?. and ?[]) on reference types.
The code's conditional compilation is therefore justified:
- For NET10_0_OR_GREATER: The syntax
nextInst?.Skip = trueworks because C# 14 is available- For earlier frameworks: The explicit null check pattern is required because null-conditional assignment syntax is not supported
IDE0031 recommends using null-conditional operators, but this rule was designed before null-conditional assignment was available. The code correctly applies the modern syntax where possible while maintaining backward compatibility.
tests/NuGetUtility.Test/NuGetUtility.Test.csproj (1)
3-3: Consistent pattern with NuGetLicense.Test.The TargetFrameworks and EnableNUnitRunner changes align with the sibling test project and PR objectives.
Also applies to: 14-14
src/NuGetLicense/NuGetLicense.csproj (1)
4-4: Architecture is clear: NuGetLicense delegates to NuGetUtility.The TargetFrameworks extension and package strategy (retaining McMaster.Extensions.CommandLineUtils for CLI arg parsing, delegating core logic to NuGetUtility) is appropriate. The removal of build/NuGet packages simplifies the dependency graph correctly.
.github/workflows/action.yml (1)
47-51: Test step framework targeting is correct.Using
-f ${{ matrix.framework }}with--no-restore --no-buildproperly executes tests for each target framework after the matrix build.tests/NuGetLicense.Test/NuGetLicense.Test.csproj (1)
14-14: EnableNUnitRunner property is correctly configured and supported.NUnit3TestAdapter version 5.2.0 supports EnableNUnitRunner (supported since 5.0), and the configuration
<EnableNUnitRunner>true</EnableNUnitRunner>is correct per Microsoft documentation. No additional configuration is needed. The change appropriately addresses the VSTest deprecation issue on .NET 10.src/NuGetUtility/NuGetUtility.csproj (1)
4-4: The review comment contains an incorrect version reference - Microsoft.Build.Locator version 1.10.12 does not exist.The latest available version of Microsoft.Build.Locator is 1.10.2, not 1.10.12. This appears to be either a typo in the review comment or in the PR code itself. If the PR actually references 1.10.12, the build will fail with a package resolution error. Verify the actual package version specified in line 21 of the csproj file and correct it to 1.10.2.
Likely an incorrect or invalid review comment.
|



Preparing project for .NET 10 compatibility with unconditional targeting.
Project Configuration:
net472;net8.0;net9.0;net10.0net8.0;net9.0;net10.0Test Project Configuration (maintained per maintainer feedback):
SPDXLicenseMatcher.Test- single framework (net9.0)NuGetUtility.Test.UrlToLicenseMapping- single framework (net9.0)MultiTargetProjectWithDifferentDependencies- original state (net472;net9.0;net8.0-browser)ProjectWithReferenceContainingFileLicense- single framework (net9.0)GitHub Actions:
-fflag in main test job, so all frameworks are built)Microsoft.Build References:
Code Formatting:
is not nullto!= nullfor better compatibilityThe project is now ready for .NET 10 using the same workflow pattern that worked before .NET 10 was added.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
Summary by CodeRabbit
New Features
Chores