Skip to content

C#: Fix autobuild on macos without mono #19251

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Apr 15, 2025
+65 −16
Merged

C#: Fix autobuild on macos without mono #19251

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
159d31d
Reenable problematic test
tamasvajk Apr 8, 2025
51388f2
Do not try running mono when it's not available on the runner
tamasvajk Apr 11, 2025
91daca1
Improve code quality based on PR review
tamasvajk Apr 14, 2025
a70536f
Improve code quality
tamasvajk Apr 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 30 additions & 5 deletions csharp/autobuilder/Semmle.Autobuild.CSharp.Tests/BuildScripts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ bool IBuildActions.FileExists(string file)

public IList<string> RunProcessIn { get; } = new List<string>();
public IDictionary<string, int> RunProcess { get; } = new Dictionary<string, int>();

/// <summary>
/// (process-exit code) pairs for commands that are executed during the assembly of the autobuild script.
/// </summary>
public IDictionary<string, int> RunProcessExecuteDuring { get; } = new Dictionary<string, int>();
public IDictionary<string, string> RunProcessOut { get; } = new Dictionary<string, string>();
public IDictionary<string, string> RunProcessWorkingDirectory { get; } = new Dictionary<string, string>();
public HashSet<string> CreateDirectories { get; } = new HashSet<string>();
Expand All @@ -66,7 +71,7 @@ int IBuildActions.RunProcess(string cmd, string args, string? workingDirectory,
if (wd != workingDirectory)
throw new ArgumentException($"Unexpected RunProcessWorkingDirectory, got {wd ?? "null"} expected {workingDirectory ?? "null"} in {pattern}");

if (!RunProcess.TryGetValue(pattern, out var ret))
if (!RunProcess.TryGetValue(pattern, out var ret) && !RunProcessExecuteDuring.TryGetValue(pattern, out ret))
throw new ArgumentException("Missing RunProcess " + pattern);

return ret;
Expand All @@ -81,7 +86,7 @@ int IBuildActions.RunProcess(string cmd, string args, string? workingDirectory,
if (wd != workingDirectory)
throw new ArgumentException($"Unexpected RunProcessWorkingDirectory, got {wd ?? "null"} expected {workingDirectory ?? "null"} in {pattern}");

if (!RunProcess.TryGetValue(pattern, out var ret))
if (!RunProcess.TryGetValue(pattern, out var ret) && !RunProcessExecuteDuring.TryGetValue(pattern, out ret))
throw new ArgumentException("Missing RunProcess " + pattern);

return ret;
Expand Down Expand Up @@ -797,11 +802,32 @@ public void TestDirsProjWindows()
}

[Fact]
public void TestDirsProjLinux()
public void TestDirsProjLinux_WithMono()
{
actions.RunProcessExecuteDuring[@"mono --version"] = 0;

actions.RunProcess[@"nuget restore C:\Project/dirs.proj -DisableParallelProcessing"] = 1;
actions.RunProcess[@"mono scratch/.nuget/nuget.exe restore C:\Project/dirs.proj -DisableParallelProcessing"] = 0;
actions.RunProcess[@"msbuild C:\Project/dirs.proj /t:rebuild"] = 0;

var autobuilder = TestDirsProjLinux();
TestAutobuilderScript(autobuilder, 0, 3);
}

[Fact]
public void TestDirsProjLinux_WithoutMono()
{
actions.RunProcessExecuteDuring[@"mono --version"] = 1;

actions.RunProcess[@"dotnet msbuild /t:restore C:\Project/dirs.proj"] = 0;
actions.RunProcess[@"dotnet msbuild C:\Project/dirs.proj /t:rebuild"] = 0;

var autobuilder = TestDirsProjLinux();
TestAutobuilderScript(autobuilder, 0, 2);
}

private CSharpAutobuilder TestDirsProjLinux()
{
actions.FileExists["csharp.log"] = true;
actions.FileExists[@"C:\Project/a/test.csproj"] = true;
actions.FileExists[@"C:\Project/dirs.proj"] = true;
Expand Down Expand Up @@ -830,8 +856,7 @@ public void TestDirsProjLinux()
</Project>");
actions.LoadXml[@"C:\Project/dirs.proj"] = dirsproj;

var autobuilder = CreateAutoBuilder(false);
TestAutobuilderScript(autobuilder, 0, 3);
return CreateAutoBuilder(false);
}

[Fact]
Expand Down
20 changes: 13 additions & 7 deletions csharp/autobuilder/Semmle.Autobuild.Shared/MsBuildRule.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,13 @@ internal static class MsBuildCommandExtensions
/// <summary>
/// Appends a call to msbuild.
/// </summary>
/// <param name="cmdBuilder"></param>
/// <param name="builder"></param>
/// <returns></returns>
public static CommandBuilder MsBuildCommand(this CommandBuilder cmdBuilder, IAutobuilder<AutobuildOptionsShared> builder)
public static CommandBuilder MsBuildCommand(this CommandBuilder cmdBuilder, IAutobuilder<AutobuildOptionsShared> builder, bool preferDotnet)
{
// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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 😄

? cmdBuilder.RunCommand("dotnet").Argument("msbuild")
: cmdBuilder.RunCommand("msbuild");
}
Expand Down Expand Up @@ -75,13 +73,21 @@ BuildScript GetNugetRestoreScript() =>
QuoteArgument(projectOrSolution.FullPath).
Argument("-DisableParallelProcessing").
Script;

BuildScript GetMonoVersionScript() => new CommandBuilder(builder.Actions).
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I pushed a commit with your idea.

RunCommand("mono").
Argument("--version").
Script;

var preferDotnet = !builder.Actions.IsWindows() && GetMonoVersionScript().Run(builder.Actions, (_, _) => { }, (_, _, _) => { }) != 0;

var nugetRestore = GetNugetRestoreScript();
var msbuildRestoreCommand = new CommandBuilder(builder.Actions).
MsBuildCommand(builder).
MsBuildCommand(builder, preferDotnet).
Argument("/t:restore").
QuoteArgument(projectOrSolution.FullPath);

if (builder.Actions.IsRunningOnAppleSilicon())
if (builder.Actions.IsRunningOnAppleSilicon() || preferDotnet)
{
// On Apple Silicon, only try package restore with `dotnet msbuild /t:restore`
ret &= BuildScript.Try(msbuildRestoreCommand.Script);
Expand Down Expand Up @@ -119,7 +125,7 @@ BuildScript GetNugetRestoreScript() =>
command.RunCommand("set Platform=&& type NUL", quoteExe: false);
}

command.MsBuildCommand(builder);
command.MsBuildCommand(builder, preferDotnet);
command.QuoteArgument(projectOrSolution.FullPath);

var target = "rebuild";
Expand Down
6 changes: 0 additions & 6 deletions csharp/ql/integration-tests/all-platforms/diag_missing_project_files/test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,2 @@
import pytest
import runs_on


# Skipping the test on macos-15, as we're running into trouble.
@pytest.mark.only_if(not runs_on.macos_15)
def test(codeql, csharp):
codeql.database.create(_assert_failure=True)