Skip to content
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

v2.0.3 introduced breaking changes and is breaking our build #142

Closed
marten-seemann opened this issue Jul 14, 2023 · 9 comments
Closed

v2.0.3 introduced breaking changes and is breaking our build #142

marten-seemann opened this issue Jul 14, 2023 · 9 comments

Comments

@marten-seemann
Copy link

We're using golang-lru in go-libp2p, and it look like v2.0.3 removed NewARC. Removing an exported function is a breaking change and according to the semver rules, should have been released in a new major release, not in a patch release.

This is now breaking our build in go-libp2p when we (or worse, any of our users) update to the latest patch release.

Would it be possible to retract the v2.0.3 and the v2.0.4 release, and / or release a v2.0.5 that restores compatibility with v2.0.2 and previous?

@jefferai
Copy link
Member

Apologies, but we will be keeping as is. NewARC was not removed, simply moved to its own module. It is a build-breaking change but not an API-breaking change and as such is in accordance with semver rules which state that major versions are for API-breaking changes.

Simply update your includes and you'll be good to go.

@marten-seemann
Copy link
Author

There’s no such distinction between “build-breaking” and “API breaking” changes in semver. But if you want to make that distinction, it would go the other way: Every build-breaking change is necessarily an API-breaking change, but you can break the API without breaking the build (e.g. by fundamentally changing the behavior of the function).

Apologies, but we will be keeping as is.

Can you please reconsider? In Go (and semver in general), updating to a patch release is supposed to be a non-brainer. Guaranteed that no code changes will be required (that’s the very reason that go get -u=patch exists). Releases like these break this assumption, for the entire ecosystem that uses your package.

It should be easy enough to add an alias for NewARC that points to the new location.

@jefferai
Copy link
Member

There’s no such distinction between “build-breaking” and “API breaking” changes in semver.

Agree, but semver does explicitly talk about API changes, and explicitly does not talk about build changes.

We will not be adding an alias, because that would defeat the entire reason for moving ARC to its own package in the first place (not including patented code in the main package). Sorry for the trouble, but please simply update your includes.

@tvlieg
Copy link

tvlieg commented Aug 8, 2023

Hi! We ran into the very same issue, our build failed because your project is an indirect (!) dependency of ours. We did not expect our build to fail because of a patch (!) release of this project. If you would have made this a major upgrade, as is indeed the convention in Go, that would not have happened.

As was mentioned above, I also agree that build-breaking changes are also API breaking changes. You changed all exported types and functions from ARC to another package, so that changed the API.

@jefferai
Copy link
Member

Semver references building quite a lot, e.g. around build metadata in tags, but it never indicates that build breakages are covered by the spec, or that build breakages are equivalent to API breakages. Perhaps it should, but unfortunately the current state of the spec is that it leaves it up to interpretation as to whether or not someone considers a build change to be an API change or not, and we simply differ in interpretation.

I'm curious as to why it would fail as an indirect dependency; this suggests that your direct dependency was also failing but did not catch this situation.

@marten-seemann
Copy link
Author

I'm curious as to why it would fail as an indirect dependency; this suggests that your direct dependency was also failing but did not catch this situation.

It does because your direct dependency might still be on the old version, so the failure doesn't occur there yet.

Semver references building quite a lot, e.g. around build metadata in tags, but it never indicates that build breakages are covered by the spec, or that build breakages are equivalent to API breakages. Perhaps it should, but unfortunately the current state of the spec is that it leaves it up to interpretation as to whether or not someone considers a build change to be an API change or not, and we simply differ in interpretation.

I'm quite surprised about this interpretation of the semver spec. In my reading, it's pretty clear.
A patch release is only permitted for patches and non-breaking changes. If you break people's compilation, this is as clearly a breaking changing change as it could be (literally!). That's the reason that commands like go get -u=patch exist: according to semver, updating to a patch release is guaranteed to never break anything.

@Stebalien
Copy link

Moving functions without providing aliases is a semver-breaking change by any reasonable definition of a breaking change.

If you want to see what go considers a breaking change, try out the https://pkg.go.dev/golang.org/x/exp/cmd/gorelease tool.

@jefferai
Copy link
Member

That's the reason that commands like go get -u=patch exist: according to semver, updating to a patch release is guaranteed to never break anything.

Again, via API. There were no API changes, only build changes.

Moving functions without providing aliases is a semver-breaking change by any reasonable definition of a breaking change.

Providing aliases would defeat the entire point of having the patent code in a separate module. I realize that several of you consider this change to be obviously outside of allowed semver rules, but that's just one interpretation. Again, semver covers API changes, not build changes. I agree the spec could be clearer on that point, but the package API did not change, only the location moved.

@Stebalien
Copy link

No, the norms here are very clear. Patch releases should not break builds (except in very special cases like lints/deprecations, compiler upgrades, etc.). The entire point of semver is to prevent that kind of thing.

I suggest you ask anyone else you work with, they'll give you the same answer. This isn't subjective.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants