-
-
Notifications
You must be signed in to change notification settings - Fork 200
Bump dotnet signal tests to .net 10 #1457
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
Conversation
|
The failing tests certainly point to a relevant change in behavior (though they don't immediately support the issue apparent in your logs). In our test setup, a managed exception caught in managed code causes the process to return a non-zero code. However, those are most likely related to these errors (or at least I would rectify them first before trying to conclude anything): from the musl runners
from arm64 ubuntu
|
|
Manually installing .net 10 on the ubuntu arm runner seems to work. I think we just have to push the changes to the dockerfile to main, triggering a run of the Docker action to also update the .net version for that image. |
|
With merging #1458 to main, the |
Thanks @JoshuaMoelans - much appreciated. I'll keep digging on our side. If it looks like it might be an issue with native, I'll try to add tests back here to confirm/replicate. btw: one of the other tests for Linux ARM64 is failing below... so not sure if I should be marking this branch ready for review or looking to merge into main/master. |
|
Looks like that was a flakey run. We can review/merge this if you want, or wait on it potentially being an issue with native & add tests before merging. I'm fine with either :) |
Cool, we may as well merge as .NET 8 is already out of support on mobile platforms. |
| # install manually until available by default (also for future .NET updates) | ||
| - name: Install .NET 10 SDK for ARM64 runners |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: dotnet signal tests will fail on non-ARM64 Linux runners due to missing .NET 10 SDK, as the installation step is ARM64-specific.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The dotnet signal tests (test_dotnet_signals_inproc, test_aot_signals_inproc) are configured to run on non-ARM64 Linux runners (e.g., ubuntu-24.04, ubuntu-22.04). These tests require the .NET 10 SDK to execute commands like dotnet run and dotnet publish. However, the new CI step responsible for installing .NET 10 is conditionally limited to ARM64 runners. Since .NET 10 is not yet pre-installed on GitHub Actions' non-ARM64 Linux images, these tests will fail with errors such as "dotnet: command not found" or "The .NET SDK cannot be located", breaking the CI pipeline on affected configurations.
💡 Suggested Fix
Modify the .NET 10 SDK installation step's condition to include non-ARM64 Linux runners where the dotnet signal tests are expected to run, or ensure these tests are skipped on configurations without the SDK.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: .github/workflows/ci.yml#L263-L264
Potential issue: The `dotnet` signal tests (`test_dotnet_signals_inproc`,
`test_aot_signals_inproc`) are configured to run on non-ARM64 Linux runners (e.g.,
ubuntu-24.04, ubuntu-22.04). These tests require the .NET 10 SDK to execute commands
like `dotnet run` and `dotnet publish`. However, the new CI step responsible for
installing .NET 10 is conditionally limited to `ARM64` runners. Since .NET 10 is not yet
pre-installed on GitHub Actions' non-ARM64 Linux images, these tests will fail with
errors such as "dotnet: command not found" or "The .NET SDK cannot be located", breaking
the CI pipeline on affected configurations.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 4395437
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI is passing on the other platforms, since .NET is already installed. Hence we only need this step specifically for the ARM64-Linux runner.
JoshuaMoelans
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This is a bit of an experiment to see if we run into similar problems to those we're seeing here when bumping to .net 10. We only see the problems in CI, so thinking the same may be true here as well.