Sdk Validation Fix #2511

MattFromRVA · 2024-01-24T14:39:32Z

src/BenchmarkDotNet/Running/BenchmarkRunnerClean.cs

MattFromRVA · 2024-02-09T21:18:59Z

@timcassell I am not sure why the linux and macos checks are failing. According to the result report all tests have passed. Am I missing something? https://github.com/dotnet/BenchmarkDotNet/pull/2511/checks?check_run_id=21320355764

timcassell · 2024-02-09T23:42:29Z

@timcassell I am not sure why the linux and macos checks are failing. According to the result report all tests have passed. Am I missing something? https://github.com/dotnet/BenchmarkDotNet/pull/2511/checks?check_run_id=21320355764

I ran into the same issue in some of my PRs. I opened an issue for it. #2520. You'll have to search the logs or run it locally to find the culprit.

MattFromRVA · 2024-02-10T01:42:22Z

@timcassell I am not sure why the linux and macos checks are failing. According to the result report all tests have passed. Am I missing something? https://github.com/dotnet/BenchmarkDotNet/pull/2511/checks?check_run_id=21320355764

I ran into the same issue in some of my PRs. I opened an issue for it. #2520. You'll have to search the logs or run it locally to find the culprit.

Ok sounds good. I will do some digging. @timcassell

timcassell · 2024-02-10T03:00:35Z

src/BenchmarkDotNet/Toolchains/CsProj/CsProjCoreToolchain.cs

@@ -26,8 +26,8 @@ public class CsProjCoreToolchain : Toolchain, IEquatable<CsProjCoreToolchain>
        [PublicAPI] public static readonly IToolchain NetCoreApp80 = From(NetCoreAppSettings.NetCoreApp80);
        [PublicAPI] public static readonly IToolchain NetCoreApp90 = From(NetCoreAppSettings.NetCoreApp90);

-        internal CsProjCoreToolchain(string name, IGenerator generator, IBuilder builder, IExecutor executor, string customDotNetCliPath)
-            : base(name, generator, builder, executor)
+        internal CsProjCoreToolchain(string name, IGenerator generator, IBuilder builder, IExecutor executor, string customDotNetCliPath, ISdkProvider sdkProvider)


Why don't we merge the customDotNetCliPath into the ISdkProvider? They seem very related, and the validation likely would change depending on the custom path.

Why don't we merge the customDotNetCliPath into the ISdkProvider? They seem very related, and the validation likely would change depending on the custom path.

Will do. Should I do the same thing with customDotNetCliPath in NativeAotToolchain as well?

Yes, please. All the toolchains that use it.

…hat use it. Will need to update some ConfigParserTests

timcassell · 2024-02-11T03:10:06Z

src/BenchmarkDotNet/Toolchains/CsProj/CsProjCoreToolchain.cs

-            : base(name, generator, builder, executor)
+        private readonly ISdkProvider sdkProvider;
+
+        public ISdkProvider SdkProvider => sdkProvider;


Suggested change

public ISdkProvider SdkProvider => sdkProvider;

internal ISdkProvider SdkProvider => sdkProvider;

timcassell · 2024-02-11T03:11:30Z

src/BenchmarkDotNet/Toolchains/Mono/MonoAotToolchain.cs

@@ -15,7 +15,7 @@ public class MonoAotToolchain : Toolchain
        public static readonly IToolchain Instance = new MonoAotToolchain();

        [PublicAPI]
-        public MonoAotToolchain() : base("MonoAot", new Generator(), new MonoAotBuilder(), new Executor())
+        public MonoAotToolchain() : base("MonoAot", new Generator(), new MonoAotBuilder(), new Executor(), null)


If not every toolchain needs an ISdkProvider, why is it part of the base toolchain?

I forgot about this one. I am not familiar with Mono and just put null for the time being while I did more research. From what I found it is possible that Mono can use the .NET SDK since .NET 5.

I will work on its implementation.

Please do that in a separate PR if you're going to do it. But it's the same thing for in process toolchains and Roslyn toolchain.

ah ok. I just committed some more changes but I will update my code.

timcassell · 2024-02-11T03:15:58Z

src/BenchmarkDotNet/Toolchains/MonoWasm/WasmToolchain.cs

@@ -13,8 +13,8 @@ public class WasmToolchain : Toolchain
    {
        private string CustomDotNetCliPath { get; }

-        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, string customDotNetCliPath, ISdkProvider sdkProvider)


Suggested change

private WasmToolchain(string name, IGenerator generator, IBuilder builder, IExecutor executor, string customDotNetCliPath, ISdkProvider sdkProvider)

private WasmToolchain(string name, IGenerator generator, IBuilder builder, IExecutor executor, ISdkProvider sdkProvider)

timcassell · 2024-02-11T03:16:19Z

src/BenchmarkDotNet/Toolchains/NativeAot/NativeAotToolchain.cs

+        private readonly ISdkProvider sdkProvider;
+#pragma warning restore CS0649
+
+        public ISdkProvider SdkProvider => sdkProvider;


Suggested change

public ISdkProvider SdkProvider => sdkProvider;

internal ISdkProvider SdkProvider => sdkProvider;

timcassell · 2024-02-11T03:19:03Z

src/BenchmarkDotNet/Validators/ISdkProvider.cs

+    public interface ISdkProvider
+    {
+        IEnumerable<string> GetInstalledSdks();
+        string CustomDotNetCliPath { get; set; }


Suggested change

string CustomDotNetCliPath { get; set; }

string CustomDotNetCliPath { get; }

timcassell · 2024-02-11T03:20:19Z

src/BenchmarkDotNet/Validators/SdkValidator.cs

+            }
+        }
+
+        private bool IsSdkInstalled(RuntimeMoniker runtimeMoniker)


This should probably be updated to check the CustomDotNetCliPath.

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

timcassell · 2024-02-11T03:21:58Z

tests/BenchmarkDotNet.Tests/Validators/FakeSdkProvider.cs

+            if (!string.IsNullOrEmpty(CustomDotNetCliPath))
+            {
+                return installedSdks;
+            }


I don't understand this check. Both branches return the same thing.

Oops. Me neither. 😄

timcassell · 2024-02-11T22:16:38Z

src/BenchmarkDotNet/Toolchains/MonoWasm/WasmToolchain.cs

@@ -11,12 +11,12 @@ namespace BenchmarkDotNet.Toolchains.MonoWasm
    [PublicAPI]
    public class WasmToolchain : Toolchain
    {
-        private string CustomDotNetCliPath { get; }
+        private readonly string CustomDotNetCliPath;


I don't think this field is needed.

timcassell · 2024-02-11T22:17:41Z

src/BenchmarkDotNet/Toolchains/MonoWasm/WasmToolchain.cs

-                    netCoreAppSettings.CustomDotNetCliPath,
                    new DotNetSdkProvider());


Need to pass the custom path to the provider.

timcassell · 2024-02-11T22:20:19Z

src/BenchmarkDotNet/Toolchains/Mono/MonoAotToolchain.cs

@@ -15,7 +15,7 @@ public class MonoAotToolchain : Toolchain
        public static readonly IToolchain Instance = new MonoAotToolchain();

        [PublicAPI]
-        public MonoAotToolchain() : base("MonoAot", new Generator(), new MonoAotBuilder(), new Executor(), null)
+        public MonoAotToolchain() : base("MonoAot", new Generator(), new MonoAotBuilder(), new Executor(), new DotNetSdkProvider())


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).

ah ok. Good info. I will definitely need a different approach in implementing this validation to just certain toolchains then.

timcassell · 2024-02-11T22:22:41Z

src/BenchmarkDotNet/Validators/DotNetSdkProvider.cs

+        public string CustomDotNetCliPath
+        {
+            get => _customDotNetCliPath;
+            set => _customDotNetCliPath = value;
+        }


Just make this a read only property and pass the string into the constructor (can make it an optional argument).

timcassell · 2024-02-11T22:28:43Z

src/BenchmarkDotNet/Validators/ISdkProvider.cs

+
+namespace BenchmarkDotNet.Validators
+{
+    public interface ISdkProvider


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?

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

That makes perfect sense. I appreciate the valuable insight.

MattFromRVA · 2024-02-14T14:34:52Z

@timcassell I am closing this PR and opening a new one. After doing some more reworking I found that the scope of this .Net validation is much smaller than I was originally assuming with Toolchains.

The new PR #2523 has a simpler approach instead of using inheritance which was messing up other Toolchains.

I appreciate all the feedback so far.

MattFromRVA and others added 4 commits January 23, 2024 15:24

sdk validation check

bb5980a

addking validator

c2f40c1

Made code more testable. Created unit tests

fc8c22c

refactor validation logic

0e426ea

timcassell reviewed Jan 28, 2024

View reviewed changes

src/BenchmarkDotNet/Running/BenchmarkRunnerClean.cs Outdated Show resolved Hide resolved

MattFromRVA added 4 commits January 29, 2024 10:20

Moving validation to toolchains. Still WIP

3217c28

updated some toolchains for validation

edeb125

expanded validator to include . Net Framework

61780c9

Code cleanup

5986aec

MattFromRVA marked this pull request as ready for review February 9, 2024 21:19

MattFromRVA requested a review from timcassell February 9, 2024 21:19

adjusted .Net Framework check

5f89f15

timcassell reviewed Feb 10, 2024

View reviewed changes

Merged customDotNetCliPath into the ISdkProvider for the Toolchains t…

3df3b0b

…hat use it. Will need to update some ConfigParserTests

MattFromRVA marked this pull request as draft February 11, 2024 02:56

timcassell requested changes Feb 11, 2024

View reviewed changes

Revised code. Still need to work on ConfigParserTests

da1dae4

timcassell reviewed Feb 11, 2024

View reviewed changes

MattFromRVA closed this Feb 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sdk Validation Fix #2511

Sdk Validation Fix #2511

MattFromRVA commented Jan 24, 2024

MattFromRVA commented Feb 9, 2024

timcassell commented Feb 9, 2024

MattFromRVA commented Feb 10, 2024

timcassell Feb 10, 2024

MattFromRVA Feb 10, 2024

timcassell Feb 10, 2024

timcassell Feb 11, 2024

timcassell Feb 11, 2024

MattFromRVA Feb 11, 2024

timcassell Feb 11, 2024

MattFromRVA Feb 11, 2024

timcassell Feb 11, 2024

timcassell Feb 11, 2024

timcassell Feb 11, 2024

timcassell Feb 11, 2024

MattFromRVA Feb 11, 2024

timcassell Feb 11, 2024 •

edited

MattFromRVA Feb 11, 2024

timcassell Feb 11, 2024

timcassell Feb 11, 2024

timcassell Feb 11, 2024

MattFromRVA Feb 12, 2024

timcassell Feb 11, 2024

timcassell Feb 11, 2024

timcassell Feb 11, 2024

MattFromRVA Feb 12, 2024

MattFromRVA commented Feb 14, 2024 •

edited

	public ISdkProvider SdkProvider => sdkProvider;
	internal ISdkProvider SdkProvider => sdkProvider;

	private WasmToolchain(string name, IGenerator generator, IBuilder builder, IExecutor executor, string customDotNetCliPath, ISdkProvider sdkProvider)
	private WasmToolchain(string name, IGenerator generator, IBuilder builder, IExecutor executor, ISdkProvider sdkProvider)

	string CustomDotNetCliPath { get; set; }
	string CustomDotNetCliPath { get; }

		netCoreAppSettings.CustomDotNetCliPath,
		new DotNetSdkProvider());

Sdk Validation Fix #2511

Sdk Validation Fix #2511

Conversation

MattFromRVA commented Jan 24, 2024

MattFromRVA commented Feb 9, 2024

timcassell commented Feb 9, 2024

MattFromRVA commented Feb 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timcassell Feb 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MattFromRVA commented Feb 14, 2024 • edited

timcassell Feb 11, 2024 •

edited

MattFromRVA commented Feb 14, 2024 •

edited