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

Add .NET 10 support #2642

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add .NET 10 support #2642

wants to merge 3 commits into from

Conversation

am11
Copy link
Member

@am11 am11 commented Sep 22, 2024

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

@am11 am11 marked this pull request as ready for review September 22, 2024 16:49
@caaavik-msft
Copy link
Contributor

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

@am11
Copy link
Member Author

am11 commented Sep 23, 2024

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 artifacts/bin or artifacts/tests/coreclr/<platform>/Tests/Core_Root to get the ball rolling?

Copy link
Collaborator

@timcassell timcassell Oct 20, 2024

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.

Comment on lines 388 to +391
[InlineData("net70")]
[InlineData("net80")]
[InlineData("net90")]
[InlineData("net10_0")]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[InlineData("net70")]
[InlineData("net80")]
[InlineData("net90")]
[InlineData("net10_0")]
[InlineData("net7.0")]
[InlineData("net8.0")]
[InlineData("net9.0")]
[InlineData("net10.0")]

@am11
Copy link
Member Author

am11 commented Oct 20, 2024

IMHO the best direction would be to introduce something similar to the [SupportedOSPlatform], such as [SupportedFramework("netX.0")], which would provide indefinite scalability. The current hardcoded enum approach doesn't scale well, which was precisely why [SupportedPlatform("")] (introduced in .NET 5.0) was designed to be string-based instead of enum-based—managing an enum over the years would have been a maintenance nightmare.

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 nativeaot9.0 feel somewhat unnatural compared to the broader ecosystem’s conventions, such as using net9.0 combined with --aot (e.g., dotnet new console -f net9.0 --aot) or simply setting PublishAot=true.

@timcassell
Copy link
Collaborator

I don't understand how [SupportedFramework("netX.0")] would be used. I think you should open an issue with more details rather than discuss it here. And/or create a WIP PR showcasing a new system.

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.

3 participants