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

Amend RFC 1105 to specify how dependency versions relate to semver #1890

Closed
wants to merge 1 commit into from

Conversation

sgrif
Copy link
Contributor

@sgrif sgrif commented Feb 8, 2017

@Ixrec
Copy link
Contributor

Ixrec commented Feb 8, 2017

it should be noted that it is impossible to update a dependency which contains a breaking change that affects your library, without making a breaking change yourself.

This sentence is supposed to say "public dependency", right?

@aturon aturon added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Feb 9, 2017
@aturon
Copy link
Member

aturon commented Feb 10, 2017

@sgrif: I have a question about the notion of "superset" that determines whether you need to issue a major bump or not.

In particular, I'm wondering whether upgrading a dependency from ^1.0.0 to ^1.0.5 is acceptable. The latter requirement is definitely not a superset of the former. But I am quite worried about such dependency upgrades causing major bumps, even if limited to public dependencies.

If we're thinking specifically about the breakage such a dependency update could cause, I believe the main issue is that other crates in the graph could have an =1.0.0 dependency, so that moving to ^1.0.5 would create a conflict. Is that what you were thinking?

As you're probably aware, there's long been an idea floating around to explicitly distinguish between public and private dependencies in Cargo.toml, together with tooling support (e.g. to check whether these declarations are correct, and such that any for any public dependencies that can come into contact, only one global version is allowed). If we moved to such a scheme, you could then imagine saying:

  • Public dependencies may only use ^-style constraints.
  • Duplication for different minor versions of a crate is allowed in the case that one is used as a private dependency (this way private deps can continue to use = constraints).

Does that make sense?

cc @wycats @alexcrichton

@sgrif
Copy link
Contributor Author

sgrif commented Feb 10, 2017

In particular, I'm wondering whether upgrading a dependency from ^1.0.0 to ^1.0.5 is acceptable. The latter requirement is definitely not a superset of the former.

No, I don't think so. There should (in theory) be no reason to need to make such a change though, is there?

If we're thinking specifically about the breakage such a dependency update could cause, I believe the main issue is that other crates in the graph could have an =1.0.0 dependency, so that moving to ^1.0.5 would create a conflict. Is that what you were thinking?

Pretty much, yeah. I could definitely see the argument behind considering patch version bumps a minor change, but at the same time there's not really any reason in practice to specify a minimum patch version.

Public dependencies may only use ^-style constraints.

This removes the ability to write >= 1.0.0, < 3.0.0 if the breaking changes in 2.0.0 didn't affect you and you are able to effectively support both. (As a side note which is out of scope for this RFC, it would be nice to be able to conditionally provide code to support two versions of a library)

@nox
Copy link
Contributor

nox commented Feb 11, 2017

No, I don't think so. There should (in theory) be no reason to need to make such a change though, is there?

While going from 1.0.0 to 1.0.5 sounds weird indeed, there is a very legit need to sometimes bump a dependency from 1.0.0 to 1.1.0 and that seems forbidden as a minor bump in your new scheme of things. This doesn't sound like a good thing. How does anyone make us of new APIs in minor bumps of dependencies if that can only be a major bump in your own crate?

@aturon
Copy link
Member

aturon commented Feb 12, 2017

@sgrif

Two clarifications:

  • I was using a patch-level update because that example was used in the RFC text -- but also, requiring a new patch-level version is perfectly reasonable if you need a bug-fixed version of the library for your code to work correctly.

-As @nox says, a minor bump of a public dependency is a very important use-case, and forcing a major bump in these cases would be a pretty heavy restriction.

To elaborate a bit more: the way that a minor/patch-level bump can cause breakage is if downstream code also publicly depends on the same version of the bumped crate but explicitly disallows the minor bump.

To formalize this, we can say that a version constraint is "semver open" if, for every version X.Y.Z allowed by the constraint, versions X.(Y+1).0 and X.Y.(Z+1) are allowed as well. In other words, any time you can successfully resolve the graph with one version, you can do so with any minor/patch-level bump as well.

Some thoughts about these constraints:

  • Semver-open constraints are (and should be) the norm. After all, if people are correctly versioning their crates, minor/patch-level bumps should remain compatible.

  • The constraint >= 1.0.0, < 3.0.0 is semver-open, so I don't think there's a problem there.

  • I'm basically proposing that crates.io not let you publish public dependencies unless you have semver-open constraints on them. The rationale is that public dependencies are where "dependency hell" arises, and ensuring that everyone allows minor bumps around them should make the entire ecosystem more robust -- and means that a minor bump of a public dependency doesn't force a major bump of your crate, since it can't break anyone.

Does that make sense to you?

@nox
Copy link
Contributor

nox commented Feb 12, 2017

The constraint >= 1.0.0, < 3.0.0 is semver-open, so I don't think there's a problem there.

With current Cargo, going from 1.0.0 to >=1.0.0, <3.0.0 (lol, I was wondering why you put a space after <, I guess it's because it became ❤️ on your system too) is IMO a breaking change, given Cargo will just use the highest version for all crates using that dependency, making some use 1.0.0 and some others use 2.0.0, and then compile errors arrive. Fixing the issue is just a cargo update -p ... --precise 1.0.0 away, but Cargo doesn't say anything about that, let alone do it itself.

@aturon
Copy link
Member

aturon commented Feb 12, 2017

@nox I think you misunderstood what I was getting at.

Let me be more concrete. Suppose we have three crates:

  • Crate A, which has latest verisons 1.1.0 and 2.5.0
  • Crate B, which publicly depends on A with constraint ^2.0.0
  • Crate C, which publicly depends on C with constraint >= 1.1.0, < 3.0.0

My point is that if crate B revises its constraint to ^2.1.0, that won't break the dependency graph, because it can still be resolved with crate C's constraints.

However, if crate C had said >= 1.1.0, <= 2.0.0, then crate B's revision would break the graph (assuming that the public dependencies need to all coalesce).

The difference is that the >= 1.1.0, < 3.0.0 is "semver-open" because for any version where you can successfully resolve the graph, you can also resolve it with a minor/patch bump. Whereas with >= 1.1.0, <= 2.0.0 that's not true: 2.0.0 is permitted, but 2.1.0 is not.

With that in hand, the proposal is that if we require all public dependency constraints to have this form -- to be "semver-open" -- then a minor/patch bump can't break the dependency graph (and should not break the ability to compile), and hence should be permitted without forcing a major bump in crate B's version.

@sgrif
Copy link
Contributor Author

sgrif commented Feb 12, 2017

I'm in favor of restricting public dependencies to semver open with or without Cargo enforcement. I will update the PR when I have time.

@nox
Copy link
Contributor

nox commented Feb 12, 2017

Oh right.

But what about the case I mentioned though?

  • Crate A, which has latest versions 1.1.0 and 2.5.0
  • Crate B, which publicly depends on A with constraint 1.0.0
  • Crate C, which publicly depends on A with constraint 1.0.0
  • Crate D, which publicly depends on B and C

If crate C revises its constraint to >=1.0.0, <3.0.0 and publishes that as a minor bump, and the user runs cargo update -p C from D, there can be compile errors because B and C don't use the same A anymore.

Should that be considered as a Cargo bug, or as something that this RFC should limit?

@aturon
Copy link
Member

aturon commented Feb 12, 2017

@nox I wouldn't consider that a Cargo bug per se. However, part of the idea of "public dependency" that's been floating around is that you'd explicitly say which of your dependencies are public, and which are private (and we could check that you're correct about this). Then Cargo would refuse to resolve a graph with multiple versions of a shared, public dependency. I'm not sure precisely what this would mean for cargo update, but at worst you get an informative error from Cargo itself (about why it can't resolve the graph) rather than a random compiler error.

All that said -- the full details of public vs private dependencies as an explicit notion in Cargo merits its own RFC, and that's something we should push toward in the near future. I'd expect that RFC to give a clearer answer to your question.

@alexcrichton
Copy link
Member

Thanks for the RFC @sgrif! I think you, @aturon, and @nox have already articulated my primary concern with this RFC where I would consider a change from ^1.0.0 to ^1.1.0 as not a breaking change.

Some other comments from the RFC I've got are:

An example of this would be changing a version range from 2.0.0 to >= 2.0.0, < 3.0.0. This change would still break downstream code, as cargo update would always resolve the transitive dependency to the highest supported version

I don't quite follow this, right now 2.0.0 and >= 2.0.0, < 3.0.0 are the same thing? (the first is "semver compatible with 2.0.0"). Could you elaborate on what the breakage would be here?

Major change: making a public dependency optional

It may be worth specifying if it's possible to make a public dependency optional but still enabled by default. While this seems like it may be backwards compatible it may not always be.


@nox the case you mentioned is accurately described by @aturon. It's not really a bug today but if Cargo had a distinction between public/private it'd definitely be a bug.

@sgrif
Copy link
Contributor Author

sgrif commented Feb 13, 2017

Could you elaborate on what the breakage would be here?

Sorry, I originally wrote that as 0.2.0 and 0.3.0, and eventually shifted it over. I should have changed that case to >= 2.0.0, < 4.0.0

@alexcrichton
Copy link
Member

Ah makes sense!

@Ericson2314
Copy link
Contributor

I really want to emphasize that we need to Cargo to a place where no dependency change is a breaking change. Otherwise, we end up in a situation where breaking changes always necessitate downstream to also make a breaking change if the new version is to be used, even if nothing downstream actually uses any part of the interface that broke. I consider this from a ecosystem/software-engineering perspective absolutely untenable.

Unique version choices (made workable via public/private deps) will solve this, so I eagerly await that RFC.

@sfackler sfackler self-assigned this Feb 13, 2017
As an extension of this, it should be noted that it is impossible to update a
dependency which contains a breaking change that affects your library, without
making a breaking change yourself. As such, all libraries should follow Rust's
example by committing to stability. Libraries which are stable (version is
Copy link
Member

Choose a reason for hiding this comment

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

A version greater than 1.0.0 doesn't mean the library is stable. You can still bump the major version as often as you want.

considers it to be so.

As an extension of this, it should be noted that it is impossible to update a
dependency which contains a breaking change that affects your library, without
Copy link
Member

Choose a reason for hiding this comment

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

That's not correct. Consider if you only use items of a crate publicly that were not broken. Then your users won't notice anything.

@aturon
Copy link
Member

aturon commented Mar 29, 2017

The libs team discussed this RFC a bit in our triage meeting. The consensus was that this seems like the right direction (modulo making updates based on the comment thread so far), but that it may be best to hold off on landing this policy until explicit public dependencies have landed in Cargo. @mitsuhiko is working on an RFC for that feature, and we're trying to line up some implementation help as well.

On that basis, I'm going to go ahead and formally propose that we postpone, but feel free to push back!

@rfcbot fcp postpone

@rfcbot
Copy link
Collaborator

rfcbot commented Mar 29, 2017

Team member @aturon has proposed to postpone this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@sgrif
Copy link
Contributor Author

sgrif commented Mar 29, 2017

I'm in favor of postponing this if explicit public dependencies are in the near future.

@rfcbot
Copy link
Collaborator

rfcbot commented Apr 13, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Apr 13, 2017
@rfcbot
Copy link
Collaborator

rfcbot commented Apr 23, 2017

The final comment period is now complete.

@alexcrichton alexcrichton added the postponed RFCs that have been postponed and may be revisited at a later time. label Apr 24, 2017
@alexcrichton
Copy link
Member

Closing now that FCP has elapsed (as postponed)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. postponed RFCs that have been postponed and may be revisited at a later time. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants