Skip to content

Conversation

@romanb
Copy link
Contributor

@romanb romanb commented Jun 30, 2020

Unfortunately this is a release of almost all crates, because of the combination of changes to libp2p-core/libp2p-swarm and the current restrictive upper bounds in all dependent crates. At least half of the releases just lift upper bounds for protocols and transport dependencies on core/swarm. To avoid this in the future, I hereby removed the upper bounds in the dependencies on core/swarm. I think as long as we pursue the multiple-crates approach this is beneficial. Of course, it will eventually result in older versions of crates trying to build against incompatible newer versions of their dependencies due to the lack of upper bounds, so this may at times be a bit less user-friendly and in a sense "encourages" users to upgrade more often. However, I think it will often be the case that minor releases of core/swarm don't require changes to all protocols and/or transports, as seen right here, in which case releasing crates just to bump upper bounds seems quite inconvenient. We can of course always selectively re-introduce upper bounds in specific situations, e.g. if there are large changes in core/swarm and we don't want to adapt all the downstream crates at once.

@romanb romanb requested a review from twittner June 30, 2020 20:31
Copy link
Contributor

@twittner twittner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[...] I hereby removed the upper bounds in the dependencies on core/swarm.

Does this not toss the whole concept of semantic versioning out of the window?

@romanb romanb mentioned this pull request Jul 1, 2020
@romanb
Copy link
Contributor Author

romanb commented Jul 1, 2020

Does this not toss the whole concept of semantic versioning out of the window?

Not entirely. There are lower bounds and this is only done for dependencies between libp2p crates and dependencies on core/swarm in particular. I am in general in favor of either lax (e.g. on major versions) or no upper bounds in library dependencies, primarily due to the "Bump upper bounds" releases required by any maintainer just to signal compatibility with a newer release of a dependency, which very often doesn't require any changes. This is a trade-off for maintainer-vs-user convenience. In the concrete case of libp2p-core/libp2p-swarm, there are so many ways in which these libraries can have a new minor release without requiring changes to the transports, protocols or both. I wouldn't mind putting in upper bounds on the major version, if that is more acceptable. Would you still have concerns with this approach?

@twittner
Copy link
Contributor

twittner commented Jul 1, 2020

I wouldn't mind putting in upper bounds on the major version, if that is more acceptable.

With versions >= 1.0.0 this would be equivalent to what we have now (minor and patch increments need to be backwards compatible with versions >= 1.0.0). It is only because we are < 1.0.0 that minor increments are considered incompatible, so if we ever get to 1.0.0 would you then want to remove the major upper bound? If not, we might as well keep the current scheme.

What concerns do you have with this approach?

It is incorrect w.r.t. semantic versioning. A user may depend on libp2p-core version X and libp2p-mplex version Y where libp2p-mplex expresses a dependency to libp2p-core >= X. The user later upgrades to libp2p-core version Z which is not compatible with X and the build breaks because libp2p-mplex no longer compiles. This puts users who rightly rely on semantic versioning at a disadvantage.

@tomaka
Copy link
Member

tomaka commented Jul 1, 2020

Keep in mind that if a user has both, say, libp2p-core v0.19 and libp2p-core v0.20 in their Cargo.lock, they might get cryptic error messages because all the traits and types defined in libp2p-core will be defined twice and their definition will be incompatible.

@romanb
Copy link
Contributor Author

romanb commented Jul 1, 2020

[..] minor and patch increments need to be backwards compatible with versions >= 1.0.0). It is only because we are < 1.0.0 that minor increments are considered incompatible [..]

I wasn't aware of the 0.x vs 1.x distinction in how minor versions are handled.

[..] so if we ever get to 1.0.0 would you then want to remove the major upper bound?

No, but as long as we are at 0.x I find it too cumbersome for ourselves to have upper bounds on the minor version between libp2p-crates, at least if we continue to increment the minor version for new, backwards-compatible additions. And as far as I can tell there are actually no hard rules to follow for 0.x.x versions w.r.t. the semantic versioning specification. So I would suggest to either establish only upper bounds on the major version as long as we are in 0.x.x or to just only increase the patch version even if new functionality is added (i.e. where we would normally increment the minor version post-1.0). Would you go along with either of these (and which do you prefer)?

@twittner
Copy link
Contributor

twittner commented Jul 1, 2020

And as far as I can tell there are actually no hard rules to follow for 0.x.x versions w.r.t. the semantic versioning specification.

According to semver.org there can be no compatible increments in versions 0.x.y. Cargo deviates from this in that it treats increments of y as compatible if x > 0.

So I would suggest to either establish only upper bounds on the major version as long as we are in 0.x.x or to just only crease the patch version even if new functionality is added (i.e. where we would normally increment the minor version post-1.0). Would you go along with either of these (and which do you prefer)?

I do not mind incrementing only the patch number if the changes are backwards compatible and I was under the impression that we already do it this way. However in this release, libp2p-core did change in an incompatible way, so the minor increment is correct. Naturally this requires increments in crates that depend on it, but if their APIs did not change, corresponding patch increments would suffice of course.

Finally, I think we are long overdue publishing version 1.0.0.

@romanb
Copy link
Contributor Author

romanb commented Jul 1, 2020

I do not mind incrementing only the patch number if the changes are backwards compatible and I was under the impression that we already do it this way.

It may have just been me being out of the loop on that.

However in this release, libp2p-core did change in an incompatible way, so the minor increment is correct.

Yes, of course, but I think the libp2p-swarm changes in turn are backward-compatible. Unfortunately, most protocols depend on both libp2p-swarm and libp2p-core. It would actually be nice to have them only depend on libp2p-swarm for better decoupling which would have made almost all the protocol crate releases unnecessary. It may similarly be beneficial if from libp2p-core the transport and muxing APIs were versioned independently - then a backward-incompatible change to either will only affect either the downstream muxing or transport crates, instead of always affecting both. But all of that is for another day.

@romanb romanb requested a review from twittner July 1, 2020 12:13
@romanb romanb merged commit e9952ea into libp2p:master Jul 1, 2020
@romanb romanb deleted the release branch July 1, 2020 13:36
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

Successfully merging this pull request may close these issues.

3 participants