C#: Fix autobuild on macos without mono #19251
Conversation
tamasvajk
force-pushed
the
fix/macos15
branch
4 times, most recently
from
40776c2 to
68da44e
Compare
tamasvajk
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR improves the autobuild process for C# by addressing issues with msbuild execution on macOS without mono and by enhancing test coverage for different operating system scenarios.
- Updated the MsBuildCommand method to conditionally use dotnet msbuild based on a new parameter and operating system checks.
- Removed the previously skipped decorator in an integration test to run tests unconditionally.
- Added new test cases for Linux scenarios with and without mono.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| csharp/ql/integration-tests/all-platforms/diag_missing_project_files/test.py | Removed the macOS-specific skip to allow the test to run on all platforms. |
| csharp/autobuilder/Semmle.Autobuild.Shared/MsBuildRule.cs | Updated the msbuild command logic to include a new boolean parameter for preferring dotnet. |
| csharp/autobuilder/Semmle.Autobuild.CSharp.Tests/BuildScripts.cs | Added new test cases and adjusted execution mappings for mono/dotnet scenarios. |
Comments suppressed due to low confidence (3)
csharp/ql/integration-tests/all-platforms/diag_missing_project_files/test.py:1
- The removal of the macOS-15 skip may lead to unexpected failures if underlying issues persist; please verify that tests run successfully on macOS.
def test(codeql, csharp):
csharp/autobuilder/Semmle.Autobuild.CSharp.Tests/BuildScripts.cs:805
- [nitpick] Consider standardizing the test method names for clarity; for example, using names that clearly distinguish between scenarios when mono is available versus when it is not.
public void TestDirsProjLinux_WithMono()
csharp/autobuilder/Semmle.Autobuild.Shared/MsBuildRule.cs:19
- Combining the Apple Silicon check with the preferDotnet flag forces 'dotnet msbuild' when preferDotnet is true; please confirm that this behavior is intended for all non-Windows platforms.
return builder.Actions.IsRunningOnAppleSilicon() || preferDotnet
| @@ -75,13 +73,21 @@ BuildScript GetNugetRestoreScript() => | |||
| QuoteArgument(projectOrSolution.FullPath). | |||
| Argument("-DisableParallelProcessing"). | |||
| Script; | |||
|
|
|||
| BuildScript GetMonoVersionScript() => new CommandBuilder(builder.Actions). | |||
There was a problem hiding this comment.
Maybe rename to IsMonoInstalled (even though an exit code is returned - the important thing is whether mono is installed)?
Would it be worth extending the IBuildActions interface (with a method returning af bool) instead of making a script that is immediately executed?
Then it is possible to make an implementation like
public bool UseDotnet() => IsRunningOnAppleSilicon() || (!IsWIndows() && !IsMonoInstalled())
which can be used in MsBuildCommand and on l. 90.
There was a problem hiding this comment.
Thank you. I pushed a commit with your idea.
| @@ -261,6 +266,25 @@ bool IBuildActions.IsRunningOnAppleSilicon() | |||
| } | |||
| } | |||
|
|
|||
| bool IBuildActions.IsMonoInstalled() | |||
| { | ||
| // mono doesn't ship with `msbuild` on Arm-based Macs, but we can fall back to | ||
| // msbuild that ships with `dotnet` which can be invoked with `dotnet msbuild` | ||
| // perhaps we should do this on all platforms? | ||
| return builder.Actions.IsRunningOnAppleSilicon() | ||
| return builder.Actions.IsRunningOnAppleSilicon() || preferDotnet |
There was a problem hiding this comment.
Nit/Scope creep: There is a possible performance optimization, where we can avoid making multiple calls to sysctl machdep.cpu.brand_string.
In IBuildActions we could declare a method that returns the value of builder.Actions.IsRunningOnAppleSilicon() || !builder.Actions.IsWindows() && !builder.Actions.IsMonoInstalled(); and stores it in backing field for subsequent uses. (It is the same condition that is used on l. 19 and l. 85).
Also perfectly fine, if you don't want to change it 😄
This pull request includes several changes to improve the handling of the autobuild process, particularly for different operating systems and scenarios where
monois or isn't available. The main changes involve updating the process execution logic and adding new test cases for better coverage.