-
- Notifications
You must be signed in to change notification settings - Fork 1k
Add .NET 10 support #2642
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
Add .NET 10 support #2642
Conversation
64dc9bb to da35b1c Compare da35b1c to 3357418 Compare | I've tested this PR against a local build from dotnet/runtime#106599 and it fails because it seems that the .NET 10 SDK doesn't actually allow to target .NET 10, rather it only allows targetting .NET 9. To get things to work we do need to make changes to the SDK validator to allow backwards compatibility. However, this change will still be useful to have in once we do have .NET 10 targeting supported |
| Yup, it's a chicken-egg situation atm, and we will soon be on alpha1 in runtime repo (based on how it was done last year, around this time). I think we can pass --corerun option in local builds that points to built corerun under |
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.
GitHub won't let me add a comment on the actual line... I think the TryParse method needs to be updated to account for the underscore.
internal static bool TryParse(string runtime, out RuntimeMoniker runtimeMoniker) { int index = runtime.IndexOf('-'); if (index >= 0) { runtime = runtime.Substring(0, index); } // Monikers older than Net 10 don't use any version delimiter, newer monikers use _ delimiter. if (Enum.TryParse(runtime.Replace(".", string.Empty), ignoreCase: true, out runtimeMoniker)) { return true; } return Enum.TryParse(runtime.Replace('.', '_'), ignoreCase: true, out runtimeMoniker); }I don't have a 10.0 sdk version, so I didn't test.
| [InlineData("net70")] | ||
| [InlineData("net80")] | ||
| [InlineData("net90")] | ||
| [InlineData("net10_0")] |
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.
| [InlineData("net70")] | |
| [InlineData("net80")] | |
| [InlineData("net90")] | |
| [InlineData("net10_0")] | |
| [InlineData("net7.0")] | |
| [InlineData("net8.0")] | |
| [InlineData("net9.0")] | |
| [InlineData("net10.0")] |
| IMHO the best direction would be to introduce something similar to the This transition to .NET 10.0 might be an ideal time to consider such a change and start deprecating the old enum, with the goal of fully removing it after a few release cycles. Additionally, pseudo-TFMs like |
| I don't understand how |
| I think now may be a good time to revisit this PR. Is there anything specifically blocking it or just another pass to clean up the suggested changes? We are hitting an issue for our performance wasm testing due to wasmnet100/wasmnet10_0. FYI @caaavik-msft |
| @timcassell is there anything else that needs to be done before merging this? It seems maybe the TryParse update, but wanted to check if there was anything else. |
| I think that's the only thing. And the test cases. |
| @LoopedBard3 Regarding wasm specifically, you could also send a PR to fix the wasm logic so you can use the simple |
adamsitnik left a comment
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.
LGTM. Since .NET Team needs it, I am going to merge it in the current form and cherry pick changes from https://github.com/am11/BenchmarkDotNet/pull/1/files.
Thank you @am11 and others involved!
src/BenchmarkDotNet.Diagnostics.dotMemory/DotMemoryDiagnoser.cs Outdated Show resolved Hide resolved
| "choices": [ | ||
| { | ||
| "choice": "net10.0", | ||
| "description": ".NET 10" |
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.
.NET 10 is not even in Preview 1 yet so we don't have to update the templates right now. But on the other hand if we don't do it now, we may simply forget about it. So let's merge it in the current form
This strongly-typed enums based implementation is not scaling very well; but for now, lets add 10.0 and soon after lets see if we can make it less verbose to ease up future updates a bit: dotnet/runtime#106599 (comment)
cc @adamsitnik, @lewing