-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Sdk Validation Fix #2511
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
Sdk Validation Fix #2511
Changes from all commits
bb5980a
c2f40c1
fc8c22c
0e426ea
3217c28
edeb125
61780c9
5986aec
5f89f15
3df3b0b
da1dae4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,12 +11,12 @@ namespace BenchmarkDotNet.Toolchains.MonoWasm | |
[PublicAPI] | ||
public class WasmToolchain : Toolchain | ||
{ | ||
private string CustomDotNetCliPath { get; } | ||
private readonly string CustomDotNetCliPath; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this field is needed. |
||
|
||
private WasmToolchain(string name, IGenerator generator, IBuilder builder, IExecutor executor, string customDotNetCliPath) | ||
: base(name, generator, builder, executor) | ||
private WasmToolchain(string name, IGenerator generator, IBuilder builder, IExecutor executor, ISdkProvider sdkProvider) | ||
: base(name, generator, builder, executor, sdkProvider) | ||
{ | ||
CustomDotNetCliPath = customDotNetCliPath; | ||
CustomDotNetCliPath = sdkProvider.CustomDotNetCliPath; | ||
} | ||
|
||
public override IEnumerable<ValidationError> Validate(BenchmarkCase benchmarkCase, IResolver resolver) | ||
|
@@ -51,6 +51,6 @@ public static IToolchain From(NetCoreAppSettings netCoreAppSettings) | |
// aot builds can be very slow | ||
logOutput: netCoreAppSettings.AOTCompilerMode == MonoAotLLVM.MonoAotCompilerMode.wasm), | ||
new WasmExecutor(), | ||
netCoreAppSettings.CustomDotNetCliPath); | ||
new DotNetSdkProvider()); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Diagnostics; | ||
using System.Linq; | ||
using System.Runtime.InteropServices; | ||
|
||
namespace BenchmarkDotNet.Validators | ||
{ | ||
public class DotNetSdkProvider : ISdkProvider | ||
{ | ||
private string _customDotNetCliPath; | ||
|
||
public string CustomDotNetCliPath | ||
{ | ||
get => _customDotNetCliPath; | ||
set => _customDotNetCliPath = value; | ||
} | ||
Comment on lines
+13
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just make this a read only property and pass the string into the constructor (can make it an optional argument). |
||
public IEnumerable<string> GetInstalledSdks() | ||
{ | ||
var installedDotNetSdks = GetInstalledDotNetSdk(); | ||
|
||
var installedFrameworkSdks = GetInstalledFrameworkSdks(); | ||
|
||
return installedDotNetSdks.Concat(installedFrameworkSdks); | ||
} | ||
|
||
private IEnumerable<string> GetInstalledDotNetSdk() | ||
{ | ||
string dotnetExecutable = string.IsNullOrEmpty(CustomDotNetCliPath) ? "dotnet" : CustomDotNetCliPath; | ||
|
||
var startInfo = new ProcessStartInfo(dotnetExecutable, "--list-sdks") | ||
{ | ||
RedirectStandardOutput = true, | ||
UseShellExecute = false, | ||
CreateNoWindow = true | ||
}; | ||
|
||
using (var process = Process.Start(startInfo)) | ||
{ | ||
if (process == null) throw new InvalidOperationException("Failed to start the dotnet process."); | ||
|
||
process.WaitForExit(); | ||
|
||
var output = process.StandardOutput.ReadToEnd(); | ||
var lines = output.Split(new[] { '\r', '\n' }, StringSplitOptions.RemoveEmptyEntries); | ||
|
||
return lines.Select(line => line.Split(' ')[0]); // The SDK version is the first part of each line. | ||
} | ||
} | ||
|
||
private IEnumerable<string> GetInstalledFrameworkSdks() | ||
{ | ||
var versions = new List<string>(); | ||
|
||
// Skip .NET Framework check on macOS and Linux | ||
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | ||
{ | ||
#pragma warning disable CA1416 | ||
using (var ndpKey = Microsoft.Win32.RegistryKey.OpenBaseKey(Microsoft.Win32.RegistryHive.LocalMachine, Microsoft.Win32.RegistryView.Registry32) | ||
.OpenSubKey("SOFTWARE\\Microsoft\\NET Framework Setup\\NDP\\")) | ||
{ | ||
foreach (var versionKeyName in ndpKey.GetSubKeyNames()) | ||
{ | ||
if (versionKeyName.StartsWith("v")) | ||
{ | ||
var versionKey = ndpKey.OpenSubKey(versionKeyName); | ||
var version = versionKey.GetValue("Version", "").ToString(); | ||
var sp = versionKey.GetValue("SP", "").ToString(); | ||
if (!string.IsNullOrEmpty(version)) | ||
versions.Add(version + (string.IsNullOrEmpty(sp) ? "" : $" SP{sp}")); | ||
} | ||
} | ||
} | ||
#pragma warning restore CA1416 | ||
} | ||
|
||
return versions; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
using System.Collections.Generic; | ||
|
||
namespace BenchmarkDotNet.Validators | ||
{ | ||
public interface ISdkProvider | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, I think this interface should be moved to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes perfect sense. I appreciate the valuable insight. |
||
{ | ||
IEnumerable<string> GetInstalledSdks(); | ||
string CustomDotNetCliPath { get; } | ||
} | ||
} |
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 appears that MonoAotToolchain uses a variant of Roslyn. So it doesn't need the ISdkProvider (unless Roslyn toolchain needs to validate the sdk also, but it seems different than csproj toolchains).
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.
ah ok. Good info. I will definitely need a different approach in implementing this validation to just certain toolchains then.