-
Notifications
You must be signed in to change notification settings - Fork 469
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 global.json rollforward #1541
Conversation
Allow a newer SDK to be used if 8.0.1xx isn't found. VS preview updates will uninstall 8.0.1xx and install 8.0.2xx-preview. We should still work with that. This uses the same global.json policy as dotnet/runtime.
@@ -1,7 +1,7 @@ | |||
{ | |||
"sdk": { | |||
"version": "8.0.100", | |||
"rollForward": "latestPatch", | |||
"rollForward": "major", |
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 don't think we want to open it up all the way to major, right? I believe the one we really want is feature
, which will cause it to allow the latest 8.0.Xxx SDK found but not a 9.0 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.
Why not? In theory, a 9.0 SDK shouldn't have any breaking changes and should work just fine. If someone just had a 9.0 SDK installed and opened our VS .sln, why prevent them from trying to work? If we know it doesn't work, that is one thing. But the SDK says that it should work, so allowing it seems just fine (IMO). If any 8.0 SDK is found, it will use that first. It will only use 9.0 if that is the only thing available.
Note that this is the strategy used in https://github.com/dotnet/runtime/blob/main/global.json#L5
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.
It likely works now because 9.0 is new, but it is very common for newer compilers and analyzers to add new warnings and errors which will cause the build to fail, and can confuse dogfooders trying to build our repo for the first time. These sorts of breaks are much less likely when within the same major version band, so my suggestion was in order to make the build more stable/predictable.
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.
Those new errors/warnings shouldn't be introduced on an updated SDK. They only get introduced when you change your TFM. If you are targeting the same TFM, you should get the same experience.
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.
They only get introduced when you change your TFM. If you are targeting the same TFM, you should get the same experience.
Yeah that's definitely not always the case. Compiler folks quite often add new analyzers by default including those with warning level diagnostics (it happened in 8.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.
@jaredpar I'd love to hear your thoughts here on what the advise 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.
https://github.com/dotnet/roslyn/blob/d845c1aea354affde0fcdb045be9dc74f86dcb52/global.json#L5
"version": "8.0.100",
"allowPrerelease": false,
"rollForward": "disable"
that might give you a hint. 😄 But that policy is way too restrictive for dotnet/aspire. Updating to a new preview VS version should still work. We do it all the time.
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.
Yes, that's why I was suggesting sticking with latest feature band, but not crossing the major boundary
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.
Hey @baronfel, we were chatting about this today at Tactics and @marcpopMSFT mentioned that you are currently tracking the work to ensure that the breaking changes from upgrading to new (major) SDK versions are minimal, so I'm wondering what are your thoughts/suggestions on whether or not we should rollforward to newer major version through global.json in both this repo, and all dotnet repos too.
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.
Sure - the design for this is dotnet/designs#308. The basic idea is that we want users to float forward, but also provide a way to signal 'ok but please act as if the SDK is some previous SDK version'. That signal is a new property that's set at a specific time in the build. From there each component would need to opt-in to actually adhering to this value, but the end goal is to allow folks to float more easily while retaining pinning for short-term workaround scenarios. The first use case is Roslyn's breaking backing-field language change that we hope to release in .NET 9 preview 1 and expand coverage of from there.
The upshot of this is - I think eventually we should be able to encourage more rapid rolling of SDK versions, but I think we'll need to have more of the SDK-delivered tools jump on the SdkAnalysisLevel bandwagon before it's practically safe enough.
Allow a newer SDK to be used if 8.0.1xx isn't found. VS preview updates will uninstall 8.0.1xx and install 8.0.2xx-preview. We should still work with that.
This uses the same global.json policy as dotnet/runtime.
Microsoft Reviewers: Open in CodeFlow