Skip to content
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

Jump to bottom

Change .NET SDK Validator to account for backwards compatibility #2645

Merged
merged 1 commit into from
Sep 23, 2024
Merged

Change .NET SDK Validator to account for backwards compatibility #2645

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
117 changes: 66 additions & 51 deletions src/BenchmarkDotNet/Validators/DotNetSdkVersionValidator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,34 +21,28 @@ public static IEnumerable<ValidationError> ValidateCoreSdks(string? customDotNet
{
yield return cliPathError;
}
else if (TryGetSdkVersion(benchmark, out string requiredSdkVersion))
else if (TryGetSdkVersion(benchmark, out Version requiredSdkVersion))
{
var installedSdks = GetInstalledDotNetSdks(customDotNetCliPath);
if (!installedSdks.Any(sdk => sdk.StartsWith(requiredSdkVersion)))
if (!installedSdks.Any(sdk => sdk >= requiredSdkVersion))
{
yield return new ValidationError(true, $"The required .NET Core SDK version {requiredSdkVersion} for runtime moniker {benchmark.Job.Environment.Runtime.RuntimeMoniker} is not installed.", benchmark);
yield return new ValidationError(true, $"The required .NET Core SDK version {requiredSdkVersion} or higher for runtime moniker {benchmark.Job.Environment.Runtime.RuntimeMoniker} is not installed.", benchmark);
}
}
}

public static IEnumerable<ValidationError> ValidateFrameworkSdks(BenchmarkCase benchmark)
{
if (!TryGetSdkVersion(benchmark, out string requiredSdkVersionString))
if (!TryGetSdkVersion(benchmark, out Version requiredSdkVersion))
{
yield break;
}

if (!Version.TryParse(requiredSdkVersionString, out var requiredSdkVersion))
{
yield return new ValidationError(true, $"Invalid .NET Framework SDK version format: {requiredSdkVersionString}", benchmark);
yield break;
}

var installedVersionString = cachedFrameworkSdks.Value.FirstOrDefault();

if (installedVersionString == null || Version.TryParse(installedVersionString, out var installedVersion) && installedVersion < requiredSdkVersion)
{
yield return new ValidationError(true, $"The required .NET Framework SDK version {requiredSdkVersionString} or higher is not installed.", benchmark);
yield return new ValidationError(true, $"The required .NET Framework SDK version {requiredSdkVersion} or higher is not installed.", benchmark);
}
}

Expand Down Expand Up @@ -77,9 +71,9 @@ public static bool IsCliPathInvalid(string customDotNetCliPath, BenchmarkCase be
return false;
}

private static bool TryGetSdkVersion(BenchmarkCase benchmark, out string sdkVersion)
private static bool TryGetSdkVersion(BenchmarkCase benchmark, out Version sdkVersion)
{
sdkVersion = string.Empty;
sdkVersion = default;
if (benchmark?.Job?.Environment?.Runtime?.RuntimeMoniker != null)
{
sdkVersion = GetSdkVersionFromMoniker(benchmark.Job.Environment.Runtime.RuntimeMoniker);
Expand All @@ -88,7 +82,7 @@ private static bool TryGetSdkVersion(BenchmarkCase benchmark, out string sdkVers
return false;
}

private static IEnumerable<string> GetInstalledDotNetSdks(string? customDotNetCliPath)
private static IEnumerable<Version> GetInstalledDotNetSdks(string? customDotNetCliPath)
{
string dotnetExecutable = string.IsNullOrEmpty(customDotNetCliPath) ? "dotnet" : customDotNetCliPath;
var startInfo = new ProcessStartInfo(dotnetExecutable, "--list-sdks")
Expand All @@ -104,7 +98,7 @@ private static IEnumerable<string> GetInstalledDotNetSdks(string? customDotNetCl
{
if (process == null)
{
return Enumerable.Empty<string>();
return Enumerable.Empty<Version>();
}

process.WaitForExit();
Expand All @@ -113,17 +107,38 @@ private static IEnumerable<string> GetInstalledDotNetSdks(string? customDotNetCl
{
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.

var versions = new List<Version>(lines.Count());
foreach (var line in lines)
{
// Each line will start with the SDK version followed by the SDK path. Since we only support
// targeting SDK versions by the major version, we'll just look at extracting the major and
// minor versions. This is done by looking for the first two dots in the line.
var firstDot = line.IndexOf('.');
if (firstDot < 0)
continue;

var secondDot = line.IndexOf('.', firstDot + 1);
if (secondDot < 0)
continue;

if (Version.TryParse(line.Substring(0, secondDot), out var version))
{
versions.Add(version);
}
}

return versions;
Comment on lines +111 to +131
Copy link
Member

Choose a reason for hiding this comment

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

Do need to account for framework versions too? i.e.

Suggested change
var versions = new List<Version>(lines.Count());
foreach (var line in lines)
{
// Each line will start with the SDK version followed by the SDK path. Since we only support
// targeting SDK versions by the major version, we'll just look at extracting the major and
// minor versions. This is done by looking for the first two dots in the line.
var firstDot = line.IndexOf('.');
if (firstDot < 0)
continue;
var secondDot = line.IndexOf('.', firstDot + 1);
if (secondDot < 0)
continue;
if (Version.TryParse(line.Substring(0, secondDot), out var version))
{
versions.Add(version);
}
}
return versions;
return lines.Select(line => Version.Parse(line.Split(' ')[0]));

Copy link
Member

Choose a reason for hiding this comment

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

@timcassell this will break net framework since it is accounting for only the major version. Sometimes like net472 will break? Suggested code was fixing that.

Copy link
Collaborator

@timcassell timcassell Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Framework sdk versions are retrieved in a separate method.

Copy link
Member

Choose a reason for hiding this comment

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

There are also few others: netcoreapp3.1 and netstandrd2.1 etc. (not sure if tests are exercising them, but we do have tests for those TFMs).

Copy link
Collaborator

Choose a reason for hiding this comment

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

This code is comparing major and minor version, so netcoreapp3.1 should work fine.
BDN doesn't support netstandard builds (it must be executable).

}
else
{
return Enumerable.Empty<string>();
return Enumerable.Empty<Version>();
}
}
}
catch (Win32Exception) // dotnet CLI is not installed or not found in the path.
{
return Enumerable.Empty<string>();
return Enumerable.Empty<Version>();
}
}

Expand Down Expand Up @@ -193,42 +208,42 @@ private static string CheckFor45PlusVersion(int releaseKey)
return "";
}

private static string GetSdkVersionFromMoniker(RuntimeMoniker runtimeMoniker)
private static Version GetSdkVersionFromMoniker(RuntimeMoniker runtimeMoniker)
{
return runtimeMoniker switch
{
RuntimeMoniker.Net461 => "4.6.1",
RuntimeMoniker.Net462 => "4.6.2",
RuntimeMoniker.Net47 => "4.7",
RuntimeMoniker.Net471 => "4.7.1",
RuntimeMoniker.Net472 => "4.7.2",
RuntimeMoniker.Net48 => "4.8",
RuntimeMoniker.Net481 => "4.8.1",
RuntimeMoniker.NetCoreApp31 => "3.1",
RuntimeMoniker.Net50 => "5.0",
RuntimeMoniker.Net60 => "6.0",
RuntimeMoniker.Net70 => "7.0",
RuntimeMoniker.Net80 => "8.0",
RuntimeMoniker.Net90 => "9.0",
RuntimeMoniker.NativeAot60 => "6.0",
RuntimeMoniker.NativeAot70 => "7.0",
RuntimeMoniker.NativeAot80 => "8.0",
RuntimeMoniker.NativeAot90 => "9.0",
RuntimeMoniker.Mono60 => "6.0",
RuntimeMoniker.Mono70 => "7.0",
RuntimeMoniker.Mono80 => "8.0",
RuntimeMoniker.Mono90 => "9.0",
RuntimeMoniker.Wasm => Portability.RuntimeInformation.IsNetCore && CoreRuntime.TryGetVersion(out var version) ? $"{version.Major}.{version.Minor}" : "5.0",
RuntimeMoniker.WasmNet50 => "5.0",
RuntimeMoniker.WasmNet60 => "6.0",
RuntimeMoniker.WasmNet70 => "7.0",
RuntimeMoniker.WasmNet80 => "8.0",
RuntimeMoniker.WasmNet90 => "9.0",
RuntimeMoniker.MonoAOTLLVM => Portability.RuntimeInformation.IsNetCore && CoreRuntime.TryGetVersion(out var version) ? $"{version.Major}.{version.Minor}" : "6.0",
RuntimeMoniker.MonoAOTLLVMNet60 => "6.0",
RuntimeMoniker.MonoAOTLLVMNet70 => "7.0",
RuntimeMoniker.MonoAOTLLVMNet80 => "8.0",
RuntimeMoniker.MonoAOTLLVMNet90 => "9.0",
RuntimeMoniker.Net461 => new Version(4, 6, 1),
RuntimeMoniker.Net462 => new Version(4, 6, 2),
RuntimeMoniker.Net47 => new Version(4, 7),
RuntimeMoniker.Net471 => new Version(4, 7, 1),
RuntimeMoniker.Net472 => new Version(4, 7, 2),
RuntimeMoniker.Net48 => new Version(4, 8),
RuntimeMoniker.Net481 => new Version(4, 8, 1),
RuntimeMoniker.NetCoreApp31 => new Version(3, 1),
RuntimeMoniker.Net50 => new Version(5, 0),
RuntimeMoniker.Net60 => new Version(6, 0),
RuntimeMoniker.Net70 => new Version(7, 0),
RuntimeMoniker.Net80 => new Version(8, 0),
RuntimeMoniker.Net90 => new Version(9, 0),
RuntimeMoniker.NativeAot60 => new Version(6, 0),
RuntimeMoniker.NativeAot70 => new Version(7, 0),
RuntimeMoniker.NativeAot80 => new Version(8, 0),
RuntimeMoniker.NativeAot90 => new Version(9, 0),
RuntimeMoniker.Mono60 => new Version(6, 0),
RuntimeMoniker.Mono70 => new Version(7, 0),
RuntimeMoniker.Mono80 => new Version(8, 0),
RuntimeMoniker.Mono90 => new Version(9, 0),
RuntimeMoniker.Wasm => Portability.RuntimeInformation.IsNetCore && CoreRuntime.TryGetVersion(out var version) ? version : new Version(5, 0),
RuntimeMoniker.WasmNet50 => new Version(5, 0),
RuntimeMoniker.WasmNet60 => new Version(6, 0),
RuntimeMoniker.WasmNet70 => new Version(7, 0),
RuntimeMoniker.WasmNet80 => new Version(8, 0),
RuntimeMoniker.WasmNet90 => new Version(9, 0),
RuntimeMoniker.MonoAOTLLVM => Portability.RuntimeInformation.IsNetCore && CoreRuntime.TryGetVersion(out var version) ? version : new Version(6, 0),
RuntimeMoniker.MonoAOTLLVMNet60 => new Version(6, 0),
RuntimeMoniker.MonoAOTLLVMNet70 => new Version(7, 0),
RuntimeMoniker.MonoAOTLLVMNet80 => new Version(8, 0),
RuntimeMoniker.MonoAOTLLVMNet90 => new Version(9, 0),
_ => throw new NotImplementedException($"SDK version check not implemented for {runtimeMoniker}")
};
}
Expand Down
Loading