-
-
Notifications
You must be signed in to change notification settings - Fork 978
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
Issue 2064: Mono70 moniker #2142
Conversation
@@ -19,6 +19,7 @@ public class CoreRuntime : Runtime | |||
public static readonly CoreRuntime Core50 = new CoreRuntime(RuntimeMoniker.Net50, "net5.0", ".NET 5.0"); | |||
public static readonly CoreRuntime Core60 = new CoreRuntime(RuntimeMoniker.Net60, "net6.0", ".NET 6.0"); | |||
public static readonly CoreRuntime Core70 = new CoreRuntime(RuntimeMoniker.Net70, "net7.0", ".NET 7.0"); | |||
public static readonly CoreRuntime Mono70 = new CoreRuntime(RuntimeMoniker.Mono70, "mono7.0", "Mono with .NET 7.0"); |
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.
Not sure if this should be here or in MonoRuntime
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.
I agree, MonoRuntime
would most likely be a better place.
@@ -25,7 +25,7 @@ internal static class RuntimeInformation | |||
internal const string ReleaseConfigurationName = "RELEASE"; | |||
internal const string Unknown = "?"; | |||
|
|||
public static bool IsMono { get; } = Type.GetType("Mono.Runtime") != null; // it allocates a lot of memory, we need to check it once in order to keep Engine non-allocating! | |||
public static bool IsMono { get; } = Type.GetType("Mono.RuntimeStructs") != null; // it allocates a lot of memory, we need to check it once in order to keep Engine non-allocating! |
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.
Does this still return true on the old Mono runtime?
Mono checks are performed elsewhere in the library like in the memory diagnoser (GcStats
) since it did not support AppDomain.MonitoringIsEnabled
. Is that still the case with the new Mono runtime?
It's also checked in the MonoDisassembler
, and I have no idea if that's relevant here.
And other places in RuntimeInformation.cs, all of which should be verified.
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.
I have checked it through this sample running on latest Mono 6.12.0
and Mono 5.0.0.100 (2017-05-11)
. The property returns true
on those versions.
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.
Ok, I just tested (using Adam's example), and it seems monitoring is still not available on the new Core/Mono runtime. I would assume everything else behaves the same, too (MonoDisassembler
), but I didn't test.
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.
@Serg046 I am impressed that you have selected such ambitious task for first contribution!
It looks promising, but we need a new toolchain to make sure it actually runs Mono. PTAL at my comments. Thank you!
MonoAOTLLVMNet70, | ||
|
||
/// <summary> | ||
/// Mono with .net7.0 |
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.
nit: the comment could be improved
/// Mono with .net7.0 | |
/// .NET 7 using MonoVM (not CLR which is the default) |
@@ -410,11 +408,21 @@ private static Job CreateJobForGivenRuntime(Job baseJob, string runtimeId, Comma | |||
return MakeMonoAOTLLVMJob(baseJob, options, "net6.0"); | |||
case RuntimeMoniker.MonoAOTLLVMNet70: | |||
return MakeMonoAOTLLVMJob(baseJob, options, "net7.0"); | |||
case RuntimeMoniker.Mono70: | |||
return CreateCoreJob(baseJob, "net7.0", "Mono with .NET 7.0", options, CoreRuntime.Core70) | |||
.WithArguments(new Argument[] { new MsBuildArgument("/p:UseMonoRuntime=true") }); |
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.
I am afraid that this might not be enough. I've failed to find any official documentation (this is sad), but according to https://twitter.com/lambdageek/status/1457784508371980291 we need to specify /p:UseMonoRuntime=true
and publish a self-contained application for specific runtime.
Here is a small repro:
Console.WriteLine(Type.GetType("Mono.RuntimeStructs") != null);
PS D:\projects\repros\consoleBdn> dotnet publish -c Release -f net7.0 --self-contained -r win-x64 /p:UseMonoRuntime=true
MSBuild version 17.4.0-preview-22428-01+14c24b2d3 for .NET
Determining projects to restore...
Restored D:\projects\repros\consoleBdn\consoleBdn.csproj (in 11 ms).
C:\Program Files\dotnet\sdk\7.0.100-rc.1.22431.12\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.RuntimeIdentifierInference.targets(219,5): message NETSDK1057: You are using a preview version of .NET. See: https://aka.ms/dotnet-support-p
olicy [D:\projects\repros\consoleBdn\consoleBdn.csproj]
consoleBdn -> D:\projects\repros\consoleBdn\bin\Release\net7.0\win-x64\consoleBdn.dll
consoleBdn -> D:\projects\repros\consoleBdn\bin\Release\net7.0\win-x64\publish\
PS D:\projects\repros\consoleBdn> D:\projects\repros\consoleBdn\bin\Release\net7.0\win-x64\publish\consoleBdn.exe
True
PS D:\projects\repros\consoleBdn> dotnet build -c Release -f net7.0 /p:UseMonoRuntime=true
MSBuild version 17.4.0-preview-22428-01+14c24b2d3 for .NET
Determining projects to restore...
Restored D:\projects\repros\consoleBdn\consoleBdn.csproj (in 5 ms).
C:\Program Files\dotnet\sdk\7.0.100-rc.1.22431.12\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.RuntimeIdentifierInference.targets(219,5): message NETSDK1057: You are using a preview version of .NET. See: https://aka.ms/dotnet-support-p
olicy [D:\projects\repros\consoleBdn\consoleBdn.csproj]
consoleBdn -> D:\projects\repros\consoleBdn\bin\Release\net7.0\consoleBdn.dll
Build succeeded.
0 Warning(s)
0 Error(s)
Time Elapsed 00:00:00.29
PS D:\projects\repros\consoleBdn> dotnet D:\projects\repros\consoleBdn\bin\Release\net7.0\consoleBdn.dll
False
{ | ||
return baseJob | ||
.WithRuntime(runtime) | ||
.WithToolchain(CsProjCoreToolchain.From(new NetCoreAppSettings(runtimeId, null, runtimeName, options.CliPath?.FullName, options.RestorePath?.FullName))); |
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.
CsProjCoreToolchain
uses DotNetCliBuilder
for building:
new DotNetCliBuilder(settings.TargetFrameworkMoniker, settings.CustomDotNetCliPath), |
which performs dotnet build
to build the benchmark:
.RestoreThenBuild(); |
and then DotNetCliExecutor
to run the executable:
new DotNetCliExecutor(settings.CustomDotNetCliPath), |
the executor basically performs dotnet $pathToLibrary.dll
to run the benchmarks:
$"{executableName.EscapeCommandLine()} {benchmarkId.ToArguments(inputFromBenchmark.GetClientHandleAsString(), acknowledgments.GetClientHandleAsString())}", |
You most likely need to define a new toolchain that consists of:
CsProjGenerator
same way asCsProjCoreToolchain
does:new CsProjGenerator(settings.TargetFrameworkMoniker, settings.CustomDotNetCliPath, settings.PackagesPath, settings.RuntimeFrameworkVersion), DotNetCliPublisher
withextraArguments
set to-r $runtimeId
:public DotNetCliPublisher(string customDotNetCliPath = null, string extraArguments = null, IReadOnlyList<EnvironmentVariable> environmentVariables = null)
You can take a look atNatvieAOTToolchain
to see how it provides the runtime id:public static string GetExtraArguments(string runtimeIdentifier) => $"-r {runtimeIdentifier}"; Executor
that runs provided.exe
rather thandotnet $pathTo.dll
: https://github.com/dotnet/BenchmarkDotNet/blob/1f5637c784ad9887a2d5abdba54d1337655281b1/src/BenchmarkDotNet/Toolchains/Executor.cs
The tricky part will be to make sure that ExecutablePath
provided by CsProjGenerator
is the one that points to the executable build by dotnet publish
, not the dll
build by dotnet build
.
It's used here by the executor:
string exePath = executeParameters.BuildResult.ArtifactsPaths.ExecutablePath; |
It comes from here:
protected virtual string GetExecutablePath(string binariesDirectoryPath, string programName) => Path.Combine(binariesDirectoryPath, $"{programName}{GetExecutableExtension()}"); |
=> Path.Combine(buildArtifactsDirectoryPath, "bin", configuration, TargetFrameworkMoniker); |
You might need to introduce a new type that derives from CsProjGenerator
and does this:
BenchmarkDotNet/src/BenchmarkDotNet/Toolchains/NativeAot/Generator.cs
Lines 66 to 67 in 1f5637c
protected override string GetBinariesDirectoryPath(string buildArtifactsDirectoryPath, string configuration) | |
=> Path.Combine(buildArtifactsDirectoryPath, "bin", configuration, TargetFrameworkMoniker, runtimeIdentifier, "publish"); |
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.
Thanks for very well-detailed explanation. Everything is clear, just few notes you might want to comment on:
DotNetCliPublisher with extraArguments set to -r $runtimeId
I will do this because I see there is a way to obtain it automatically anyway (so that it is still user-friendly). But your small repro
returns true
with dotnet publish -c Release -f net6.0 --self-contained -r win-x64 /p:UseMonoRuntime=true
as well as with dotnet publish -c Release -f net6.0 --self-contained /p:UseMonoRuntime=true
on my local Windows 10.
Executor that runs provided .exe rather than dotnet $pathTo.dll:
Same thing here. Both .exe
execution and dotnet $pathTo.dll
works fine locally.
However, I am not sure what happens on Linux and Mac. I can check on Linux using Docker or WSL if you are interested in, otherwise I will just do what you suggested (just ignore these notes then).
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.
dotnet publish -c Release -f net6.0 --self-contained /p:UseMonoRuntime=true
This is something new to me, I remember dotnet SDK always complaining about lack of runtime id when running --self-contained
. But the produced exe is put to the runtime-specific folder, so we might need it anyway:
PS D:\projects\repros\consoleBdn> dotnet publish -c Release -f net7.0 --self-contained /p:UseMonoRuntime=true
MSBuild version 17.4.0-preview-22428-01+14c24b2d3 for .NET
Determining projects to restore...
All projects are up-to-date for restore.
C:\Program Files\dotnet\sdk\7.0.100-rc.1.22431.12\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.RuntimeIdentifierInference.targets(219,5): message NETSDK1057: You are using a preview version of .NET. See: https://aka.ms/dotnet-support-p
olicy [D:\projects\repros\consoleBdn\consoleBdn.csproj]
consoleBdn -> D:\projects\repros\consoleBdn\bin\Release\net7.0\win-x64\consoleBdn.dll
consoleBdn -> D:\projects\repros\consoleBdn\bin\Release\net7.0\win-x64\publish\
However, I am not sure what happens on Linux and Mac
If you add a test similar to what I've described in #2142 (comment) and it works on your Windows machine it should work on Linux and Mac too and it's enough to just use CI to verify that.
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.
Thank you @adamsitnik, this is done. Could you please take a look?
@@ -19,6 +19,7 @@ public class CoreRuntime : Runtime | |||
public static readonly CoreRuntime Core50 = new CoreRuntime(RuntimeMoniker.Net50, "net5.0", ".NET 5.0"); | |||
public static readonly CoreRuntime Core60 = new CoreRuntime(RuntimeMoniker.Net60, "net6.0", ".NET 6.0"); | |||
public static readonly CoreRuntime Core70 = new CoreRuntime(RuntimeMoniker.Net70, "net7.0", ".NET 7.0"); | |||
public static readonly CoreRuntime Mono70 = new CoreRuntime(RuntimeMoniker.Mono70, "mono7.0", "Mono with .NET 7.0"); |
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.
I agree, MonoRuntime
would most likely be a better place.
Assert.NotNull(toolchain); | ||
Assert.Equal(actualRuntimeId, ((DotNetCliGenerator)toolchain.Generator).TargetFrameworkMoniker); | ||
} | ||
|
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.
Please add tests similar to NativeAotTests where you ask to run a Mono .NET 6
benchmark (our CI uses .NET 6 SDK) and the benchmark just throws when it does not run on Mono. Like here where we do that for NativeAOT:
public void Check() | |
{ | |
if (!RuntimeInformation.IsNativeAOT) |
@@ -24,7 +24,7 @@ public class CsProjCoreToolchain : Toolchain, IEquatable<CsProjCoreToolchain> | |||
[PublicAPI] public static readonly IToolchain NetCoreApp60 = From(NetCoreAppSettings.NetCoreApp60); | |||
[PublicAPI] public static readonly IToolchain NetCoreApp70 = From(NetCoreAppSettings.NetCoreApp70); | |||
|
|||
private CsProjCoreToolchain(string name, IGenerator generator, IBuilder builder, IExecutor executor, string customDotNetCliPath) | |||
internal CsProjCoreToolchain(string name, IGenerator generator, IBuilder builder, IExecutor executor, string customDotNetCliPath) |
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.
I can keep it private but then I will have to inherit from Toolchain
in MonoToolchain
and copy/paste Validate
method
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.
@Serg046 making it internal was the right choice! 👍
protected override string GetRuntimeSettings(GcMode gcMode, IResolver resolver) | ||
{ | ||
// Workaround for 'Found multiple publish output files with the same relative path' error | ||
return base.GetRuntimeSettings(gcMode, resolver) + "<PropertyGroup><ErrorOnDuplicatePublishOutputFiles>false</ErrorOnDuplicatePublishOutputFiles></PropertyGroup>"; |
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.
Same issue #2079
buildPartition, | ||
EnvironmentVariables, | ||
buildPartition.Timeout) | ||
.Publish().ToBuildResult(generateResult); |
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.
The thing is that --self-contained
is not supported for restore/build operations that's why I introduce a new publisher and call publish
only here.
new MonoGenerator(settings.TargetFrameworkMoniker, settings.CustomDotNetCliPath, settings.PackagesPath, settings.RuntimeFrameworkVersion), | ||
new MonoPublisher( | ||
settings.CustomDotNetCliPath, | ||
$"--self-contained -r {runtimeIdentifier} /p:UseMonoRuntime=true /p:RuntimeIdentifiers={runtimeIdentifier}"), |
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.
I set RuntimeIdentifiers explicitly here because I had the issue describe here.
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.
For that it would be great to add a comment to the code and provide the link to the issue (so in the future we don't remove it by accident).
I also wonder if this logic should be moved to MonoPublisher
, as it's specific for this publisher and it won't ever be reused by other toolchains.
{ | ||
var config = ManualConfig.CreateEmpty().AddJob(Job.Dry.WithRuntime(MonoRuntime.Mono60)); | ||
CanExecute<MonoBenchmark>(config); | ||
} |
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.
Not sure if there is a need to add something like
if (ContinuousIntegration.IsAppVeyorOnWindows()) // too time consuming for AppVeyor (1h limit)
return;
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.
If AppVeyor CI builds are passing, we don't need it, we can always add it in the future when it becomes a problem,
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.
It looks very, very good!
I have one last request: could you please fix the runtime name displayed for the "new" Mono:
The code can be found here:
BenchmarkDotNet/src/BenchmarkDotNet/Portability/RuntimeInformation.cs
Lines 164 to 166 in c02c3d8
internal static string GetRuntimeVersion() | |
{ | |
if (IsMono) |
Thank you for your contribution!
{ | ||
return baseJob | ||
.WithRuntime(runtime) | ||
.WithToolchain(MonoToolchain.From(new NetCoreAppSettings(runtime.MsBuildMoniker, null, runtime.Name, options.CliPath?.FullName, options.RestorePath?.FullName))); |
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.
nit: it would be nice to use named parameters so it's obvious that all strings are passed as right arguments (when they are multiple arguments of the same type it's easy to provide them in wrong order).
You could refactor following logic to a separate method:
BenchmarkDotNet/src/BenchmarkDotNet/ConsoleArguments/ConfigParser.cs
Lines 534 to 539 in c02c3d8
new NetCoreAppSettings( | |
targetFrameworkMoniker: RuntimeInformation.GetCurrentRuntime().MsBuildMoniker, | |
customDotNetCliPath: options.CliPath?.FullName, | |
runtimeFrameworkVersion: null, | |
name: RuntimeInformation.GetCurrentRuntime().Name, | |
packagesPath: options.RestorePath?.FullName))); |
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.
Done 14a256e
@@ -24,7 +24,7 @@ public class CsProjCoreToolchain : Toolchain, IEquatable<CsProjCoreToolchain> | |||
[PublicAPI] public static readonly IToolchain NetCoreApp60 = From(NetCoreAppSettings.NetCoreApp60); | |||
[PublicAPI] public static readonly IToolchain NetCoreApp70 = From(NetCoreAppSettings.NetCoreApp70); | |||
|
|||
private CsProjCoreToolchain(string name, IGenerator generator, IBuilder builder, IExecutor executor, string customDotNetCliPath) | |||
internal CsProjCoreToolchain(string name, IGenerator generator, IBuilder builder, IExecutor executor, string customDotNetCliPath) |
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.
@Serg046 making it internal was the right choice! 👍
new MonoGenerator(settings.TargetFrameworkMoniker, settings.CustomDotNetCliPath, settings.PackagesPath, settings.RuntimeFrameworkVersion), | ||
new MonoPublisher( | ||
settings.CustomDotNetCliPath, | ||
$"--self-contained -r {runtimeIdentifier} /p:UseMonoRuntime=true /p:RuntimeIdentifiers={runtimeIdentifier}"), |
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.
For that it would be great to add a comment to the code and provide the link to the issue (so in the future we don't remove it by accident).
I also wonder if this logic should be moved to MonoPublisher
, as it's specific for this publisher and it won't ever be reused by other toolchains.
{ | ||
var config = ManualConfig.CreateEmpty().AddJob(Job.Dry.WithRuntime(MonoRuntime.Mono60)); | ||
CanExecute<MonoBenchmark>(config); | ||
} |
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.
If AppVeyor CI builds are passing, we don't need it, we can always add it in the future when it becomes a problem,
Co-authored-by: Adam Sitnik <[email protected]>
Co-authored-by: Adam Sitnik <[email protected]>
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.
Looks great to me @Serg046 ! Very impressive PR, thank you!
Not sure I understand the task properly, please take a look
#2064