-
Notifications
You must be signed in to change notification settings - Fork 198
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
Upgrade to net9 #11535
base: main
Are you sure you want to change the base?
Upgrade to net9 #11535
Conversation
I'm guessing this comment is not an issue any more? https://github.com/dotnet/razor/pull/11000/files#r1796869549 |
Hmm I'll take a look. From vs code side everything should be fine. I don't know why this would be a problem in vs but I'll kick off integration tests and a test insertion |
I know nothing about nothing, I was just mainly interested in how many more changes there are in @jjonescz's PR, but it is relatively old, so I wouldn't be surprised if it was working around things that now no longer need to be worked around. |
It probably would have been okay to upgrade but if we let Roslyn do it first then we avoid some work ;) We're roll forward so we're technically running net9 even if we target net8. The extension handles downloading the runtime as needed |
src/Razor/benchmarks/Microsoft.AspNetCore.Razor.Microbenchmarks/Resources.cs
Outdated
Show resolved
Hide resolved
Directory.Build.props
Outdated
<NetVS>net8.0</NetVS> | ||
<NetVSCode>net9.0</NetVSCode> | ||
<NetVSAndVSCode>$(NetVS);$(NetVSCode)</NetVSAndVSCode> |
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.
Consider copying the approach taken in https://github.com/dotnet/roslyn/blob/b6ed7a5f13a3f00e7edf8cc4fab0d4524251f056/eng/targets/TargetFrameworks.props#L14-L22 even more. I.e., instead of setting these in the <Otherwise>
block, declare and set them in the main <PropertyGroup>
above and like the <Choose>
just override them. That way, they're easy to update.
Also, consider renaming DefaultNetFxTargetFramework
, since you're unifying the naming convention.
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.
Consider copying the approach taken in dotnet/roslyn@b6ed7a5/eng/targets/TargetFrameworks.props#L14-L22 even more. I.e., instead of setting these in the block, declare and set them in the main above and like the just override them. That way, they're easy to update.
SGTM :)
Also, consider renaming DefaultNetFxTargetFramework, since you're unifying the naming convention.
To NetFxVS
?
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.
To
NetFxVS
?
Perfect!
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.
Looking good! I think your simplification/unification of TFMs can still be improved a bit more, but I'll definitely drop a checkmark. ✅
@@ -1,7 +1,7 @@ | |||
<Project Sdk="Microsoft.NET.Sdk"> | |||
|
|||
<PropertyGroup> | |||
<TargetFrameworks>$(DefaultNetCoreTargetFrameworks);netstandard2.0;$(DefaultNetFxTargetFramework)</TargetFrameworks> | |||
<TargetFrameworks>$(NetVSAndVSCode);netstandard2.0;$(DefaultNetFxTargetFramework)</TargetFrameworks> |
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.
This is outside of the scope of your PR, but I wonder if Razor tooling still needs to compile anything with netstandard2.0
? I believe we only ship net8.0
binaries in service hub.
This will be needed for the new EA so putting in a separate PR