C#: Improve the logic for downloading .NET and setting environment variables. #20837
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the .NET 10 RC 2 integration tests with .NET 10 final release integration tests. The platform-specific tests (Windows and Linux) are removed in favor of a single all-platforms test that covers both traced and buildless build modes.
- Removes .NET 10 RC 2 tests from
windows/dotnet_10_rc2andlinux/dotnet_10_rc2directories - Adds new .NET 10 final release tests in
all-platforms/dotnet_10directory - Tests both traced mode (
test1) and buildless mode (test2)
Reviewed Changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
csharp/ql/integration-tests/windows/dotnet_10_rc2/test.py |
Removed RC 2 test file for Windows |
csharp/ql/integration-tests/windows/dotnet_10_rc2/global.json |
Removed RC 2 SDK configuration for Windows |
csharp/ql/integration-tests/windows/dotnet_10_rc2/dotnet_build.csproj |
Removed RC 2 project file for Windows |
csharp/ql/integration-tests/windows/dotnet_10_rc2/Program.cs |
Removed RC 2 test program for Windows |
csharp/ql/integration-tests/linux/dotnet_10_rc2/test.py |
Removed RC 2 test file for Linux |
csharp/ql/integration-tests/linux/dotnet_10_rc2/global.json |
Removed RC 2 SDK configuration for Linux |
csharp/ql/integration-tests/all-platforms/dotnet_10/test.py |
Added new test with traced and buildless modes for .NET 10 final release |
csharp/ql/integration-tests/all-platforms/dotnet_10/global.json |
Added SDK configuration for .NET 10.0.100 final release |
csharp/ql/integration-tests/all-platforms/dotnet_10/dotnet_build.csproj |
Added .NET 10 project file with net10.0 target framework |
csharp/ql/integration-tests/all-platforms/dotnet_10/Program.cs |
Added simple test program that outputs command-line arguments |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
michaelnebel
force-pushed
the
csharp/dotnet10
branch
4 times, most recently
from
3563572 to
d5bd473
Compare
…eamline the minimal dotnet environment.
…nguage as english).
michaelnebel
force-pushed
the
csharp/dotnet10
branch
from
d5bd473 to
edabbfc
Compare
michaelnebel
changed the title
|
@hvitved : The PR is ready for review again. The scope has been changed to also improve the .NET downloading and environment variable setting. The tests has run successfully four times in a row - so I hope this fixes the .NET 10 crash problem on MacOS. Will also start some DCA experiments. |
| // Request ARM64 architecture on Apple Silicon machines | ||
| if (actions.IsRunningOnAppleSilicon()) | ||
| { | ||
| cb.Argument("--architecture"). | ||
| Argument("arm64"); | ||
| } |
There was a problem hiding this comment.
Drive-by comment: does this not cause problems for the tracer? I assume some test would have caught it if it was a problem, but I am curious about why this is OK. Would it maybe make sense to document that in a comment here?
There was a problem hiding this comment.
On the ARM machines (when inspecting the output from the other tests) it looks like the RiD is osx-arm64 for the installed .NET versions (at least those being used). Without setting this explicitly, the install script downloads the osx-x64 version. According to CoPilot it can cause problems when different versions of .NET are installed on the same machine. Changing this appears to have a positive effect when running the (traced .NET 10) tests on the MacOS 15 and MacOS26 runners.
| {"DOTNET_CLI_UI_LANGUAGE", "en"}, | ||
| {"MSBUILDDISABLENODEREUSE", "1"}, | ||
| {"DOTNET_SKIP_FIRST_TIME_EXPERIENCE", "true"} |
There was a problem hiding this comment.
Minor: The doc comment above explains DOTNET_CLI_UI_LANGUAGE but not the other two environment variables. Would it make sense to document why we are setting them as well? DOTNET_SKIP_FIRST_TIME_EXPERIENCE is kind of obvious, but MSBUILDDISABLENODEREUSE maybe less so.
There was a problem hiding this comment.
There was no comment explaining this before and I just moved the logic.
There was a problem hiding this comment.
Will add some comments - then the tests will also be run a again 😄
|
DCA looks good. |
3 participants
In this PR we
dotnetenvironment variables are set when executingdotnetcommands.The changes to the .NET download and environment variable setting was made because the .NET 10 tests failed spuriously.
Added some .NET 10 integration tests for traced and buildless (replacing the .NET 10 RC 2 integration tests we ran on Windows and Linux). We do not officially support .NET 10.