Skip to content

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

Closed
wants to merge 11 commits into from
11 changes: 9 additions & 2 deletions src/BenchmarkDotNet/Running/BenchmarkRunnerClean.cs
Original file line number Diff line number Diff line change
Expand Up @@ -363,9 +363,16 @@ private static ImmutableArray<ValidationError> Validate(params BenchmarkRunInfo[
.Validate(new ValidationParameters(joinedCases, null))
);
}

var sdkValidator = SdkValidator.Instance;
foreach (var benchmarkRunInfo in benchmarks)
validationErrors.AddRange(benchmarkRunInfo.Config.GetCompositeValidator().Validate(new ValidationParameters(benchmarkRunInfo.BenchmarksCases, benchmarkRunInfo.Config)));
{
var validators = benchmarkRunInfo.Config.GetValidators().ToList();
validators.Add(sdkValidator);
var compositeValidator = new CompositeValidator(validators.ToImmutableHashSet());

var currentValidationErrors = compositeValidator.Validate(new ValidationParameters(benchmarkRunInfo.BenchmarksCases, benchmarkRunInfo.Config));
validationErrors.AddRange(currentValidationErrors);
}

return validationErrors.ToImmutableArray();
}
Expand Down
30 changes: 30 additions & 0 deletions src/BenchmarkDotNet/Validators/DotNetSdkProvider.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;

namespace BenchmarkDotNet.Validators
{
public class DotNetSdkProvider : ISdkProvider
{
public IEnumerable<string> GetInstalledSdks()
{
var startInfo = new ProcessStartInfo("dotnet", "--list-sdks")
{
RedirectStandardOutput = true,
UseShellExecute = false,
CreateNoWindow = true
};

var process = Process.Start(startInfo);
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.
}
}


}
9 changes: 9 additions & 0 deletions src/BenchmarkDotNet/Validators/ISdkProvider.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
using System.Collections.Generic;

namespace BenchmarkDotNet.Validators
{
public interface ISdkProvider
Copy link
Collaborator

Choose a reason for hiding this comment

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

ISdkProvider name implies that a different sdk besides dotnet could be used. Does that make sense? Does RoslynToolchain use an sdk that isn't dotnet?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, I think this interface should be moved to BenchmarkDotNet.Toolchains namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes perfect sense. I appreciate the valuable insight.

{
IEnumerable<string> GetInstalledSdks();
}
}
109 changes: 109 additions & 0 deletions src/BenchmarkDotNet/Validators/SdkValidator.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
using BenchmarkDotNet.Jobs;
using System;
using System.Collections.Generic;
using System.Linq;

namespace BenchmarkDotNet.Validators
{
public class SdkValidator : IValidator
{
private readonly ISdkProvider sdkProvider;

public static readonly SdkValidator Instance = new SdkValidator(new DotNetSdkProvider());

public SdkValidator(ISdkProvider sdkProvider)
{
this.sdkProvider = sdkProvider;
}

public bool TreatsWarningsAsErrors => true;

public IEnumerable<ValidationError> Validate(ValidationParameters validationParameters)
{
foreach (var benchmark in validationParameters.Benchmarks)
{
if (benchmark == null || benchmark.Job == null || benchmark.Job.Environment?.Runtime?.RuntimeMoniker == null)
{
continue;
}

var runtimeMoniker = benchmark.Job.Environment.Runtime.RuntimeMoniker;
if (!IsSdkInstalled(runtimeMoniker))
{
yield return new ValidationError(
TreatsWarningsAsErrors,
$"The required SDK for {runtimeMoniker} is not installed",
benchmark);
}
}
}

private bool IsSdkInstalled(RuntimeMoniker runtimeMoniker)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably be updated to check the CustomDotNetCliPath.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch. The path info is passed into DotNetSdkProvider but I forgot to update my method there. I will revise it.

{
string requiredSdkVersion = GetSdkVersionFromMoniker(runtimeMoniker);
var installedSdks = sdkProvider.GetInstalledSdks();

return installedSdks.Any(sdk => sdk.StartsWith(requiredSdkVersion + ".") || sdk == requiredSdkVersion);
}

private string GetSdkVersionFromMoniker(RuntimeMoniker runtimeMoniker)
{
switch (runtimeMoniker)
{
case RuntimeMoniker.Net461:
return "4.6.1";

case RuntimeMoniker.Net462:
return "4.6.2";

case RuntimeMoniker.Net47:
return "4.7";

case RuntimeMoniker.Net471:
return "4.7.1";

case RuntimeMoniker.Net472:
return "4.7.2";

case RuntimeMoniker.Net48:
return "4.8";

case RuntimeMoniker.Net481:
return "4.8.1";

case RuntimeMoniker.NetCoreApp20:
return "2.0";

case RuntimeMoniker.NetCoreApp21:
return "2.1";

case RuntimeMoniker.NetCoreApp22:
return "2.2";

case RuntimeMoniker.NetCoreApp30:
return "3.0";

case RuntimeMoniker.NetCoreApp31:
return "3.1";

case RuntimeMoniker.Net50:
return "5.0";

case RuntimeMoniker.Net60:
return "6.0";

case RuntimeMoniker.Net70:
return "7.0";

case RuntimeMoniker.Net80:
return "8.0";

case RuntimeMoniker.Net90:
return "9.0";

default:
throw new NotImplementedException($"SDK version check not implemented for {runtimeMoniker}");
}
}
}
}
34 changes: 34 additions & 0 deletions tests/BenchmarkDotNet.Tests/Validators/ExecutionValidatorTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
using System.Linq;
using System.Threading.Tasks;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Jobs;
using BenchmarkDotNet.Running;
using BenchmarkDotNet.Validators;
using JetBrains.Annotations;
Expand Down Expand Up @@ -563,5 +564,38 @@ public async ValueTask<int> GlobalCleanup()
[Benchmark]
public void NonThrowing() { }
}

[Fact]
public void MissingSdksAreDiscovered()
{
var fakeSdkProvider = new FakeSdkProvider(new string[0]); // No SDKs are installed.

var validator = new SdkValidator(fakeSdkProvider);

var validationErrors = validator.Validate(BenchmarkConverter.TypeToBenchmarks(typeof(BenchmarkWithNet461))).ToList();

Assert.NotEmpty(validationErrors);
Assert.StartsWith("The required SDK for Net461 is not installed", validationErrors.Single().Message);
}

[Fact]
public void InstalledSdksAreDetected()
{
var fakeSdkProvider = new FakeSdkProvider(new[] { "4.6.1" });

var validator = new SdkValidator(fakeSdkProvider);

var validationErrors = validator.Validate(BenchmarkConverter.TypeToBenchmarks(typeof(BenchmarkWithNet461))).ToList();

Assert.Empty(validationErrors);
}

[DryJob(RuntimeMoniker.Net461)]
public class BenchmarkWithNet461
{
[Benchmark]
public void Method()
{ }
}
}
}
20 changes: 20 additions & 0 deletions tests/BenchmarkDotNet.Tests/Validators/FakeSdkProvider.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
using BenchmarkDotNet.Validators;
using System.Collections.Generic;

namespace BenchmarkDotNet.Tests.Validators
{
public class FakeSdkProvider : ISdkProvider
{
private readonly string[] installedSdks;

public FakeSdkProvider(string[] installedSdks)
{
this.installedSdks = installedSdks;
}

public IEnumerable<string> GetInstalledSdks()
{
return installedSdks;
}
}
}