Remove the dotnet global tool #1572
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In #1006 (0.11.4) we have introduced a new dotnet global tool.
By looking at the number of reported bugs I believe that the tool has not passed the test of time.
Why is that? Because it's based entirely on dynamic assembly loading which is very hard to get right in .NET and the fact that we support all existing .NET Runtimes (.NET, .NET Core, Mono, CoreRT) makes it even harder (if not impossible).
First of all, the .NET Runtime version of the assembly with benchmarks and the tool should match which in reality means that we should target:
netcoreapp2.0netcoreapp2.1netcoreapp2.2netcoreapp3.0netcoreapp3.1net5.0If there is no match, the users get errors: #1347, #1249
As of today, the tool targets only
netcoreapp2.1:BenchmarkDotNet/src/BenchmarkDotNet.Tool/BenchmarkDotNet.Tool.csproj
Line 5 in cd0bda4
Because this is what our CI targets. We could add all the TFMs to our CI (like in #1254), but it would increase the time it takes to run our test suite (we used to get a lot of timeout in the past and I am sure that they would come back). Moreover, it would raise the bar for contributing to the project because the new contributors would have to install all the mentioned SDKs.
Second of all, despite having a custom assembly resolver, our users very often run into issues with dynamic assembly loading and missing dependencies: #1355, #1371, #1435, #1444, #1460
I am sure that we could extend the custom assembly resolver and solve some of this bugs, but in my opinion it would never be working in all possible scenarios (due to the complexity of dependency resolving and dynamic loading).
Last but not least, the tool does not solve the most important problem: the need of having the source code of the benchmarks and .NET SDK installed on the machine. It's using the same logic that
BenchmarkRunnerdoes: it generates a new project file and references the project with benchmarks and builds it. If we ever want to tackle this problem, I believe that we should rather go with an option that publishes a self-contained .NET app that can be copied to other machine and executed without having .NET SDK installed.I believe that we won't have time to fix all these issues and the design based on dynamic assembly loading would never be futureproof and the tool should just be removed.
Our users who started with the tool and then switched to old good
BenchmarkSwitcherconfirm this:#1460 (comment)
@AndreyAkinshin let's discuss it on Monday during our call. I know that you don't like removing things from BenchmarkDotNet, but I believe that this is the best solution.
Fixes #1254
Fixes #1347 Fixes #1249
Fixes #1355 Fixes #1371 Fixes #1435, Fixes #1444, Fixes #1460