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.

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

Allow custom runtimes to use Executor #2285

Merged
merged 2 commits into from Mar 12, 2023
Merged

Conversation

tgjones
Copy link
Contributor

@tgjones tgjones commented Mar 10, 2023

Hello folks! This PR makes a small change to Executor to allow it be used for custom runtimes. The only downside I can see is that the current NotSupportedException is probably a useful reminder when adding built-in runtimes, to ensure the correct behaviour is added or re-used here.

My use-case: I've written a custom runtime, and currently I need to copy/paste all of Executor and its supporting classes into my own codebase, which seems a bit redundant when all I really want to change are these 3 lines. Of course this particular setup for ProcessStartInfo is not applicable for every custom runtime - but at least it allows Executor to be used for those runtimes for which this is applicable.

But of course you'll have a better view of whether this change is a good idea and / or acceptable.

@AndreyAkinshin
Copy link
Member

I believe it would be better to introduce a separate base class or interface for such a purpose. E.g., like this:

Index: src/BenchmarkDotNet/Toolchains/Executor.cs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/BenchmarkDotNet/Toolchains/Executor.cs b/src/BenchmarkDotNet/Toolchains/Executor.cs
--- a/src/BenchmarkDotNet/Toolchains/Executor.cs	(revision d4de82a3f0bcd71f1f695554eef343bbfc1357ea)
+++ b/src/BenchmarkDotNet/Toolchains/Executor.cs	(date 1678458305076)
@@ -148,6 +148,10 @@
                     start.Arguments = args;
                     start.WorkingDirectory = artifactsPaths.BinariesDirectoryPath;
                     break;
+                case CustomRuntime _:
+                    start.FileName = exePath;
+                    start.Arguments = args;
+                    break;
                 default:
                     throw new NotSupportedException("Runtime = " + runtime);
             }
Index: src/BenchmarkDotNet/Environments/Runtimes/CustomRuntime.cs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/BenchmarkDotNet/Environments/Runtimes/CustomRuntime.cs b/src/BenchmarkDotNet/Environments/Runtimes/CustomRuntime.cs
new file mode 100644
--- /dev/null	(date 1678458285120)
+++ b/src/BenchmarkDotNet/Environments/Runtimes/CustomRuntime.cs	(date 1678458285120)
@@ -0,0 +1,10 @@
+using BenchmarkDotNet.Jobs;
+
+namespace BenchmarkDotNet.Environments
+{
+    public abstract class CustomRuntime : Runtime
+    {
+        protected CustomRuntime(RuntimeMoniker runtimeMoniker, string msBuildMoniker, string displayName) :
+            base(runtimeMoniker, msBuildMoniker, displayName) { }
+    }
+}
\ No newline at end of file

@tgjones would it cover your use case?

@tgjones
Copy link
Contributor Author

tgjones commented Mar 10, 2023

Thanks! That's a better idea - and yes, it would cover my use-case. I'll modify my PR to look like that.

@tgjones
Copy link
Contributor Author

tgjones commented Mar 10, 2023

@AndreyAkinshin I've applied your suggestion.

@AndreyAkinshin AndreyAkinshin merged commit 565870f into dotnet:master Mar 12, 2023
@AndreyAkinshin
Copy link
Member

@tgjones thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants