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

Update .NET SDKs #10752

Merged
merged 5 commits into from
Nov 14, 2024
Merged

Conversation

martincostello
Copy link
Contributor

@martincostello martincostello commented Oct 8, 2024

What are you trying to accomplish?

Update .NET 8 and 9 SDKs to their latest versions - the main motivation to use a stable version of .NET 9.

Anything you want to highlight for special attention from reviewers?

No.

How will you know you've accomplished your goal?

The build is green.

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.

@martincostello martincostello requested a review from a team as a code owner October 8, 2024 17:41
@github-actions github-actions bot added the L: dotnet:nuget NuGet packages via nuget or dotnet label Oct 8, 2024
@martincostello
Copy link
Contributor Author

I'm not sure why the tests are failing - will take a look tomorrow.

@JamieMagee
Copy link
Contributor

JamieMagee commented Oct 8, 2024

@martincostello it looks like we might have been working around a bug you logged upstream, that has since been fixed: dotnet/msbuild#10682. In nuget/spec/dependabot/nuget/native_helpers_spec.rb

 it "contains the expected output" do
  # In CI when the terminal logger is disabled by default in .NET 9 there is no
  # output from the test runner: https://github.com/dotnet/msbuild/issues/10682.
  # Instead we have to rely on the cmd invocation failing with a non-zero exit code
  # if any tests fail. Locally when the terminal logger is enabled we can check
  # there is an absence of any evidence of test failures in the output.
  # expect(dotnet_test).to include("Passed!")
  expect(dotnet_test).not_to include("Build failed")
end

@martincostello
Copy link
Contributor Author

Well, well, well, if it isn't the consequences of my own actions 😆

I'll fix that up in the morning.

@martincostello
Copy link
Contributor Author

Still debugging, but it looks like it's not the issue being fixed (it's still open, which makes sense), but that because of the bug, we can't see why the tests are failing.

Running them locally, one fails:

[xUnit.net 00:21:18.64]   Finished:    NuGetUpdater.Core.Test
  NuGetUpdater.Core.Test test failed with 1 error(s) (1280.0s)
    /home/martin/coding/dependabot/dependabot-core/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core.Test/Update/UpdateWorkerTestBase.cs(304): error TESTERROR:
      NuGetUpdater.Core.Test.Update.UpdateWorkerTests+Sdk.UpdateVersionAttribute_InProjectFile_ForPackageR
      eferenceInclude_Windows (1s 505ms): Error Message: Assert.Equal() Failure
                                       ↓ (pos 257)
      Expected: ···e.Package" Version="13.0.1" />\n  </ItemGroup>\n</Project>
      Actual:   ···e.Package" Version="9.0.1" />\n  </ItemGroup>\n</Project>
                                       ↑ (pos 257)
      Stack Trace:
         at NuGetUpdater.Core.Test.Update.UpdateWorkerTestBase.AssertContainsFiles(ValueTuple`2[] expected
      , ValueTuple`2[] actual) in /home/martin/coding/dependabot/dependabot-core/nuget/helpers/lib/NuGetUp
      dater/NuGetUpdater.Core.Test/Update/UpdateWorkerTestBase.cs:line 304
         at NuGetUpdater.Core.Test.Update.UpdateWorkerTestBase.TestUpdateForProject(String dependencyName,
       String oldVersion, String newVersion, ValueTuple`2 projectFile, String expectedProjectContents, Boo
      lean isTransitive, ValueTuple`2[] additionalFiles, ValueTuple`2[] additionalFilesExpected, MockNuGet
      Package[] packages, UpdateOperationResult expectedResult) in /home/martin/coding/dependabot/dependab
      ot-core/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core.Test/Update/UpdateWorkerTestBase.cs:line 15
      1
         at NuGetUpdater.Core.Test.Update.UpdateWorkerTests.Sdk.UpdateVersionAttribute_InProjectFile_ForPa
      ckageReferenceInclude_Windows() in /home/martin/coding/dependabot/dependabot-core/nuget/helpers/lib/
      NuGetUpdater/NuGetUpdater.Core.Test/Update/UpdateWorkerTests.Sdk.cs:line 246
      --- End of stack trace from previous location ---
  NuGetUpdater.Cli.Test succeeded (0.3s) → artifacts/bin/NuGetUpdater.Cli.Test/debug/NuGetUpdater.Cli.Test.dll
[xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v2.4.5+1caef2f33e (64-bit .NET 9.0.0-rc.2.24473.5)
[xUnit.net 00:00:00.80]   Discovering: NuGetUpdater.Cli.Test
[xUnit.net 00:00:00.85]   Discovered:  NuGetUpdater.Cli.Test
[xUnit.net 00:00:00.85]   Starting:    NuGetUpdater.Cli.Test
[xUnit.net 00:01:23.65]   Finished:    NuGetUpdater.Cli.Test
  NuGetUpdater.Cli.Test test succeeded (84.4s)

Test summary: total: 357, failed: 1, succeeded: 356, skipped: 0, duration: 1364.5s
Build failed with 1 error(s) and 24 warning(s) in 1373.7s

@martincostello
Copy link
Contributor Author

Just saw, the issue is fixed, but it didn't make it into RC2 so will be in 9.0.100.

@@ -249,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.34", Files:
new("Microsoft.Windows.SDK.NET.Ref", "10.0.19041.45", 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.

This version was changed since RC1 by dotnet/sdk#41936, and it's going to change again with dotnet/sdk#43889 to .46, so will probably need changing next month when updating to 9.0.100.

@martincostello
Copy link
Contributor Author

I'm not sure why the lockfile smoke test is failing.

The error is:

updater | /tmp/package-dependency-resolution_kuH1Yp/Project.csproj : error NU1102: Unable to find package Microsoft.WindowsDesktop.App.Ref with version (= 8.0.10)
updater | /tmp/package-dependency-resolution_kuH1Yp/Project.csproj : error NU1102:   - Found 120 version(s) in nuget.org [ Nearest version: 9.0.0-preview.1.24081.3 ]

but the package is in NuGet.org: [email protected].

Maybe the test depends on whatever version of the .NET SDK happens to be installed on the hosted runner?

@martincostello martincostello force-pushed the bump-dotnet-sdks branch 2 times, most recently from 920b972 to 970f2ac Compare October 17, 2024 14:45
@martincostello
Copy link
Contributor Author

From the build logs:

#10 [ 5/15] RUN dotnet --list-runtimes
#10 0.181 Microsoft.AspNetCore.App 8.0.10 [/usr/local/dotnet/current/shared/Microsoft.AspNetCore.App]
#10 0.181 Microsoft.AspNetCore.App 9.0.0-rc.2.24474.3 [/usr/local/dotnet/current/shared/Microsoft.AspNetCore.App]
#10 0.181 Microsoft.NETCore.App 8.0.10 [/usr/local/dotnet/current/shared/Microsoft.NETCore.App]
#10 0.181 Microsoft.NETCore.App 9.0.0-rc.2.24473.5 [/usr/local/dotnet/current/shared/Microsoft.NETCore.App]
#10 DONE 0.2s

When I do the same locally on my Windows machine:

Microsoft.AspNetCore.App 6.0.35 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 7.0.20 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 8.0.10 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 9.0.0-rc.2.24474.3 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.NETCore.App 6.0.35 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 7.0.20 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 8.0.10 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 9.0.0-rc.2.24473.5 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.WindowsDesktop.App 6.0.35 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 7.0.20 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 8.0.10 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 9.0.0-rc.2.24474.4 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

Maybe the issue is that Microsoft.WindowsDesktop.App isn't being installed? Though, if that is the case, I'd have thought the tests would be broken already?

Update .NET 8 and 9 SDKs to their latest versions.
Bump Microsoft.Windows.SDK.NET.Ref version.
Update to November 2024 releases for .NET 8 and 9.
Update the version of Microsoft.Windows.SDK.NET.Ref used in the tests.
@martincostello
Copy link
Contributor Author

@brettfo @JamieMagee Do either of you have any idea what's going on with the end-to-end tests? I'm not sure what I'm missing - feels like the tests depend on a version of .NET that's not present in the Docker image.

@brettfo
Copy link
Contributor

brettfo commented Nov 14, 2024

@martincostello It looks like the packages weren't yet present on nuget.org. The publish date says otherwise, but maybe a replication issue? I've restarted the failing jobs to see what happens now.

error NU1102: Unable to find package Microsoft.WindowsDesktop.App.Ref with version (= 8.0.11)

@brettfo
Copy link
Contributor

brettfo commented Nov 14, 2024

Well the re-run didn't work. I thought I remembered something about the smoke tests caching some network calls which means the cache won't know about that 8.0.11, etc. packages. Let me see if I can track down if there is a cache and if so how to clear or regenerate it.

@martincostello
Copy link
Contributor Author

I notice that the original one I had issues with was the lockfiles test, which is now passing with the re-runs, so there's now 2 failing instead of 3. Caching sounds like a good avenue to solve the mystery.

@brettfo
Copy link
Contributor

brettfo commented Nov 14, 2024

Got it! It was the cache. The results of querying the Microsoft.WindowsDesktop.App.Ref was cached and didn't have the 8.0.11 result, but re-running the cache captured that. All green now, I'll talk to the powers that be to get this merged.

@sachin-sandhu sachin-sandhu merged commit d9be882 into dependabot:main Nov 14, 2024
52 checks passed
@martincostello martincostello deleted the bump-dotnet-sdks branch November 14, 2024 19:11
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants