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

Make MaxSupportedLangVersion calculation dynamic #75795

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

kasperk81
Copy link

Uses historical trend to calculate language version based on the .NET version, removing hardcoded mappings. Ensures future .NET versions automatically align with expected C# version.

@kasperk81 kasperk81 requested a review from a team as a code owner November 7, 2024 00:10
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 7, 2024
@dotnet-policy-service dotnet-policy-service bot added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Nov 7, 2024
@kasperk81
Copy link
Author

see dotnet/templating#6781 (comment), every year we need to update this file when it's time to update the templates. for .net 10: dotnet/sdk#44349

@ViktorHofer @jcouv, how do you feel about this type of scalable approach?

Copy link
Member

@jaredpar jaredpar left a comment

Choose a reason for hiding this comment

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

Pretty sure this will break the moment people start targeting net10.0

Uses historical trend to calculate language version based on the
.NET version, removing hardcoded mappings. Ensures
future .NET versions automatically align with expected C# version.
@jaredpar
Copy link
Member

jaredpar commented Nov 7, 2024

@jcouv want to make sure you've seen this. MSBuild logic looks good but you often do the lang version updates so want to make sure you see the new setup.

'$(_MaxSupportedLangVersion)' == ''">$([MSBuild]::Add(9, $([MSBuild]::Subtract($(_TargetFrameworkVersionWithoutV), 5)))).0</_MaxSupportedLangVersion>

<!-- Cap _MaxSupportedLangVersion if it exceeds _MaxAvailableLangVersion -->
<_MaxAvailableLangVersion>13.0</_MaxAvailableLangVersion>
Copy link
Author

Choose a reason for hiding this comment

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

@jcouv When a new language version is released, we'll just need to update _MaxAvailableLangVersion here and make adjustments in TargetTests.cs (updating the net10.0 line and adding a net11.0 line to match net10.0's language version). This setup means we won’t need to modify anything else for new framework versions released during this period each year.

Copy link
Member

Choose a reason for hiding this comment

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

If we still need to update this file every year when we increase the max language version, then what do we actually save by making this change over the current system?

Copy link
Author

Choose a reason for hiding this comment

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

The current system requires two updates each year: one when a new framework version is released (around October/November, managed by dotnet/sdk) and another when a new language version is added, which is handled in this repo. With this change, we're eliminating the need for the first update, as the framework version alignment will now happen automatically.

Copy link
Member

Choose a reason for hiding this comment

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

I see. Thanks for clarifying for me. I'll let Julien be the second sign-off here though, since he does the roslyn-side of the process usually.

Copy link
Member

Choose a reason for hiding this comment

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

one [update] when a new framework version is released (around October/November, managed by dotnet/sdk)

@kasperk81 Would you mind sharing a link to one of those previous dotnet/sdk changes? That'll help provide more context

Copy link
Author

@kasperk81 kasperk81 Nov 21, 2024

Choose a reason for hiding this comment

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

@jcouv it happens in the sdk each year when templates are updated. e.g. since dotnet/sdk#44349, if we use the daily build (net10.0) and do something like:

$ dotnet new record -n record1

it issues warnings:

Warning: Failed to evaluate bind symbol 'evaluatedLangVersion', it will be skipped.
Warning: Failed to evaluate bind symbol 'evaluatedLangVersion', it will be skipped.
The template "Record" was created successfully.

these type of warnings will go away when 14.0 will be introduced around july 2025 but then they will reappear around october 2025 until july 2026. pr is basically decoupling this sdkVersion + 1 to compilerLatestVersion binding so things keep working smoothly throughout the year.

@jaredpar jaredpar added this to the 17.12 milestone Nov 11, 2024
@jaredpar
Copy link
Member

@jcouv for second sign off

@jaredpar jaredpar modified the milestones: 17.12, 17.13 Nov 11, 2024
@arunchndr arunchndr modified the milestones: 17.13, 17.13 P2 Nov 13, 2024
@jcouv
Copy link
Member

jcouv commented Nov 21, 2024

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

jcouv
jcouv previously approved these changes Nov 21, 2024
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 4)

@jcouv jcouv dismissed their stale review November 21, 2024 19:26

Additional comment

'$(_MaxSupportedLangVersion)' == ''">$([MSBuild]::Add(9, $([MSBuild]::Subtract($(_TargetFrameworkVersionWithoutV), 5)))).0</_MaxSupportedLangVersion>

<!-- Cap _MaxSupportedLangVersion if it exceeds _MaxAvailableLangVersion -->
<_MaxAvailableLangVersion>13.0</_MaxAvailableLangVersion>
Copy link
Member

Choose a reason for hiding this comment

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

The comment on the C# test named LanguageVersionAdded_Canary should be updated to say _MaxAvailableLangVersion instead of MaxSupportedLangVersion. Those are the instructions we follow when adding a new language version.

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Done with review pass (iteration 4)

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 5)

@jcouv jcouv enabled auto-merge (squash) November 21, 2024 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants