Adding guidance on what is considered a breaking change in the .NET SDK #45288
Conversation
ghost
added
baronfel
force-pushed
the
doc-change-guidance
branch
from
ee0019b to
f67cb20
Compare
There was a problem hiding this comment.
PR Overview
This PR introduces a draft guidelines document for managing breaking changes and diagnostics in the .NET SDK and updates the README to include a reference to the new document.
- Introduces a comprehensive draft outlining procedures, types of changes, and available configuration knobs.
- Updates the documentation index to include a link to the new guidelines.
Reviewed Changes
| File | Description |
|---|---|
| documentation/project-docs/breaking-change-guidelines.md | Added a draft document outlining breaking change guidelines for the SDK. |
| documentation/README.md | Updated to include a link to the new breaking change guidelines. |
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
|
@baronfel, @marcpopMSFT - I have pushed a commit with updates to the original draft from @baronfel . Would you please take a look? Who else should review? |
There was a problem hiding this comment.
Nice to see breaking changes procedures called out!
One thing I'm unclear on is who is the audience for this? Overall, the wording feels very strong and not welcoming for public contributions. It feels like an instruction manual for the PM team or similar. Is that the goal?
|
|
||
| * introducing new changes in a staged/gradual way | ||
| * trying to tie opinionated analyzers/diagnostics to a mechanism that requires explicit user opt-in | ||
| * providing a way to opt out of a change entirely | ||
|
|
There was a problem hiding this comment.
nit: The other bulleted lists begin with a capital letter. I prefer capital for lists and would suggest doing that here.
| ### Cut-over to new behavior after a long introduction period | ||
|
|
||
| After the change has been introduced in a gradual way, cut over to the new behavior. This may mean removing the old behavior entirely, or it may mean making the new behavior the default and providing a way to opt out of it. It is important that you | ||
| provide enough time for users to adapt - for example the `dotnet new --list` example above took an entire major release to make the new forms the default. |
There was a problem hiding this comment.
Suggest a newline for each start of sentence to make future updates cleaner.
There was a problem hiding this comment.
I'm following the convention of other markdown in the repo.
There was a problem hiding this comment.
Git diffs on markdown work better with my suggestion. Up to you whether that's important.
| Have some kind of knob that allows users to opt out of the change entirely. This could be a flag, an environment variable, or a global.json setting. This allows users to continue using the old behavior if they need to in exceptional situations. It is | ||
| important to document this knob and its behavior in the SDK documentation. It is also important to define a timeline for when this knob will be removed entirely, forcing users to adopt the new behavior. | ||
|
|
||
| For systems like Analyzers that time may be 'never', because the cost of detection is so low. This is a product-level decision that is hard to give universal guidance for. |
There was a problem hiding this comment.
nit: Maybe italics instead of a code callout for never
|
|
||
| Changes that are expected to cause significant disruption should only be introduced behind the Target Framework knob. This eliminates business continuity and allows developers to address changes needed as part of scheduled work to migrate a codebase to a new TFM. | ||
|
|
||
| Concrete example: NuGet warnings for vulnerable transitive dependencies were introduced only for applications targeting .NET 10 and higher. |
There was a problem hiding this comment.
IMO the terminology "concrete" is a bit strong and unnecessary.
There was a problem hiding this comment.
I will change to "specific"
| ## Other recommendations | ||
|
|
||
| * If possible, introduce significant breaking changes in a non-LTS release. | ||
| * Publish blog posts and update public documentation as appropriate as early as possible. |
There was a problem hiding this comment.
Who is the audience for this document? Who are we asking to publish a blog? I would imagine this is an internal PM responsibility, but it reads to me like we are asking anyone contributing to the SDK to publish a blog.
|
|
||
| ## Configuration Knobs | ||
|
|
||
| The following knobs are available to enable/disable these changes (some may not apply to all kinds of changes): |
There was a problem hiding this comment.
Change waves and LangVer also apply to the tooling ecosystem.
There was a problem hiding this comment.
LangVer - I read https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/configure-language-version, and it looks like it's the same as TFM for .NET Core. So, I'm wondering in which cases langver would be used instead of TFM.
Change waves - I found a description here: https://learn.microsoft.com/en-us/visualstudio/msbuild/change-waves?view=vs-2022. Moving forward, do we recommend people using change waves or using the new SdkAnalysisLevel property?
There was a problem hiding this comment.
LangVersion is often the same as the numeric part of TFM, but that's purely a convenience. Users often set it explicitly when
- multitargeting and wanting to ensure a consistent level of analysis across TFMs
- when they want to use
previewlanguage features
There was a problem hiding this comment.
MSBuild Change Waves are necessary still for changes to MSBuild engine behavior, especially changes that occur before project evaluation takes place (e.g. globbing). MSBuild engine changes remain a place where we do not have great user-facing controls. MSBuild logic (props/targets files) are great for SdkAnalysisLevel, WarningLevel, etc knobs that we already have.
| In general, we want to make updating the .NET SDK as smooth as possible for developers. This means: | ||
|
|
||
| * introducing new changes in a staged/gradual way | ||
| * trying to tie opinionated analyzers/diagnostics to a mechanism that requires explicit user opt-in |
There was a problem hiding this comment.
not sure what opinionated means in this context. Maybe just remove or are there specific types of diagnostics that you would include and some you wouldn't.
| * Implementation changes for MSBuild Tasks | ||
| * NuGet Restore algorithm enhancements | ||
| * Changes to DotNet CLI grammar | ||
| * Changes to defaults in CLI flags that impact behavior |
There was a problem hiding this comment.
changes to CLI behavior itself. For example, we're considering changing dotnet clean to clean everything, not just your tfm/rid config which is what it does today.
| ### Implement changes in an informational/non-blocking way initially | ||
|
|
||
| What this means will vary change-to-change. For example, for a change expressed as an Analyzer or MSBuild diagnostic, consider | ||
| Informational level severities initially. For a behavioral change on a CLI, consider an informational message written to the |
There was a problem hiding this comment.
theoretically, this is also breaking but we have to draw a line somewhere and I think if some script is reading and depending on the stderr output, that seems like a reasonable line to draw.
There was a problem hiding this comment.
@rainersigwald - Based on your experience, should changes int the output be considered breaking?
There was a problem hiding this comment.
Changes in output can certainly be breaking. MSBuild has had to demote some high-importance info messages (e.g. dotnet/msbuild#9228) because including location and/or a code made them look "too much" like warnings for enough users.
|
|
||
| If a change in introduced in an informational/non-blocking way, determine the time frame where it is safe to increase the severity. For Analyzers, this may mean tying it to the next value of AnalysisLevel (which is downstream of TFM). For small MSBuild and NuGet diagnostics, this may mean tying it to the next Warning Level or SdkAnalysisLevel. For CLI changes, this may mean tying it to the next LTS major version of the SDK. Ideally the way you would structure this increase would be automated and documented so that users know what's coming down the pipe. | ||
|
|
||
| ### Cut-over to new behavior after a long introduction period |
There was a problem hiding this comment.
FYI, for CLI grammar, our model has been one release with both the new and old, one release with a message when using the old, and then remove in the third release. I don't know if we've done a good job of always having a release where they are both available first and I'm also not always in a rush to remove things later. An example would be dotnet package add versus dotnet add package.
I don't know if you want to specifically call out this as gradually increase over major release fwiw.
There was a problem hiding this comment.
I called this out separately now.
|
|
||
| ### Always provide a way to opt out of the change | ||
|
|
||
| Have some kind of knob that allows users to opt out of the change entirely. This could be a flag, an environment variable, or a global.json setting. This allows users to continue using the old behavior if they need to in exceptional situations. It is |
There was a problem hiding this comment.
I think we want to avoid global.json settings. That should be used in limited scenarios and not for flags like this. I also think that env variables are not preferred if you can help it as you have to set them before you run (though they are required for some types of changes like msbuild enginge changes). By flag, you should call out project property (unless you meant something else).
| ### Always provide a way to opt out of the change | ||
|
|
||
| Have some kind of knob that allows users to opt out of the change entirely. This could be a flag, an environment variable, or a global.json setting. This allows users to continue using the old behavior if they need to in exceptional situations. It is | ||
| important to document this knob and its behavior in the SDK documentation. It is also important to define a timeline for when this knob will be removed entirely, forcing users to adopt the new behavior. |
There was a problem hiding this comment.
fwiw, I might never remove some knobs if they are simple properties or env variables. I don't think Roslyn removes langver support but we should check with them.
There was a problem hiding this comment.
ahh, you mention that may be never for analyzers below. I might include items that have a simple on/off.
There was a problem hiding this comment.
I removed the break line.
|
|
||
| ### Tie potentially impactful changes to the TFM targeted by the application/library. | ||
|
|
||
| Changes that are expected to cause significant disruption should only be introduced behind the Target Framework knob. This eliminates business continuity and allows developers to address changes needed as part of scheduled work to migrate a codebase to a new TFM. |
There was a problem hiding this comment.
I might explicitly state that this is preferred where possible.
There was a problem hiding this comment.
The current wording is a bit stronger, and I think that's something good. Thoughts?
|
|
||
| ## Other recommendations | ||
|
|
||
| * If possible, introduce significant breaking changes in a non-LTS release. |
There was a problem hiding this comment.
fwiw, .NET leadership does not consider there to be a separate bar for LTS versus STS when it comes to things like this. Not sure if you included this coming from VS guidance.
|
|
||
| Concrete example: NuGet warnings for vulnerable transitive dependencies were introduced only for applications targeting .NET 10 and higher. | ||
|
|
||
| ## Other recommendations |
There was a problem hiding this comment.
Do we want to mention the breaking change label, tactics, or any of the internal aliases?
There was a problem hiding this comment.
This is public documentation, but the breaking-change label is relevant. I'll add that.
aortiz-msft
changed the title
| ### Cut-over to new behavior after a long introduction period | ||
|
|
||
| After the change has been introduced in a gradual way, cut over to the new behavior. This may mean removing the old behavior entirely, or it may mean making the new behavior the default and providing a way to opt out of it. It is important that you | ||
| provide enough time for users to adapt - for example the `dotnet new --list` example above took an entire major release to make the new forms the default. |
There was a problem hiding this comment.
Git diffs on markdown work better with my suggestion. Up to you whether that's important.
| ## Required process for all .NET SDK breaking changes | ||
|
|
||
| * Create an issue in the appropriate GitHub repository to track the change, if one does not already exist. | ||
| * Add the breaking-change label to the issue. This label should be available in all .NET repositories that ship as part of the .NET SDK. If the label is not available, please file an issue in [dotnet/sdk](https://github.com/dotnet/sdk). |
There was a problem hiding this comment.
| * Add the breaking-change label to the issue. This label should be available in all .NET repositories that ship as part of the .NET SDK. If the label is not available, please file an issue in [dotnet/sdk](https://github.com/dotnet/sdk). | |
| * Add the breaking-change label to the issue. This label should be available in all .NET repositories that ship as part of the .NET SDK. If the label is not available, please file an issue in [dotnet/sdk](https://github.com/dotnet/sdk/issues). |
Adding guidance for breaking changes in the .NET SDK