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

Pinning dependency versions prevents downstream users from depending on newer compatible releases #42

Closed
nightkr opened this issue Jan 7, 2022 · 9 comments
Labels
D-rejected A decision (D) has been made and the issue will not be worked on I-bug This issue (I) regards a (potential) bug in the project T-accepted Triage (T): Initial review accepted issue/PR as valid

Comments

@nightkr
Copy link

nightkr commented Jan 7, 2022

Situation:

Combine duplicate with another dependency that depends on heck ^0.3.3.

Reproduction:

$ cargo new foo
     Created binary (application) `foo` package
$ cd foo/
$ cargo add duplicate@=0.3.0 heck@=0.3.3
    Updating 'https://github.com/rust-lang/crates.io-index' index
      Adding duplicate v=0.3.0 to dependencies
      Adding heck v=0.3.3 to dependencies
$ cargo b
    Updating crates.io index
error: failed to select a version for `heck`.
    ... required by package `duplicate v0.3.0`
    ... which satisfies dependency `duplicate = "=0.3.0"` of package `foo v0.1.0 (/home/teo/junk/foo)`
versions that meet the requirements `=0.3.2` are: 0.3.2

all possible versions conflict with previously selected packages.

  previously selected package `heck v0.3.3`
    ... which satisfies dependency `heck = "=0.3.3"` of package `foo v0.1.0 (/home/teo/junk/foo)`

failed to select a version for `heck` which could resolve this conflict
$ cat Cargo.toml 
[package]
name = "foo"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
duplicate = "=0.3.0"
heck = "=0.3.3"

Expected Behavior:

duplicate is built using the semver-compatible heck 0.3.3 instead.

Actual Behavior:

The build fails because Cargo refuses to make a build graph that combines both heck 0.3.2 and heck 0.3.3.

Affected Versions:

0.3.0

Local Environment:

cargo 1.56.0 (4ed5d137b 2021-10-04)
rustc 1.56.1 (59eed8a2a 2021-11-01)

Miscellaneous:

Submitted a PR as #41.

@nightkr nightkr added D-discussion A decision (D) has not been made yet and is open to discussion I-bug This issue (I) regards a (potential) bug in the project labels Jan 7, 2022
@github-actions github-actions bot added the T-new Triage (T): Has yet to be reviewed label Jan 7, 2022
@nightkr
Copy link
Author

nightkr commented Jan 7, 2022

It's also worth mentioning that the pinning didn't actually provide any MSRV guarantees anyway, since you still have unpinned transitive dependencies.

@Emoun Emoun added T-accepted Triage (T): Initial review accepted issue/PR as valid and removed T-new Triage (T): Has yet to be reviewed labels Jan 7, 2022
@Emoun
Copy link
Owner

Emoun commented Jan 7, 2022

Thanks for this bug report.

Until a solution is found and released, a workaround for this issue would be to turn off the module_disambiguation feature (which is the one that uses heck). This can be done by turning off the default features: duplicate={ version = "0.3.0", default-features = false }

@Emoun
Copy link
Owner

Emoun commented Jan 7, 2022

It seems this is an artifact of cargo's implementation and could be solved by cargo itself in the future.

I therefore don't see this as an issue with duplicate itself and will close this issue.

@teozkr, I suggest you use the workaround I mentioned in my previous comment if this issue is blocking you. I'm planning on updating heck to v0.4 for the next major release, which is WIP (though I don't know when I'll get to it).
If you really need heck v0.3.3 and module_disambiguation desperately, I'd be willing to release a v3.1 with heck v0.3.3.

@Emoun Emoun closed this as completed Jan 7, 2022
@Emoun Emoun added D-rejected A decision (D) has been made and the issue will not be worked on and removed D-discussion A decision (D) has not been made yet and is open to discussion labels Jan 7, 2022
@nightkr
Copy link
Author

nightkr commented Jan 7, 2022

It seems this is an artifact of cargo's implementation and could be solved by cargo itself in the future.

It might not affect this case in particular, but there's a whole other can of worms you're opening as soon as you start allowing mixing crate versions (in particular, around trait resolution). I doubt it will be supported any time soon. And as the comment you linked to ends with:

The ~ requirement is indeed more strict than ^, but Cargo strongly discourages usage of any version requirement other than ^ unless you're really sure you know what you're doing

There does seem to be some movement towards using the MSRV field from Cargo.toml for generating compatible resolutions (https://rust-lang.github.io/rfcs/2495-min-rust-version.html#influencing-version-resolution), but I'm not sure about the current state of that.

@teozkr, I suggest you use the workaround I mentioned in my previous comment if this issue is blocking you. I'm planning on updating heck to v0.4 for the next major release, which is WIP (though I don't know when I'll get to it).
If you really need heck v0.3.3 and module_disambiguation desperately, I'd be willing to release a v3.1 with heck v0.3.3.

With all due respect to you and this project, it's not really sustainable for the whole ecosystem to have to do these upgrades in precise lockstep, which is the inevitable outcome if this practice becomes standard across the board. While I appreciate your offer to do an emergency release, we can't in good conscience depend on that happening every time this issue rears its head. The description of #43 also makes it seem like it is headed in the direction of further breakage, so it looks like our only long-term option is to eliminate our dependencies on duplicate.

@Emoun
Copy link
Owner

Emoun commented Jan 7, 2022

in particular, around trait resolution

How so? Cargo already allows mixing two different major versions of the same crate even if they have the same traits. Wouldn't this already be a problem then?

From my reading of the linked issue and @alexcrichton's comment, I didn't see any specific issue that would block this implementation except for manpower.

if this practice becomes standard across the board

Which practice are you referring to? pinning versions in general, duplicate's specific MSRV, or using an MSRV in general.
I suspect even if duplicate had a more mainstream MSRV, pinning would still be needed where dependencies don't provide them (which is the case here).

The description of #43 also makes it seem like it is headed in the direction of further breakage

Switching to heck to v0.4 should actually solve your problem, since cargo will then happily compile both v0.3.3 and v0.4.0 at the same time. (you can try it by changing to heck v0.4 in you example above)
However, I must warn you that duplicate will come with breaking changes of its own. See #31 for the open and closed issues, I'm hoping v0.4.0 will be the last version before v1.0.0

so it looks like our only long-term option is to eliminate our dependencies on duplicate

If you disable all duplicate's features, the crate has zero dependencies, so all this becomes irrelevant to you.
I can't find your code that uses module_disambiguation, but I suspect you wont need it (I have yet to see anyone other than me use it).

@nightkr
Copy link
Author

nightkr commented Jan 7, 2022

How so? Cargo already allows mixing two different major versions of the same crate even if they have the same traits. Wouldn't this already be a problem then?

Currently, foo_1_0_0::Foo and foo_2_0_0::Foo are always distinct, while foo_1_0_0::Foo and foo_1_0_1::Foo are equivalent (since Cargo effectively makes foo_1_0_0 an alias for foo_1_0_1). Allowing multiple semver-compatible releases would break anyone relying on that assumption. And worse, it would only break sometimes, in conditions that are relatively far removed from the underlying cause. That doesn't really affect duplicate, but imagine if some of your serde::Deserialize bounds broke every time you upgraded serde.

This isn't even guaranteed to fail at compile time. For example, it also affects downcasting values at runtime from &dyn Any into concrete types.

Which practice are you referring to? pinning versions in general, duplicate's specific MSRV, or using an MSRV in general.
I suspect even if duplicate had a more mainstream MSRV, pinning would still be needed where dependencies don't provide them (which is the case here).

Version pinning.

Switching to heck to v0.4 should actually solve your problem, since cargo will then happily compile both v0.3.3 and v0.4.0 at the same time. (you can try it by changing to heck v0.4 in you example above)

Until heck v0.4.1 comes out and we're back here again.

If you disable all duplicate's features, the crate has zero dependencies, so all this becomes irrelevant to you.
I can't find your code that uses module_disambiguation, but I suspect you wont need it (I have yet to see anyone other than me use it).

As I discovered after posting this issue, it turns out that all of our uses (but not the dependency declarations) of duplicate actually disappeared in a recent refactoring anyway for unrelated reasons.

@Emoun
Copy link
Owner

Emoun commented Jan 7, 2022

Allowing multiple semver-compatible releases would break anyone relying on that assumption.

How can anyone rely on this (technically)? At any one time, each crates is only compiled with and interfaces with 1 version of a dependency. Why does it matter what version other crates are using?
(I'm not trying to argue a point, just trying to understand. If you have references to how this happens that would be great)

Version pinning

While I understand the problem and agree that it is problematic, I don't see how to avoid this when trying to uphold an MSRV (even the ones more lenient than duplicate's). Any crate that has an MSRV must ensure its dependencies uphold it too, otherwise it'll be violated, rendering the MSRV an empty promise.
Is the only solution then to only use dependencies that have the exact same MSRV?

Until heck v0.4.1 comes out and we're back here again

Let's hope heck is an abandoned project. /s

@nightkr
Copy link
Author

nightkr commented Jan 7, 2022

How can anyone rely on this (technically)? At any one time, each crates is only compiled with and interfaces with 1 version of a dependency. Why does it matter what version other crates are using?
(I'm not trying to argue a point, just trying to understand. If you have references to how this happens that would be great)

Crate A depends a trait Foo, crate B defines a type Bar and implements a::Foo for it. Crate C depends on both A and B and tries to use b::Bar in a way that requires that b::Bar: a::Foo. If crates B and C have different versions of crate A then this check will fail, since ::a::Foo (from C's perspective) and ::b::a::Foo are now distinct traits that have nothing to do with each other (apart from being identical).

For this simple case you could solve this by reexporting A from B (pub use a; in B's lib.rs), but that falls down once you get into diamond dependencies.

At any one time, each crates is only compiled with and interfaces with 1 version of a dependency.

That's the most common case, but it isn't strictly true. You can use renamed crate references to depend on multiple (incompatible) versions of the same crate. This is sometimes used for compatibility layers (like futures-compat, which implements futures_0_2::Future for types that impl futures_0_1::Future, and vice versa).

While I understand the problem and agree that it is problematic, I don't see how to avoid this when trying to uphold an MSRV (even the ones more lenient than duplicate's). Any crate that has an MSRV must ensure its dependencies uphold it too, otherwise it'll be violated, rendering the MSRV an empty promise.

If libraries use wide viable version bounds then downstream application crates that do depend on some MSRV can add direct dependencies on the same upstream library with additional constraints. It also won't break existing dependencies unless they manually run cargo update, providing them an opportunity to add additional version constraints or bumping their own MSRV.

The downside is a somewhat more confusing experience for users that add the library as a new dependency but need to content with a restrictive existing MSRV. Hopefully the project.rust field becomes widely used soon, so that the data is available for smarter tooling in the future.

Let's hope heck is an abandoned project. /s

:P

@Emoun
Copy link
Owner

Emoun commented Jan 8, 2022

@teozkr thanks for the explanations, I appreciate you putting the time in to explain. I think I understand it, but it feels like quantum mechanics (if you think you understand it, then you don't).

I will think about this for a bit and try to find some sort of solution that would allow not pinning dependencies with more lenient MSRV's.
I can see that duplicate's MSRV policy might be too restrictive (or maybe my enforcement of it is), so maybe it needs to be relaxed a bit so that #41 could be acceptable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D-rejected A decision (D) has been made and the issue will not be worked on I-bug This issue (I) regards a (potential) bug in the project T-accepted Triage (T): Initial review accepted issue/PR as valid
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants