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

Dependencies resolution with --minimal-versions #5657

Open
matklad opened this issue Jun 26, 2018 · 71 comments
Open

Dependencies resolution with --minimal-versions #5657

matklad opened this issue Jun 26, 2018 · 71 comments
Labels
C-tracking-issue Category: A tracking issue for something unstable. S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. Z-minimal-versions Nightly: minimal-versions
Projects

Comments

@matklad
Copy link
Member

matklad commented Jun 26, 2018

Implementation PR: #5200
Docs: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#minimal-versions
Issues: Z-minimal-versions Nightly: minimal-versions

Steps:

Unresolved questions:

  • do we want to "impose" this feature on the ecosystem? Currently, everything seems to work fine due to eager dependency resolution. Adding --minimal-versions has costs: one-time ecosystem transition cost, cost to run CI job for minimal versions, cost to actually update minimal versions. There's anecdotal evidence that wrong minimal versions actually are a problem: https://www.reddit.com/r/rust/comments/8ob598/rust_minimum_versions_semver_is_a_lie/e027mtz/.

  • should we implement "--minimal-versions-for-me-but-not-my-dependencies" as well, to make the initial roll-out of this feature easier?

Stabilization TODO:

  • change -Z minimal-versions to just --minimal-versions and add it alongside --frozen/locked,
  • this will require community-wide effort to make the things work, so a special announcement should be prepared. Announcement should describe the links problem and solution.
@matklad matklad added the C-tracking-issue Category: A tracking issue for something unstable. label Jun 26, 2018
@Eh2406
Copy link
Contributor

Eh2406 commented Jun 26, 2018

I would like to cc some persons:

I'd like to see us come to a shared understanding of what we would recommend users do with this feature both at rollout and at steady state in the idealized future.

  • A comically-pro-this-feature opinion thinks a crate is only valid if it can be built with minimal-versions, so we should check at publish time with no override, and we should yank all existing crates that don't meet this standard.
  • A comically-against-this-feature opinion thinks that the meaning of Cargo.toml is what cargo dose with it, so if ">=0.0" as a version requirement gets cargo build to work than what is the harm, So we should not stabilize this feature.

I think we are all in the middle, and probably closer than we think, we just need to articulate it. To start it off I will propose a position as a strawman, so you can point out where I am wrong.

All crates should try minimal-versions. If it can be made to work it should be added as a separate test in CI. As then things that depend on your crate will be able to use minimal-versions easily. yanking is generally not called for, only to be used when it predates rust 1.0.0 and it is a real problem for the ecosystem. If CI resources are tight, then minimal-versions can be combined with one of the other runs, like the minimal rust version or the beta run.

@klausi
Copy link
Contributor

klausi commented Jun 27, 2018

I'm currently not that interested in how strongly we should impose this option on the ecosystem. First we should make the minimal-versions option work properly. Not even cargo itself can be compiled with minimal-versions as explored in #5275.

Repeating the open points here:

  1. cargo publish always needs to send the links key to the registry. This needs to be stabilized as default behaviour in stable cargo.
  2. Mass update the crates.io registry to add the links key where it is missing right now.
  3. Stabilize the -Z minimal-versions flag into --minimal-versions on Cargo stable

@Eh2406
Copy link
Contributor

Eh2406 commented Jun 27, 2018

Well one small peace is done. At this time cargo publish from stable pushes the links key.

@klausi
Copy link
Contributor

klausi commented Jun 27, 2018

Oh cool, that is good news!

The inconsistent crates.io registry is still a problem. @alexcrichton said in rust-lang/crates.io#7310 that getting minimal-versions to work smoothly was not a priority then, so he didn't want to mass update the creates.io index. Is that still the assumption?

@SimonSapin argued that people will always use old cargo publish versions and by that make the crates.io registry inconsistent again. In order to prevent that we would have to implement server side rules on crates.io to prevent entries without links attributes. Which means breaking those old cargo publish versions.

For experimenting I'm maintaining a crates.io registry fork, that I update very irregularly. It has the links attribute fixed for some popular crates such as curl-sys.

CARGO_REGISTRY_INDEX=https://github.com/klausi/crates.io-index cargo build -Z minimal-versions

@Eh2406
Copy link
Contributor

Eh2406 commented Jun 28, 2018

Is that still the assumption?

That is one of the questions I think we need to address. :-) Hopefully, @alexcrichton and @matklad will have a chance soon to articulate what there current thoughts are.

What we decided to recommend needs to work and fairly smoothly. That means that if there are hard changes that need to be made to make it work then we need to be willing to do them. Correspondingly if we are not willing to make the big changes then we need to recommend something smaller.

Big changes may include some or all of:

  • foundational and otherwize stable crates releasing new versions just to bump minimal dep.
  • yanking versions too old to be worth making a new release for.
  • manually updating the index to add the links attribute, and requiring it to be correct for all future uploads.
  • other ideas?

Recommend something smaller may include some or all of:

  • recommending a --minimal-versions-for-me-but-not-my-grand-dependencies
  • endorsing a community maintained fork of the index that simulates the "Big changes"
  • making it clear that this is an optional "nice to have" and that it is not worth doing unless your dependencies are already doing it.
  • having some kind of log of things cargo discover do not work, that gets included into the index if they get tried again. (this would require a lot of design work.)
  • other ideas?

@alexcrichton
Copy link
Member

I think we'll definitely want to get this working with the main index, and I think we probably just want to keep going as-is, fixing up crates and publishing them as we discover mismatches. In that sense I think it might be good to do some more work to get some of the "base crates" working and then perhaps make a post on internals asking for testers so we can discover new crates and publish new versions

illicitonion added a commit to illicitonion/lmdb-rs that referenced this issue Jun 28, 2018
Earlier versions of pkg-config don't build with any post-1.0 rust
compiler.

This is an attempt to get lmdb, and some non-trivial crates which depend
on it, building with `-Z minimal-versions`. See
rust-lang/cargo#5657 for more information.
illicitonion added a commit to illicitonion/tokio-signal that referenced this issue Jun 28, 2018
This allows tokio-signal to build with `-Z minimal-versions` - see
rust-lang/cargo#5657 (comment)
for more details.

Earlier versions depend on log 0.3.1, which itself depends on libc
0.1, which doesn't build on any post-1.0 version of rust.
@illicitonion
Copy link
Contributor

I got a non-trivial crate (https://github.com/pantsbuild/pants/tree/c8b42cb52eca9acbca98eaaf9599a47bc7b1f51e/src/rust/engine) compiling with -Z minimal-versions, and have started sending out a few PRs to the ecosystem.

My two big questions from this process are:

  1. What version of the log crate should libraries depend on? Nothing before 0.3.4 builds with post-1.0 rust. protoc depends on 0.* (https://github.com/stepancheg/rust-protobuf/blob/264debff3cd1cb26048925e90bec998941c2a328/protoc/Cargo.toml#L17), and many crates depend on 0.3. Most things which depend on 0.3 work with 0.4 It would be great to have some published advice on this one, as it's a slightly weird dependency in the first place. log itself notes some compatibility guidelines (https://docs.rs/log/0.4.2/log/#version-compatibility), but it would be nice to expand that to a firm suggestion ("You should always depend on the most recent version where possible" or "You should always depend on the oldest version which works for your library", or something else)
  2. If we decide that --minimal-versions is something people should strive to support, how should people handle dependencies which aren't actively maintained, and which need updating? If a transitive dependency is the only problem with an otherwise functioning dependency which hasn't been touched in 2-3 years, and which doesn't respond to PRs, what should people do? Fork the crate? Try to get the bad transitive dep yanked? Give up?

@alexcrichton
Copy link
Member

@illicitonion nice!

Those are indeed good questions too :). You can somewhat force the process by having some crate have a higher version bound (aka requiring 0.3.10 of log synthetically) but that's not a great solution to either problem. I think for now the best advice we'd have is "try to send a PR to the crate and get a new version published" but that indeed reduces the usability of this feature :(

@Eh2406
Copy link
Contributor

Eh2406 commented Jul 2, 2018

Actually I quite like the suggestion,

If you minimal-versions build is broken by log then add a dependency on 'log = "3.10"' to your Cargo.toml.
If you can't do to your crate already depending on 'log = "4"' then depend on the helper crate 'logs-that-works-with-minimal-versions = "1"' witch is just a Cargo.toml that has 'log = "3.10"'.

I don't think "try to send a PR to the crate and get a new version published" reduces the usability, I think it is begging the question "If a transitive dependency is the only problem with an otherwise functioning dependency, and which doesn't respond to PRs, what should people do?".

@alexcrichton
Copy link
Member

A snag I've now thought of as well: from time to time crates will break due to language/compiler changes, but we're generally pretty good about ensuring that the most recent version on crates.io always builds and point releases are updated. This means, however, that lots of crates' CI will break when that Rust version is published, because not everyone will say they require the newer version of the crate.

@matklad
Copy link
Member Author

matklad commented Jul 13, 2018

@alexcrichton I think that depends on the CI setup. It seems to me that long-term the CI job with --minimal-versions should also use the minimal supported Rust version(MSRV): this is needed so that crates can bump minimal supported rust version in minor release, and it also saves one CI job as well.

However, the day when your MSRV supports --minimal-versions is far away; in the meantime, the following guideline should work:

  • if your MSRV has --minimal-version, add a single CI job which both sets MSRV and --minimal-versions
  • otherwise, add two jobs:
    • MSRV with a lockfile (otherwise CI breaks when your deps bump their MSRV)
    • --minimal-version with rust 1.xy, where 1.xy is the first rust release to support --minimal-versions

@Eh2406
Copy link
Contributor

Eh2406 commented Jul 13, 2018

@matklad in the meantime can it be done in one CI job now with cargo +nightly generate-lockfile --minimal-versions && cargo +MSRV test?

@matklad
Copy link
Member Author

matklad commented Jul 13, 2018

Excellent idea @Eh2406!

@alexcrichton
Copy link
Member

@matklad makes sense to me! And I like @Eh2406's idea as well, that should make an excellent suggestion for how to best use this flag

@Eh2406
Copy link
Contributor

Eh2406 commented Jul 25, 2018

This is a list of foundational crates with versions that do not build on modern rust. Purging these from tomls is the "startup cost" of getting minimal-versions working. This is in a format that can be copied into a tomls to fix each dep.

  • winapi = "0.2.7"
  • libc = "0.1.x" dont know x yet
  • log = "0.3.4"
  • num = "0.1" update to num-traits

I will keep this up to date as I find more, and one day make a working-with-minimal-versions-hack crate with this as it's starting toml.

@Eh2406
Copy link
Contributor

Eh2406 commented Jul 25, 2018

So @dwijnand asked me to write up how do figure out what to do when a minimal-versions CI job fails.

So hear gose.

  1. Reproduce locally. This involves checking out the branch and running what the CI job does. IntelliJ and RLS has a habit of updating a lock (without -Z minimal-versions) file on Cargo.toml change, So just to be safe I tend to close my editer when investigating.
  2. Identify the problematic dependency. This is often easy to do by reading the path where the errors occurred. For example this hit an error in C:\Users\appveyor\.cargo\registry\src\github.com-1ecc6299db9ec823\libc-0.1.1\rust/src/liblibc/lib.rs:79:46 So that is libc-0.1.1 It is significantly harder if it is a build script that errored, as it can be any build-dep of that dep.
  3. Determine how that ended up in your tree. I find cargo tree extreamly helpful for this. So I do cargo +nightly generate-lockfile -Z minimal-versions && cargo tree -p libc:0.1.1 -i Cargo tree also updates the lock (without -Z minimal-versions) if it is not fully up-to-date, so I always run the two commands in one line. Also the format for specifying a dep uses a : instead of a - so watch out for that.
  4. Find something to add to your toml to fix the dep. Best case is that the most recent version of the thing you actually depend on builds with minimal-versions requiring to that can solve a large swath of problems. For example curl = "0.4.13+" . Next best is that newer versions the thing you actually depend on no longer require the thing that causes the problem. This can solve the immediate problem, but can release new problems. For example git2 = "0.7.3" Next is to add a synthetic deps. I find that opening a tab for each compatible version of a deb on crates.io makes it fairly easy to binary search for when a dep got bumped.

danburkert pushed a commit to danburkert/lmdb-rs that referenced this issue Aug 4, 2018
Earlier versions of pkg-config don't build with any post-1.0 rust
compiler.

This is an attempt to get lmdb, and some non-trivial crates which depend
on it, building with `-Z minimal-versions`. See
rust-lang/cargo#5657 for more information.
carllerche pushed a commit to carllerche/tokio-signal that referenced this issue Sep 10, 2018
This allows tokio-signal to build with `-Z minimal-versions` - see
rust-lang/cargo#5657 (comment)
for more details.

Earlier versions depend on log 0.3.1, which itself depends on libc
0.1, which doesn't build on any post-1.0 version of rust.
@epage
Copy link
Contributor

epage commented Apr 21, 2023

It's also worth noting that (I believe) both Go and npm have version resolution that chooses minimal versions rather than maximum versions by default. That's not necessarily an argument for us doing the same, but I wonder if their rationale for that choice is documented anywhere.

The quintessential golang post is https://research.swtch.com/vgo-mvs

See also the HN discussion: https://news.ycombinator.com/item?id=16433425

@kornelski
Copy link
Contributor

One option to consider is treating dependency requirement syntax differently, and when a full version is specified, treat it as minimum:

serde = "1" # means maximum version
serde = "1.1.123" # minimum version instead
serde = "^1.1.123" # means maximum version again

This is a departure from what Cargo does currently and would need an opt-in mechanism (perhaps edition?), but OTOH it is more similar to what npm does.

@ehuss ehuss added the S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. label Apr 25, 2023
elasticdog added a commit to EarthmanMuons/rustops-blueprint that referenced this issue May 16, 2023
This is to reliably test MSRV by ensuring that the lower-bounds for
dependencies are correct, rather than testing the MSRV Rust version and
minimal cargo dependency versions separately.

See:
- rust-lang/cargo#5657
github-merge-queue bot pushed a commit to EarthmanMuons/rustops-blueprint that referenced this issue May 16, 2023
This is to reliably test MSRV by ensuring that the lower-bounds for
dependencies are correct, rather than testing the MSRV Rust version and
minimal cargo dependency versions separately.

See:
- rust-lang/cargo#5657
@epage
Copy link
Contributor

epage commented Jul 12, 2023

Since others may be interested, here is a blog post explaining why this isn't default: https://web.archive.org/web/20190408003011/http://aturon.github.io/2018/07/25/cargo-version-selection/ (reddit discussion)

@carols10cents
Copy link
Member

I expanded all the comments on this issue and then did CTRL-F for "path" and didn't find this concern raised, so I apologize if I've missed discussion of this somewhere but it doesn't look like I have.

I recently had an issue with a project that uses croaring, and I upgraded croaring, but I neglected to upgrade croaring-sys as well. My project's lockfile kept it on an older version of croaring-sys, which caused my project to fail to build. The version specified in croaring for croaring-sys indicated that the older version of croaring-sys should have been compatible, but that wasn't actually true. Longer explanation over here: RoaringBitmap/croaring-rs#124

So I thought "this is a job for -Zdirect-minimal-versions! I'll send in a PR adding that job to CI, show that it would catch the problem I had, and then fix the problem!" only my plan didn't work because when building croaring in croaring's project, the path dependency on croaring-sys gets used even if -Zdirect-minimal-versions is specified.

So it seems like the path dependency is preventing Cargo from even considering a minimally compatible version that might be on crates.io. I think when -Zdirect-minimal-versions is specified, Cargo should consider both existing versions on crates.io that might satisfy the constraint as well as the path dependency, in case the dependency hasn't been published yet.

I can work up a minimal reproduction of this problem in a repo if that would be helpful.

@Nemo157
Copy link
Member

Nemo157 commented Oct 16, 2023

I was hoping that cargo minimal-versions would deal with path dependencies, but it just has an open PR adding the ability: taiki-e/cargo-minimal-versions#4. I can confirm that using that branch and cargo minimal-versions --detach-path-deps check successfully finds the issue in croaring.

@taiki-e
Copy link
Member

taiki-e commented Oct 23, 2023

it just has an open PR adding the ability: taiki-e/cargo-minimal-versions#4.

FYI, it has been merged and released in cargo-minimal-versions 0.1.20.

@daxpedda

This comment was marked as outdated.

@Eh2406

This comment was marked as outdated.

@ctz
Copy link

ctz commented Jan 29, 2024

Does -Zdirect-minimal-versions work as intended?

I see:

$ cargo minimal-versions --direct --ignore-private check
info: running `cargo update -Z direct-minimal-versions`
    Updating crates.io index
error: failed to select a version for `zeroize`.
    ... required by package `aws-lc-rs v1.6.0`
    ... which satisfies dependency `aws-lc-rs = "^1.6"` of package `rustls v0.23.0-alpha.0 (/home/runner/work/rustls/rustls/rustls)`
versions that meet the requirements `^1.7` are: 1.7.0

all possible versions conflict with previously selected packages.

  previously selected package `zeroize v1.6.0`
    ... which satisfies dependency `zeroize = "^1.6.0"` of package `rustls v0.23.0-alpha.0 (/home/runner/work/rustls/rustls/rustls)`

failed to select a version for `zeroize` which could resolve this conflict

To clarify: rustls has a dependency zeroize = "1.6.0", and a dependency on aws-lc-rs which itself has a dependency zeroize = "1.7".

I would expect this to be fine: direct-minimal-versions should minimize the rustls -> zeroize version to 1.6.0, and do the normal resolution for rustls -> aws-lc-rs -> zeroize. But instead, that error is seen.

This is 1.77.0-nightly (635124704 2024-01-27)

@epage
Copy link
Contributor

epage commented Jan 29, 2024

Direct-minimal-versions picks the minimum version for your dependencies (so 1.6). As a consequence, if it can't find a way for transitive dependencies on zeorize that are compatible with 1.6, it errors. It can't / shouldn't force aws-lc-rs to use 1.6 when it says it needs 1.7.

@ctz
Copy link

ctz commented Jan 29, 2024

Apologies, what I was expecting here was to use =1.6 for my immediate zeroize dependency, and then >=1.7 for aws-lc-rs. Sounds like I have misunderstood what the "direct" part of direct-minimal-versions means?

@mathstuf
Copy link
Contributor

It can't / shouldn't force aws-lc-rs to use 1.6 when it says it needs 1.7.

Why not give the dep 1.7 and the top-level crate 1.6? Or is this conservative based on "all dependencies may be public" logic and can only be broken with public = false dependencies?

@epage
Copy link
Contributor

epage commented Jan 29, 2024

Cargo unifies versions both because they may be public and to reduce build times. We could evaluate allowing duplicates in the future after public dependencies are stabilized and widely used (currently blocked on bugs in the compiler with no one stepping up to fix) but we'd have to tread very carefully because of the build time impact. We could easily avoid unifying in places it doesn't make sense and slow things down a lot.

@mathstuf
Copy link
Contributor

Diagnostics when duplication of dependencies occurs may be appreciated anyways. Those using minimal-versions may accept it in order to verify their own minimal version requirements accurately. Thanks for the details though.

toptalhook pushed a commit to toptalhook/proc-macro-crate that referenced this issue Jun 1, 2024
toml version `0.5.0` incorrectly specified serde version `1.0.0` as one
of its dependencies, even though it does not work with with serde
version `1.0.0`. This [was
fixed](toml-rs/toml-rs#311) and released in
version `0.5.2`.

This commit updates the toml dependency to version `0.5.2`. Now
`proc-macro-crate` builds fine with the cargo flag
[`-Z minimal-versions`](rust-lang/cargo#5657).
toptalhook pushed a commit to toptalhook/proc-macro-crate that referenced this issue Jun 1, 2024
toml version `0.5.0` incorrectly specified serde version `1.0.0` as one
of its dependencies, even though it does not work with with serde
version `1.0.0`. This [was
fixed](toml-rs/toml-rs#311) and released in
version `0.5.2`.

This commit updates the toml dependency to version `0.5.2`. Now
`proc-macro-crate` builds fine with the cargo flag
[`-Z minimal-versions`](rust-lang/cargo#5657).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for something unstable. S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. Z-minimal-versions Nightly: minimal-versions
Projects
Status: Unstable, no backers
Roadmap
  
Unstable, no backers
Development

No branches or pull requests