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

Allow multiple pre-release versions of a crate to co-exist #6019

Closed
wants to merge 1 commit into from

Conversation

Diggsey
Copy link
Contributor

@Diggsey Diggsey commented Sep 12, 2018

Fixes #6016

Does not change behaviour described in #2222

The semver crate currently differs from the spec in that it believes that a version 2.0.0 is compatible with a version requirement of ^2.0.0-alpha. The spec says that all pre-release versions are unstable, and therefore not necessarily compatible with any other versions. This is why the test needs to use exact version requirements to exhibit the behaviour where multiple pre-release versions are included.

I also renamed the resolving_allows_multiple_compatible_versions test as it is actually testing that incompatible versions can co-exist (this relationship should probably not be called is_compatible to begin with!)

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matklad (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@alexcrichton
Copy link
Member

Thanks for the PR! The implementation here looks reasonable to me (cc @Eh2406 on it as well). I'd be curious to see if others from @rust-lang/cargo have thoughts on this as well!

@Eh2406
Copy link
Contributor

Eh2406 commented Sep 14, 2018

As my opinion was requested...

This does look like the correct implementation. Though I am wondering if one of these days we should add in line test of compatible in addition to the unit tests of registry. There are a lot of ifs in a specific order.

Also compatible is the hottest part of the innermost loop of the resolver, so we should check that this did not make the compiler do something silly like not inlining it. I don't think it will be measurable change to performance, but worth a check.

@alexcrichton
Copy link
Member

Ok we got a chance to chat about this in the @rust-lang/cargo team meeting yesterday, and actually after some discussion it looks like this may not be our ideal end state.

Prerelease versions have always been somewhat buggy in Cargo in that they weren't thoroughly tested and designed well from the get go (more bolted on after the fact). To that end we tried to reevaluate what we would like the behavior of prerelease versions to be, and concluded that there are two main properties that we desire:

  • If a prerelease version is just published, it should not automatically be picked up by existing crates. Instead, prerelease versions should only be considered if explicitly included.
  • If a new version such as 1.3.0-beta is published, then it should unify with previous semver-compatible versions. For example if a new beta release for a semver-compatible release is published and requested, then all crates should be unified to use the beta release.

To that end this PR is implementing a solution that we ended up identifying as an antipattern, allowing duplicate prerelease versions instead of unifying them. This means, for example, that if 1.2.0 of a crate is published and is in use while a different crate requires 1.3.0-beta, this PR would cause both versions to be linked into the crate graph. If the 1.2.0 version is a public dependency, however, this would likely cause compilation errors now that there are two versions in the crate graph.

All in all we ended up deciding that we do not want to merge this PR as-is at this time. Instead we concluded that the prerelease version story is underbaked enough that we'd prefer to see an RFC specifying changes rather than only a PR. @Diggsey would you be willing to write up such an RFC? Do the above thoughts make sense?

@Diggsey
Copy link
Contributor Author

Diggsey commented Sep 20, 2018

@alexcrichton did you discuss the issue which motivated this PR? (#6016)

Specifically, the combination of crates.io being immutable and forever, and the choice to deviate from semver in this case, means there is no way to "go back" to pre-1.0 stability guarantees once a 1.0 version has been published, even if that publication was a mistake. IMO, when releasing a new major version, it should be possible to have a period of relaxed stability guarantees (ie. a pre-release period) without breaking all crates downstream when the actual release happens.

Regarding your two points:

  • If a prerelease version is just published, it should not automatically be picked up by existing crates. Instead, prerelease versions should only be considered if explicitly included.

I definitely agree with this.

  • If a new version such as 1.3.0-beta is published, then it should unify with previous semver-compatible versions. For example if a new beta release for a semver-compatible release is published and requested, then all crates should be unified to use the beta release.

Under semver, pre-release versions are not guaranteed to be compatible with any other versions, so silently using 1.3.0 instead of 1.3.0-beta will break builds which would have otherwise succeeded (at the cost of duplicate versions).

Note that if consumers of the pre-release want to upgrade automatically, they can always use >=1.3.0-beta, <2.0.

In summary: the ^version bound on versions is intended to match crate versions which are guaranteed (under semver) to be compatible with the specified version. This means that cargo must be deviating from semver in one of these two ways:

  1. The ^ operator no longer represents semver compatibility, but is instead shorthand for >=x, <y
  2. Pre-release versions are now required to be forwards compatible with the actual release (in which case, what's the point of them, why not just release the actual major version?)

In order for cargo to have a consistent behaviour, one of these two options should be chosen by the @rust-lang/cargo team, or else the decision should be changed.

@withoutboats
Copy link
Contributor

I think that's not what Alex meant. He meant that 1.3.0-beta should be compatible with 1.2.0, not with 1.3.0. That is, our intuition is that prereleases represent "forking paths:" a prerelease version is compatible with all prior versions the non-prerelease version would be compatible with, but not with that version or any subsequent version.

@Diggsey
Copy link
Contributor Author

Diggsey commented Sep 20, 2018

@withoutboats ah that makes more sense. In that case, how will multiple pre-release versions be handled?

For example, it makes sense to me that 1.3.0-alpha would be compatible with 1.2.0, but it doesn't follow that 1.3.0-beta is compatible with 1.3.0-alpha (even though it would still have to be compatible with 1.2.0).

The goal from my point of view is to be able to publish N new major pre-release versions (ie. for a 1.x crate to be able to publish 2.0.0-alpha, 2.0.0-beta, etc. where there are no compatibility guarantees between those pre-release versions (or a potential future 2.0.0 release)

@withoutboats
Copy link
Contributor

I think each prerelease should be treated as an independent fork and they aren't compatible with one another, just with previous compatible versions.

@alexcrichton
Copy link
Member

Ah yes @withoutboats's clarifications are right, but I think though I'd prefer if you can still only have one prerelease version in a crate graph (that's compatible)

For example if you have one crate which requests 1.2.0-alpha, then everything prior to 1.2.0 should use 1.2.0-alpha. Once 1.2.0 is published then a dependency requirement of 1.2.0-alpha won't match that, but 1.2.0-alpha will continue to conflict with 1.2.0.

If one crate request 1.2.0-alpha and one requests 1.2.0-beta, I think that should be an error. Put another way I think our "semver compatible" logic as we have it today is correct and what we want to keep, but we need to tweak a bit when prerelease versions are available for version resolution and when they're selected.

@Diggsey
Copy link
Contributor Author

Diggsey commented Sep 21, 2018

I think though I'd prefer if you can still only have one prerelease version in a crate graph (that's compatible)

Hm, I'd much prefer it if we kept the notion we have today where crates are either compatible, or you can link multiple versions of them. What's the benefit of adding a relationship where crates are neither compatible nor can be linked together?

If you go back to the motivating example of mime_guess again, if pre-release versions cannot be linked together, then the crate author is still stuck in this impossible situation where there's no way for them to rectify their mistake of accidentally publishing a 1.0 crate.

I think this will cause other issues too: let's say we have two crates (A, B) each of which has a (private) dependency on different pre-release versions of a crate C. Now A and B can never be linked together, even though crate C is purely an implementation detail of the two crates.

Today this can never happen: regardless of what version (pre-release or otherwise) of C, A and B depend on, either the two versions of crate C are compatible (in which case the newer version will be chosen) or else they are incompatible (in which case crate C will be linked twice).

These are the three things I think are important in order:

  1. Backwards compatibility (things that did work must continue to work)
  2. Flexibility (as many configurations as possible should work)
  3. Clarity (it should be obvious why things don't work)

Disallowing multiple pre-release versions helps with 3) because you less frequently have errors about incompatible types coming from two versions of the same crate, but it comes at the cost of 2) which I think is more important (because dependency management is just a means to an end: getting your program to compile).

I think there are other things that could be done to help with 3) without preventing multiple pre-release versions to be combined:

  • Better error messages when types from different versions of the same crate are mixed
  • Distinguish public from private dependencies at the cargo level
    • Disallow publishing non-pre-release versions of crates with public dependencies on pre-release crates
  • Warn or error when multiple versions of a dependency are publicly exported

@Eh2406
Copy link
Contributor

Eh2406 commented Sep 22, 2018

I don't have an opinion on how pre-release version should work, but I have some nits with your arguments.

there's no way for them to rectify their mistake of accidentally publishing a 1.0 crate.

The next semver version is 2.0, and there is an infinite supply after that. The situation is that if feels like a small deal to release a new 0.x version, but feels like a big deal to release a x.0 version. Another way to deal is to release a new crate until things stabilize, then make a long term stable 2.0. There aren't perfect solutions but there are ways to deal with the mistake.

Today this can never happen: regardless of what version (pre-release or otherwise) of C, A and B depend on, either the two versions of crate C are compatible (in which case the newer version will be chosen) or else they are incompatible (in which case crate C will be linked twice).

To be pedantic, that is only true if normal semver type dependency requirements are used. If A requires "C" = "=1.1.0" (or "~1.1.0") and B "C" = "=1.2.0" (or "~1.2.0") then they can not be built together. My impression is that the only way to get a pre-release version is to use an exact version requirement, so it seems consistent for it to be impossible to combine two different = requirements.

Distinguish public from private dependencies at the cargo level

There is an accepted rfc for that rust-lang/rust#44663, but it has not yet been implemented.

@Diggsey
Copy link
Contributor Author

Diggsey commented Sep 22, 2018

The next semver version is 2.0, and there is an infinite supply after that. The situation is that if feels like a small deal to release a new 0.x version, but feels like a big deal to release a x.0 version.

It's not just how it feels: it also becomes much harder to work with version numbers like that. If my crate has a single public dependency, then I can no longer easily track which versions should be used together if I have to bump the major version every time the dependency does a patch release.

My impression is that the only way to get a pre-release version is to use an exact version requirement, so it seems consistent for it to be impossible to combine two different = requirements.

You can get pre-release versions with non-exact version requirements, you just have to mention the pre-release version in the requirement. (eg. ^1.2.0-alpha will get you that pre-release version. The problem today is that it will automatically upgrade to ^1.2.0 when that is released.

@Diggsey
Copy link
Contributor Author

Diggsey commented Sep 22, 2018

There's also the issue that major versions imply a level of stability / long term support to users of the crate which is unwarranted in this case. Pre-release versions clearly indicate that there will likely be breaking changes in every release.

@withoutboats
Copy link
Contributor

withoutboats commented Sep 22, 2018

On consideration, I agree with Alex that we should only allow one of the "forked paths" to exist at a time. Consider the case where three crates depend on 1.0, 1.1-alpha, and 1.1-beta; we've said that 1.0 unifies with either of the other two, but they don't unify with one another. Which version should the crate that depends on 1.0 get? It seems we'll have to select in an arbitrary way, and possibly that our selection could change between release. Seems bad.

Today this can never happen: regardless of what version (pre-release or otherwise) of C, A and B depend on, either the two versions of crate C are compatible (in which case the newer version will be chosen) or else they are incompatible (in which case crate C will be linked twice).

Isn't this false because of the current behavior of prereleases (which you're trying to change)? I thought that currently they are treated as incompatible but are not linked twice, resulting in a hard error. Be careful not to overstate your case: you believe the behavior should be that crates are either compatible & unified or incompatible & linked multiple times, but unless I've misunderstood right now that isn't the behavior of cargo.

@Diggsey
Copy link
Contributor Author

Diggsey commented Sep 22, 2018

Isn't this false because of the current behavior of prereleases (which you're trying to change)? I thought that currently they are treated as incompatible but are not linked twice, resulting in a hard error.

AFAIK, pre-releases are currently treated as being compatible with each other, unless there is some secondary code path I'm unaware of - my PR is specifically to change that (make them incompatible) so that multiple versions can be linked together. Cargo uses the same function (is_compatible) to determine whether versions are compatible vs determine whether two versions can be linked together, so all pairs of crate versions are either compatible, or can be linked together by definition. This is what I meant by "this cannot happen today".

Consider the case where three crates depend on 1.0, 1.1-alpha, and 1.1-beta; we've said that 1.0 unifies with either of the other two, but they don't unify with one another. Which version should the crate that depends on 1.0 get? It seems we'll have to select in an arbitrary way, and possibly that our selection could change between release. Seems bad.

That's a good point, and I agree with that logic for pre-release minor versions of a crate: the requirement for minor versions to be compatible precludes including multiple forks in the same dependency graph.

However, for pre-release major versions, there is no such requirement: there's no issue with 2.0.0-alpha being linked with 2.0.0-beta and even 2.0.0 itself - IMO, they should all be treated as though they were entirely separate major versions, like "the 2.0 that could have been".

@withoutboats
Copy link
Contributor

However, for pre-release major versions, there is no such requirement: there's no issue with 2.0.0-alpha being linked with 2.0.0-beta and even 2.0.0 itself - IMO, they should all be treated as though they were entirely separate major versions, like "the 2.0 that could have been".

Personally I'm open to this, or even to limiting the error to when there is actually a lesser version compatible with both incompatible prerelease versions.

@alexcrichton
Copy link
Member

Much of this discussion is also i think highly indicative of where the Cargo team figured an RFC was good, but I can respond to a few points as well:

Hm, I'd much prefer it if we kept the notion we have today where crates are either compatible, or you can link multiple versions of them.

Oh for sure! That's what I intend to keep as well. I meant that if you request an alpha version the semver requirement won't match beta, so if those two versions are in a crate graph I'd advocate for the being considered compatible which results in no possible resolution graphs to satisfy our constraints (one must have beta, one must have alpha, they are considered distinct).

Even for new pre-release major versions it seems to me that we'd want to consider them compatible in the sense of only requiring one in the crate graph. I'd imagine that all the same arguments apply for public dependencies and such

@alexcrichton
Copy link
Member

I'm going to close this as activity has died down for quite some time, I'll be sure to update discussion on #6016

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.

6 participants