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
change the runtimes API #1277
change the runtimes API #1277
Conversation
docs/_changelog/header/v0.11.6.md
Outdated
[TargetFrameworkJob(TargetFrameworkMoniker.Mono)] | ||
[TargetFrameworkJob(TargetFrameworkMoniker.NetCoreApp21)] | ||
[TargetFrameworkJob(TargetFrameworkMoniker.NetCoreApp30)] | ||
[SimpleJob(runtimeMoniker: RuntimeMoniker.Net48)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't like the runtimeMoniker
prefix. Could we introduce a few additional SimpleJobAttribute
ctor overloads with the RuntimeMoniker
as the first ctor argument?
[Obsolete("Please use SimpleJobAttribute instead.", false)] | ||
public class ClrJobAttribute : JobConfigBaseAttribute | ||
{ | ||
public ClrJobAttribute() : base(Job.Default.With(ClrRuntime.GetCurrentVersion())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
namespace BenchmarkDotNet.Attributes | ||
{ | ||
[Obsolete("Please use DryJobAttribute instead. Use the ctor that requires RuntimeMoniker argument.", false)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of "Use the ctor that requires RuntimeMoniker argument", it will be better to write a code example that can be copy-pasted.
@adamsitnik in general, it looks very good, thanks! I left just a few minor (but important) comments. |
@AndreyAkinshin thanks for the review! I've pushed the fixes. Please let me know if there is any blocker for 0.11.6 from my side |
@AndreyAkinshin this fixes the things we have discussed yesterday at Dotnetos
When it comes to:
I made it a warning as we talked but I've not introduced the concept of
InvalidConfig
.What I did instead is to call the
GetCurrentVersion
method for every runtime.If the
host runtime == job runtime
, then it's going to work fine (99% of the cases). If the runtimes are different: for example running as Full .NET Framework and trying to use[CoreJob]
attribute, thenGetCurrentVersion
is going to throw:BenchmarkDotNet/src/BenchmarkDotNet/Environments/Runtimes/CoreRuntime.cs
Line 44 in 2ba3033