Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support .NET 9 #10593

Merged
merged 18 commits into from
Oct 2, 2024
Merged

Support .NET 9 #10593

merged 18 commits into from
Oct 2, 2024

Conversation

martincostello
Copy link
Contributor

@martincostello martincostello commented Sep 12, 2024

What are you trying to accomplish?

Support .NET 9 by installing both the STS and LTS releases of .NET.

Relates to #10496, but doesn't resolve it in the general case (i.e. it won't work for .NET 10).

Anything you want to highlight for special attention from reviewers?

As .NET 9 is in RC and not "official" yet, I figured the least risky option (at the expense of increasing the image size) was to install both .NET 8 and .NET 9. These could then be updated on alternate years (LTS changes to 10 next year, STS changes to 11 in 2026, etc.)

The .NET 9 SDK also makes NuGet Audit enabled by default and breaks package restore when vulnerable packages are in the dependency tree. I've resolved that for building NuGetUpdated, but it might occur when upgrading users' own projects. I couldn't see an obvious place in the code to change the tool to set NuGetAudit=false to avoid that when updating packages. I did do a search for NoWarn, but those all seemed to be in projects being built rather than when invoking the tool.

How will you know you've accomplished your goal?

Dependabot can update NuGet packages for projects targeting net9.0.

Checklist

  • I have run the complete test suite to ensure all tests and linters pass.
  • I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
  • I have written clear and descriptive commit messages.
  • I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
  • I have ensured that the code is well-documented and easy to understand.

Support .NET 9 by installing both the STS and LTS releases of .NET.
@github-actions github-actions bot added the L: dotnet:nuget NuGet packages via nuget or dotnet label Sep 12, 2024
@martincostello
Copy link
Contributor Author

martincostello commented Sep 12, 2024

From the tests' Docker build:

#9 [ 4/10] RUN dotnet --list-runtimes
#9 0.185 Microsoft.AspNetCore.App 8.0.8 [/usr/local/dotnet/current/shared/Microsoft.AspNetCore.App]
#9 0.185 Microsoft.AspNetCore.App 9.0.0-rc.1.24452.1 [/usr/local/dotnet/current/shared/Microsoft.AspNetCore.App]
#9 0.185 Microsoft.NETCore.App 8.0.8 [/usr/local/dotnet/current/shared/Microsoft.NETCore.App]
#9 0.185 Microsoft.NETCore.App 9.0.0-rc.1.24431.7 [/usr/local/dotnet/current/shared/Microsoft.NETCore.App]
#9 DONE 0.2s

#10 [ 5/10] RUN dotnet --list-sdks
#10 0.212 8.0.401 [/usr/local/dotnet/current/sdk]
#10 0.212 9.0.100-rc.1.24452.12 [/usr/local/dotnet/current/sdk]
#10 DONE 0.2s

Try to fix warnings building NuGetUpdater with the .NET 9 SDK from NuGet Audit.
Disable NuGet audit when running the native C# tests, as they don't seem to otherwise obey the changes in Directory.Build.props of the project itself.
@martincostello
Copy link
Contributor Author

I'm going to move this out of draft so that someone who knows Ruby and the repo layout can point me at where I need to make the changes to make the build happy.

@martincostello martincostello marked this pull request as ready for review September 12, 2024 15:39
@martincostello martincostello requested a review from a team as a code owner September 12, 2024 15:39
Update tests for the changes to the output format of `dotnet test` in .NET 9.
Forcibly enable the terminal logger in CI so the expected test output is present.
Attempt to run the tests with the .NET 8 SDK instead of .NET 9 to see if it fixes the failing tests.
@martincostello martincostello requested a review from a team as a code owner September 21, 2024 11:17
Account for extra parameter.
@github-actions github-actions bot added the L: github:actions GitHub Actions label Sep 21, 2024
Revert to the expected output for .NET 8 SDK.
@martincostello
Copy link
Contributor Author

Changing the C# tests to run in the current working directory of NuGetUpdater to force it to use the .NET SDK from global.json instead of rolling forward to the .NET 9 SDK has fixed one of the tests, but 4 tests are failing for NuGetUpdater.Core.Test for some reason I need to dig into.

    - Fix tests broken by error about usage of CPVM.
    - Update Microsoft.Windows.SDK.NET.Ref version.
Add missing line to expected contents.
Add the C# Dev Kit extension to the devcontainers.
Install the .NET 9 SDK in the devcontainer.
Update NuGetUpdater to target .NET 9 so that tests can test compatibility with `net9.0`.
@@ -9,7 +9,7 @@
NuGetUpdater\NuGetUpdater.Core\FrameworkChecker\SupportedFrameworks.cs
2. Update tests as needed at `NuGetUpdater\NuGetUpdater.Core.Test\FrameworkChecker\CompatibilityCheckerFacts.cs`
-->
<CommonTargetFramework>net8.0</CommonTargetFramework>
<CommonTargetFramework>net9.0</CommonTargetFramework>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Despite the comments above, the referenced files haven't been updated to explicitly add net9.0, and the changes for .NET 8 were only made 6 months ago, way after it was released: NuGet/NuGetGallery#9858

@@ -244,7 +249,7 @@ await TestUpdateForProject("Some.Package", "9.0.1", "13.0.1",
MockNuGetPackage.CreateSimplePackage("Some.Package", "9.0.1", "net8.0"),
MockNuGetPackage.CreateSimplePackage("Some.Package", "13.0.1", "net8.0"),
// necessary for the `net8.0-windows10.0.19041.0` TFM
new("Microsoft.Windows.SDK.NET.Ref", "10.0.19041.31", Files:
new("Microsoft.Windows.SDK.NET.Ref", "10.0.19041.34", Files:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest .NET 8 SDK bumps this version, so it's been updated so the tests can find the package in the local cache.

@@ -186,6 +187,7 @@ await TestUpdateForProject("Some.Package", "12.0.1", "13.0.1",
projectContents: $"""
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<ManagePackageVersionsCentrally>false</ManagePackageVersionsCentrally>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change to force the ruby tests for C# to use the version of the .NET SDK in global.json for NuGetUpdater cause an error that package versions shouldn't be used in projects using central package management. I think this is because the build then picks up the Directory.Packages.props file in that directory despite the test project not being in the directory tree of that file.

Update script to build for .NET 9.
@@ -32,7 +32,7 @@ cd "$install_dir/lib/NuGetUpdater/NuGetUpdater.Cli"
dotnet publish \
--configuration Release \
--output "$install_dir/NuGetUpdater" \
--framework net8.0 \
--framework net9.0 \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To test net9.0, the tests have to target net9.0 due to this logic for setting up the local package repository used in the tests:

// find the actual SDK directory
string privateCoreLibPath = typeof(object).Assembly.Location; // e.g., C:\Program Files\dotnet\shared\Microsoft.NETCore.App\8.0.4\System.Private.CoreLib.dll
string sdkDirectory = Path.Combine(Path.GetDirectoryName(privateCoreLibPath)!, "..", "..", "..", "sdk", sdkVersionString); // e.g., C:\Program Files\dotnet\sdk\8.0.204
string bundledVersionsPropsPath = Path.Combine(sdkDirectory, "Microsoft.NETCoreSdk.BundledVersions.props");
FileInfo normalizedPath = new(bundledVersionsPropsPath);
return normalizedPath.FullName;

Otherwise the file read doesn't contain support for .NET 9, and then the local package repo doesn't contain any of the required fake packages and dotnet restore fails for net9.0 projects.

Update the assertions for the Ruby tests for C# to account for the behaviour changes for the new terminal logger.
@@ -16,6 +16,7 @@ public class Sdk : UpdateWorkerTestBase
[InlineData("net472")]
[InlineData("net7.0")]
[InlineData("net8.0")]
[InlineData("net9.0")]
Copy link
Contributor Author

@martincostello martincostello Sep 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this test were skipped, the need to update NuGetUpdater to net9.0 itself can be reverted. Otherwise, the tests need reworking in some way to discover .NET SDK versions without relying on the exact version of .NET the tests are executing with.

@martincostello
Copy link
Contributor Author

@brettfo Should I abandon this PR?

It looks like changes you've made in #10521 (specifically this bit) will have the stuff you're working on install the SDK in global.json if it's not installed, and would also resolve #10496 for arbitrary versions of .NET going forwards.

@brettfo
Copy link
Contributor

brettfo commented Sep 26, 2024

@martincostello I think this PR is still needed. The changes in #10521 aren't active yet, that's just the skeleton for longer term future work. I marked one place where a PR that's about to get merged will conflict with this, but it's a minor fix.

@martincostello
Copy link
Contributor Author

@brettfo Thanks - once that's merged I'll rebase this.

@brettfo
Copy link
Contributor

brettfo commented Oct 1, 2024

@martincostello The other PRs have all been merged. Can you update this on the latest main? There should be 2 conflicts: 1 right before ProcessEx.RunAsync is called; the function was changed to take IEnumerable<string> arguments instead of string arguments, and another conflict in nuget/Dockerfile; we're now installing PowerShell and rearranged slightly.

@martincostello
Copy link
Contributor Author

@brettfo Should be good now.

@thavaahariharangit thavaahariharangit merged commit 03aa6dc into dependabot:main Oct 2, 2024
109 checks passed
@martincostello martincostello deleted the dotnet-9 branch October 2, 2024 16:41
@martincostello
Copy link
Contributor Author

My .NET 9 repos are now generating dependabot PRs again 🎉 (e.g. martincostello/costellobot#1736)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: dotnet:nuget NuGet packages via nuget or dotnet L: github:actions GitHub Actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants