-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Update to net9.0 #75465
Update to net9.0 #75465
Conversation
Does this also fix #75205 (are there any mentions of |
No, there don't seem to be any. I will mark the issue as fixed by this PR, thanks. |
@@ -52,7 +52,7 @@ | |||
<MicrosoftExtensionsOptionsConfigurationExtensionVersion>8.0.0</MicrosoftExtensionsOptionsConfigurationExtensionVersion> | |||
<MicrosoftExtensionsOptionsVersion>8.0.0</MicrosoftExtensionsOptionsVersion> | |||
<MicrosoftExtensionsPrimitivesVersion>8.0.0</MicrosoftExtensionsPrimitivesVersion> | |||
<MicrosoftNetCompilersToolsetVersion>4.11.0-2.24270.4</MicrosoftNetCompilersToolsetVersion> | |||
<MicrosoftNetCompilersToolsetVersion>4.12.0-3.24473.3</MicrosoftNetCompilersToolsetVersion> |
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.
Curious: what if we just delete this and move to the compiler that comes with the .NET SDK.
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.
I can try that. But generally we sometimes want to consume new compiler features before we update our SDK, right? So then we would need to re-add this again.
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.
Although I updated this to bring in the "better conversion from collection expression" feature that's needed because of some string.Join ambiguities - and that might not be even in RC1, right? Well, definitely it's not in preview 7 which we are currently at.
@@ -110,3 +110,22 @@ When the .NET SDK RTMs and Roslyn adopts it all occurrences of `$(NetRoslynNext) | |||
|
|||
**DO NOT** include both `$(NetRoslyn)` and `$(NetRoslynNext)` in the same project unless there is a very specific reason that both tests are adding value. The most common case is that the runtime has changed behavior and we simply need to update our baselines to match it. Adding extra TFMs for this just increases test time for very little gain. | |||
|
|||
## Checklist for updating TFMs (once a year) |
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.
One other item to check is that you don't regress the number of tests that are running in CI. Our logic for finding tests can include looking at TFM. In the past I messed this up and stopped running certain unit test assemblies.
@@ -40,10 +40,6 @@ internal static bool IsCoreClr8OrHigherRuntime | |||
internal static bool IsCoreClr9OrHigherRuntime | |||
=> CoreClrRuntimeVersion is { } v && v >= 9; | |||
|
|||
#if NET9_0_OR_GREATER | |||
#error Make the above check be an #if NET9_OR_GREATER when we add net8 support to build | |||
#endif |
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.
I'm not sure what this was trying to say.
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.
I'm guessing it was important at some point when I was converting locally but not by the time I merged the change.
@@ -4,7 +4,7 @@ | |||
<PropertyGroup> | |||
<OutputType>Library</OutputType> | |||
<RootNamespace>Microsoft.CodeAnalysis.MSBuild.UnitTests</RootNamespace> | |||
<TargetFrameworks>$(NetRoslyn);net472</TargetFrameworks> | |||
<TargetFrameworks>$(NetVS);net472</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.
I wouldn't expect it to be necessary to hold this back. Was there a particular issue when moving this to 9.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.
We run the unit tests on a machine with VS installed, so that might matter somehow if maybe the regular SDK isn't there. If that somehow broke, do file a bug @jjonescz and we can investigate further.
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.
We run the unit tests on a machine with VS installed, so that might matter somehow if maybe the regular SDK isn't there
Yes, that's exactly what happens. Created an issue: #75498
@@ -38,12 +38,14 @@ static Test() | |||
// reasonable TLS protocol version for outgoing connections. | |||
#pragma warning disable CA5364 // Do Not Use Deprecated Security Protocols | |||
#pragma warning disable CS0618 // Type or member is obsolete | |||
#pragma warning disable SYSLIB0014 // 'ServicePointManager' is obsolete |
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.
Is this obsoleted in a way that means this isn't working anymore? Is this just stale now?
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.
(I have no idea what this is, but might be worth filing a bug as a follow up to track what this is.)
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.
WebRequest, WebClient, and ServicePoint are obsolete since .NET 6. See https://learn.microsoft.com/en-us/dotnet/core/compatibility/networking/6.0/webrequest-deprecated. The SYSLIB0014 is a pre-existing diagnostic that started to be reported for ServicePointManager in .NET 9 via dotnet/runtime#103456.
Filed an issue: https://github.com/dotnet/roslyn/issues/75499
@@ -4,7 +4,7 @@ | |||
<PropertyGroup> | |||
<OutputType>Library</OutputType> | |||
<RootNamespace>Microsoft.CodeAnalysis.MSBuild.UnitTests</RootNamespace> | |||
<TargetFrameworks>$(NetRoslyn);net472</TargetFrameworks> | |||
<TargetFrameworks>$(NetVS);net472</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.
We run the unit tests on a machine with VS installed, so that might matter somehow if maybe the regular SDK isn't there. If that somehow broke, do file a bug @jjonescz and we can investigate further.
net7.0
target #75205.