Azure / azure-sdk-for-net

This repository is for active development of the Azure SDK for .NET. For consumers of the SDK we recommend visiting our public developer docs at https://learn.microsoft.com/dotnet/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-net.
MIT License
5.26k stars 4.6k forks source link

[QUERY] What is the .NET SDK approach to SemVer? #33816

Open jovinson-ms opened 1 year ago

jovinson-ms commented 1 year ago

Library name and version

All

Query/Question

Summary: I expect .NET SDK libraries to follow SemVer, but some don't and I'd like to understand why that is and whether it will change in the upcoming new libraries.

Top-level Guidelines

From the overall Azure SDK guidelines, it's pretty clear that SemVer is the north star for package versioning across all supported languages, with the caveat that the SDKs in various languages may follow different conventions.

The .NET-specific guidelines on that page don't explicitly address whether .NET packages use SemVer, but do strongly imply that they do, both by the example of a stable release incrementing after release goes from 1.1.0 to 1.2.0-beta.1, incrementing the minor version as expected, as well as by not calling out any divergence from the top-level guidance.

.NET-specific Guidelines

Unfortunately, the .NET-specific guidelines add some complication by addressing breaking changes in clients in this way:

DO introduce a new package (with new assembly names, new namespace names, and new type names) if you must do an API breaking change.

Breaking changes should happen rarely, if ever. Register your intent to do a breaking change with adparch. You’ll need to have a discussion with the language architect before approval.

These guidelines do imply SevVer:

DO use MAJOR.MINOR.PATCH format for the version of the library dll and the NuGet package.

Some Libraries Don't Follow Semver

As an example, the Azure.Identity library is a new client library that should follow the latest guidelines. However, a recent breaking change was published along with a minor version bump. When this was flagged as an issue, the response was that Azure.Identity doesn't follow SemVer.

Should consumers of the .NET Azure SDK libraries following the latest guidelines expect SemVer? It seems that there should be a unified approach for the new libraries, and SemVer did seem to be expected with the new libraries. I'd love to see all .NET SDK libraries follow SemVer, but at the least, it seems that the documentation should be updated to indicate that not all .NET libraries do.

Environment

No response

jsquire commented 1 year ago

//cc: @schaabs, @christothes

jsquire commented 1 year ago

Thank you for your feedback. Tagging and routing to the team member best able to assist.

jovinson-ms commented 1 year ago

@SeanFeldman

christothes commented 1 year ago

Library name and version

All

.NET-specific Guidelines

Unfortunately, the .NET-specific guidelines add some complication by addressing breaking changes in clients in this way:

DO introduce a new package (with new assembly names, new namespace names, and new type names) if you must do an API breaking change. Breaking changes should happen rarely, if ever. Register your intent to do a breaking change with adparch. You’ll need to have a discussion with the language architect before approval.

Hi @jovinson-ms - The Guidelines quoted above essentially state that we will avoid breaking changes if at all possible, but there are some rare exceptions (usually security related). We do document breaking changes explicitly in changlelogs, which seems more discoverable than stating in the guidelines that this may occur without a fully new assembly and type names.

ghost commented 1 year ago

Hi @jovinson-ms. Thank you for opening this issue and giving us the opportunity to assist. We believe that this has been addressed. If you feel that further discussion is needed, please add a comment with the text “/unresolve” to remove the “issue-addressed” label and continue the conversation.

KrzysztofCwalina commented 1 year ago

One of the primary objectives for the Azure SDK libraries is to be idiomatic to their language ecosystems, and so language-specific guideline override language-independent guidelines. Because of that, Azure SDK .NET libraries follow .NET platform's versioning guidelines, which are roughly: a) no API breaking changes, b) avoid almost at all cost behavioral breaking changes, c) use version numbers simply as a way to communicate the size/impact of changes, as opposed to associate some semantics with the versions.

The .NET platform (and so the .NET Azure SDK) does not use semver the same way as other platforms and languages (e.g. JavaScript). The reason is that CLR cannot load multiple versions of a simple library into the same context, and so the version numbers don't have semantic meaning (use different load strategy depending on major-minor versions) besides "newer library has more APIs"

Having said that, I agree with the point that we should have used the major version bump as a way to communicate (see point c above) to users that this release contains changes (behavioral breaking change) that require a bit more attention when upgrading. We will try to be more careful with this in the future.

SeanFeldman commented 1 year ago

Thank you for the elaborate description, @KrzysztofCwalina. Personally, I'd rather see Azure SDKs following SemVer than provide a false MAJOR.MINOR.PATCH version on the packages.

I would like to comment on point a. While it's plausible, it's never guaranteed that it can always be achieved. And rather than cornering yourself with no API-breaking changes at all costs, a new major version is a well-established industry indicator of potentially breaking changes.

Looking at SemVer 2 and Azure SDK guidelines, those are not conflicting in a way to avoid following more stricter versioning policies.

Given a version number MAJOR.MINOR.PATCH, increment the:

MAJOR version when you make incompatible API changes MINOR version when you add functionality in a backwards compatible manner PATCH version when you make backwards compatible bug fixes

When you write

Having said that, I agree with the point that we should have used the major version bump as a way to communicate (see point c above) to users that this release contains changes (behavioral breaking change) that require a bit more attention when upgrading. We will try to be more careful with this in the future.

You must remember those breaking changes do not require "a bit more attention". They require rigorous testing and verification. And that's precisely what's SemVer's objective is. As a consumer of the libraries that follow SemVer, remove surprises and have clear expectations about the changes a new version introduces.

jovinson-ms commented 1 year ago

Really appreciate the replies, @KrzysztofCwalina and @christothes. I think we may be looking at different angles of the versioning issue: I'm focusing on how the breaking change is communicated to the consumer, not on how the build system resolves dependencies.

First off, I appreciate the point that .NET's issues with dependency resolution are not resolved simply by using SemVer - any time breaking changes are introduced, there's the possibility of successfully compiling code that breaks under certain circumstances. As you point out, the current .NET Azure SDK guidelines avoid this problem by saying breaking changes have to be in a new namespace (with no exception mentioned for security patches). This approach makes sense for the runtime, but:

  1. it's quite limiting, and has issues with discoverability when changes are needed
  2. doesn't match the guidance for versioning libraries which suggests using SemVer
  3. most importantly, I don't think it's being followed by the .NET Azure SDKs - API breaking changes are regularly introduced with a minor version bump

As @christothes explains above, the approach that libraries are taking is to communicate a breaking change via minor version bump (in this case, minor) for stable version, while prerelease versions are free to change. So the reality as a consumer of these packages is that every time I upgrade one of the packages, there's a possibility that my code breaks, or that behavior has changed. The risk of encountering a dependency issue is still there whether the bumped version is major or minor. It sounds like communicating this via a major version bump is on the table, which would align with SemVer. That would be great!

As a side point, I read the guidelines to say that a breaking change requires a new namespace, which requires adparch approval, not that a breaking change can be communicated via version bump as long as there's adparch approval. If I'm reading it wrong, I suspect others are as well. If the actual practice is that breaking changes are introduced via minor version bumps, (but only with approval), then the guidelines should say that in my opinion.

wsugarman commented 1 year ago

+ 1 for semantic versioning in these libraries. Especially on a website where many repositories are having their NuGet packages automatically updated via Dependabot, communicating breaking changes via the version number is incredibly important; it gives consumers a signal that they need to carefully evaluate the change while automated tests may miss breaking behavioral changes.

Furthermore, most language ecosystems encourage semantic versioning for their packages (or equivalent concept) including NPM, Maven (albeit for SemVer 1.0), Go, pip, and Composer. I think it makes sense for these packages to comply too.

ghost commented 1 year ago

Hi @jovinson-ms, since you haven’t asked that we “/unresolve” the issue, we’ll close this out. If you believe further discussion is needed, please add a comment “/unresolve” to reopen the issue.

jovinson-ms commented 1 year ago

/unresolve

jovinson-ms commented 1 year ago

@KrzysztofCwalina it seems clear from the discussion that the Azure .NET SDK client libraries are not following SemVer, and the .NET versioning section is at the least ambiguous - what's the process of updating the Azure SDK guidelines for .NET to clarify the approach outlined by @christothes?

KrzysztofCwalina commented 1 year ago

@jovinson-ms, could you summarize which parts are ambiguous and should be updated?

I think one ambiguous part is that the guidelines don't make a distinction between API breaking changes and behavioral breaking changes. The note about introducing a new namespace, for example, is strictly for situations where we would have to break APIs otherwise. I do think this would be worth clarifying. Anything else?

jovinson-ms commented 1 year ago

Sure, as a consumer here's the changes I would propose:

Release Policies

Package Versioning

This section begins:

The team makes every effort to follow SemVer for versioning.

and concludes in a way that suggests .NET follows SemVer:

While all the languages follow the general versioning scheme, they each have slight differences based on their ecosystem, for those differences see the individual language sections below.

Suggested change:

Not all of the languages follow this general versioning scheme, see the individual language sections below for specifics.

.NET

Suggested change: add a link to the .NET-specific versioning guidelines

.NET Guidelines

Versioning

Client Versions

This section says:

DO introduce a new package (with new assembly names, new namespace names, and new type names) if you must do an API breaking change. Breaking changes should happen rarely, if ever. Register your intent to do a breaking change with adparch. You’ll need to have a discussion with the language architect before approval.

Suggested change:

DO add a note to the changelog if you must do an API breaking change. This should only happen after an approval from adparch.

Package Version Numbers

Suggested addition:

DO increment the major or minor version when adding a breaking change to the public API.

KrzysztofCwalina commented 1 year ago

DO add a note to the changelog if you must do an API breaking change. This should only happen after an approval from adparch.

It might be a terminology issue, but we have never done a binary API breaking change and we don't intend to do them. So, I don't think this is a guideline we want to have. Here is what I think would be a good clarification:

Breaking changes can be categorized roughly as: behavioral changes and API breaking changes. API breaking changes can be further grouped into source API breaking changes and binary API breaking changes. Another pivot in this taxonomy is backwards compatibility breaks (breaks when moving from older to newer package) and forward compatibility breaks (when moving from newer to older package), but this distinction is not super relevant to our discussion.

It's obvious what behavioral changes mean. As to binary and source API changes, it might be good to define them: a) source API breaking change is a change where source code compiled using one version of a package does not successfully compile when compiled using a different version of the same package. Binary API breaking change is when a binary calling one version of a package cannot call/bind an API in a different version of the same package.

.NET Azure SDK approach to each of the categories is as follows (and matches very closely what the .NET platform itself is doing):

  1. We have not ever done, nor are planning to do binary API breaking changes. These are extremely disruptive to users of our APIs given how CLR loader and NuGet work, and are relatively easy to avoid. These change can affect programs without developer recompiling the program, and so giving the compiler a chance to alert the developer that there is an issue. Also, even if the developer is aware of the issue, they might not have a way to mitigate such breaks. We of course reserve the right to do such change at some point (e.g. if it's the only way to fix a severe security issue), but in reality we have never needed that.
  2. Source API breaking changes. These are annoying, but are easily detected (in almost all but very corner cases) during component update. These are also unavoidable given the C# type system. For example, adding an overload can result in a source API breaking change. Also, there is a trafeoff here between the degree of the annoyance when moving to a new version of a package and the annoyance that outdated APIs (without proper overloads) can cause. And so here we try to minimize such changes to those that "pay for themselves", i.e. yield net benefit to our user bases. Such changes should be described in our release notes including code sample showing how to fix the calling code in the light of these changes. Keep in mind that these adjustments are always trivial for source breaking changes. If a change requires large source changes, it will always end up being a binary breaking change too, which we don't do.
  3. Behavioral changes. These are super tricky. As you might have noticed, I used the word "breaking" when describing API changes, but not when describing behavioral changes. The reason is that all behavioral changes can break something. Of course, some behavioral changes are more likely to break apps than others, but it does not change the fact that there is a spectrum here. The way we approach these changes is that we assess the likelihood of such changes affecting real apps and if we think the likelihood is more than trivial, we consider these are "non trivial", in which case we often reject such changes. If we cannot reject them, we provide compat switches and document them diligently. For the most severe cases, we intend to increment the major version number (and yes, we have made some mistakes here in the past for which I apologize). But because the effect behavioral changes have on apps is so dependent on the callers code, it's very difficult to have an approach (e.g. to version numbers change) that fits all.
jovinson-ms commented 1 year ago

Thanks for talking through this - really appreciate the clarification. With that in mind I'd suggest:

Release Policies

Package Versioning

This section ends:

While all the languages follow the general versioning scheme, they each have slight differences based on their ecosystem, for those differences see the individual language sections below.

Suggested change:

Some languages interpret breaking changes more granularly than SemVer, see the individual language sections below for specifics.

.NET

Suggested change: add a link to the .NET-specific versioning guidelines

.NET Guidelines

Versioning

Suggested change: clarify behavioral, binary and source breaking changes, with links to relevant documentation (OSS guidance)

Client Versions

This section says:

DO introduce a new package (with new assembly names, new namespace names, and new type names) if you must do an API breaking change. Breaking changes should happen rarely, if ever. Register your intent to do a breaking change with adparch. You’ll need to have a discussion with the language architect before approval.

Suggested change:

DO introduce a new package (with new assembly names, new namespace names, and new type names) if you must do a binary API breaking change. DO increment the minor version and add a note to the changelog if you must do a source API breaking change. DO engage adparch if you must do a behavioral breaking change. This may require a major version bump. All breaking changes should happen only after an approval from adparch.

jovinson-ms commented 1 year ago

@KrzysztofCwalina thoughts?