test(travis): Test build with minimal versions

[klausi]

We should make sure that cargo itself builds successfully with minimal dependency versions. Not sure this is the correct place in the Travis config, let me know if this should be added somewhere else.

This should currently fail because

$ cargo update -Z minimal-versions
...
$ cargo build
error: multiple packages link to native library `curl`, but a native library can be linked only once

package `curl-sys v0.4.0`
    ... which is depended on by `libgit2-sys v0.7.0`
    ... which is depended on by `git2 v0.7.0`
    ... which is depended on by `cargo v0.27.0 (file:///home/klausi/workspace/cargo)`
links to native library `curl`

package `curl-sys v0.3.10`
    ... which is depended on by `curl v0.4.6`
    ... which is depended on by `crates-io v0.16.0 (file:///home/klausi/workspace/cargo/src/crates-io)`
    ... which is depended on by `cargo v0.27.0 (file:///home/klausi/workspace/cargo)`
also links to native library `curl`

This is not good because cargo update -Z minimal-versions should produce a setup that is always able to be built. This can have 2 causes:

• cargo update -Z minimal-versions is broken and should work differently
• Dependencies in Cargo.toml are not specified correctly and need to be updated

[klausi]

The first build error with competing curl versions can be solved by bumping the curl version.

Now I'm falling a bit into a rabbit hole with lots of dependencies that need to yank their broken old versions from crates.io. Started with rust-lang/libz-sys#36

[matklad]

@klausi I think that rather than yanking, we should probably just bump minimal dependencies across the board?

For example, we can say = "1" here: https://github.com/alexcrichton/curl-rust/blob/a15e3e35a2f5c7b9bbc83c59768b1d12fbd05506/curl-sys/Cargo.toml#L23

[klausi]

The counter argument to that is then that all libraries across the Rust ecosystem need to update their minimum requirements for old package versions that are broken anyway.

In servo/rust-url#441 @SimonSapin decided to rather yank broken packages from crates.io, which makes a lot of sense to me.

[matklad]

Probably!

Personally, I wouldn't be too worried about checking with --minimal-version: because Cargo always tries to pick the latest versions, it doesn't really help that much.

That said, libz-sys = ">= 0" constraint in curl-sys definitelly seems weird :)

[SimonSapin]

For what it’s worth, those versions were published before Rust 1.0.0 and relevant changes to the crate were accounting for breaking changes in the language. Today this would be similar to a crate that requires unstable features, where the ecosystem’s convention is to update to new Rust in non-SemVer-breaking version nubmers.

[klausi]

Agreed that fixing their weird minimal versions in curl-rust also makes sense, opened alexcrichton/curl-rust#205

We can tackle this on multiple fronts :)

In the future every Rust library should add something like cargo update -Z minimal-versions && cargo build to their CI configuration. That way a library can ensure that it does not only build with maximum dependency versions (cargo default) but also with the minimum. Makes it easier to detect breaking API changes and to ensure that the library works over a spectrum of dependency versions.

[alexcrichton]

Thanks for the PR @klausi! I think though that this PR is mostly showing that we're not quite ready for this option.

One of the reasons we considered adding this was to ensure that everyone's "lower bound" in their Cargo.toml was correct as our suspicion was that it wasn't correct for many many projects. Clearly that appears to be the case!

I'm not so sure that we should test this for Cargo itself, but rather fix issues as they come up. Perhaps one day we may be able to do this on Cargo's own CI but I think for now we won't be able to.

I'll comment on some of the assorted issues as well, but I think that yanking probably isn't the right approach here but rather the intended solution to this was updating the minimum version required in Cargo.toml manifests.

[Eh2406]

Note that as part of the "multiple fronts" this multiple packages link to native library can also be fixed by retroactively adding a "links" attribute to the index. cc rust-lang/crates.io#7310

@alexcrichton I agree we should work on things as they come up. As crates try and use this feature, that should demand that their dependencies do it to, that is just how these things will come up. Also cargo as a library should be using this feature. I.E. this PR may take a long time to land but it should get implemented.

@klausi thanks for thinking of this!

[Eh2406]

To remove the dep on libc:0.1.0
filetime = "0.1.7"
git2-curl = "0.8.1"
edit cargo tree is very helpful

[alexcrichton]

I.E. this PR may take a long time to land but it should get implemented

makes sense to me!

[klausi]

I'm currently trying to make progress by testing crates (and cargo itself) with

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

(see also rust-lang/crates.io#7310 for the reason why I run my fork of the registry).

cargo fails to build with this because curl-sys has broken minimum dependencies. 2 ways to fix this:

1. Yank old broken libz-sys and pkg-config versions from crates.io (proposed that already above, Alex thought that was not quite the right way to fix this)
2. Release a new version of curl and curl-sys, fixed minimum dependencies are alrey committed to the master branch. Will open an issue for that.

[klausi]

I managed to compile cargo now with minimum dependency versions, yay! I modified the Travis config to test that with my registry index fork.

I had to use version ranges for some dependencies because for example "0.1.2" means that only this version can be installed, not "0.1.3" nor "0.2.0". This is special behavior for pre 1.0 versions, see https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html for background info.

If this passes then I think we can spin out the Cargo.toml dependency changes to a separate pull request and leave this for automating the minimal version check with Travis CI.

[matklad]

This could be crypto-hash = "0.3.1" I think. We actually have an issue about linting against < versions #5340 :)

[matklad]

I had to use version ranges for some dependencies because for example "0.1.2" means that only this version can be installed, not "0.1.3" nor "0.2.0".

I believe 0.1.3 could be installed! Quoting the linked docs:

The string "0.1.12" is a semver version requirement. Since this string does not have any operators in it, it is interpreted the same way as if we had specified "^0.1.12", which is called a caret requirement.

[bors]

☔ The latest upstream changes (presumably #5379) made this pull request unmergeable. Please resolve the merge conflicts.

[klausi]

Oh, you are totally right! I misunderstood how the version ranges work below 1.x and we should be good with specifying "0.x.y" in all cases.

I also had to bump tar because earlier versions want to use winapi 0.0.x which does not compile anymore.

[Eh2406]

You're not the first person to be confused by that, if you have ideas for what would make it clear since that might make a good PR. :-)

[matklad]

@Eh2406 hm, excellent idea, I think we can try to explain this specifically in #5340!

[klausi]

Cool, that one was merged! The remaining task here is now to establish a Travis CI config that ensures Cargo's minimal dependency versions keep compiling successfully in the future. In order for that to happen we need:

1. cargo publish always needs to send the links key to the registry. This needs to be stabilized as default behavior 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

After that all crates should be able to throw this into their CI config:

cargo build --minimal-versions

[SimonSapin]

cargo publish always needs to send the links key […]
Mass update the crates.io registry to add the links key where it is missing right now.

You can’t assume everyone uses the latest version of Cargo. Some people will keep using older versions, and those versions still need to be able to read the index and publish new crates.

[matklad]

@klausi I am trying to understand what needs to be done to move this forward.

I see we use custom registry here, to work around the links problem. Could we instead just bump some requirements to versions which include the links key?

After that we should be ready to merge it?

[klausi]

Unfortunately I think this will take some time until we can finish this. We need to do the steps 1. -3. I mentioned above.

That will not stop people from still publishing crates without the "links" attribute. 2 options to solve this:

1. Forbid people from publishing with old cargo versions on the crates.io server
2. Live with the fact that the "links" field on crates.io will always be inconsistent. We could refactor cargo's compile step: if a compilation fails because of a "links" clash then restart the dependency resolution process with the gained knowledge that a crate meta info is missing the "links" attribute. cargo could solve this locally by restarting itself as soon as it learns that "links" is missing.

[matklad]

Forbid people from publishing with old cargo versions on the crates.io server

Hm, my understanding that only ancient (pre-Rust 1.0) versions of Cargo don't set links on publish, so this is really not a problem in practice. Nobody should depend on crates without links, because they are likely not even build with modern rust.

That is, I still don't think we need to change anything in Cargo to move this forward, we only need to make sure that Cargo dependencies don't depend on ancient stuff like libz-sys 0.0.1. Am I missing something?

[alexcrichton]

@matklad oh FWIW publishing links to the registry is quite recent, supporting links in a crate graph already downloaded is pre-1.0.

[alexcrichton]

Should we perhaps close this PR in favor of the tracking issue for now?

[Eh2406]

I think they are separate, this is on adding minimal-versions to cargos CI, the tracking issue is on stabilizing the flag and messaging around that.

[alexcrichton]

True! But this is also testing the ability for a published Cargo to build with this flag rather than the in-tree version? Additionally I figured we may want to close until the standard registry has been fixed up

[Eh2406]

My impression is that what is preventing the mass fixing of the standard registry is your opposition. Nor have I heard an alternative for dealing with the conflicting sys crates issues.

In general if all my dependents can build with minimal-versions then I can be made to build with minimal-versions. What makes the sys crates issue so difficult is that it breaks this roul. Eche of my dependents can build with minimal-versions but they pick different compatible versions of cul-sys. I can not build, nor can I fix it with an upstream pr as they already use minimal-versions, nor can I fix it with a by changing my requirements (there is no way to require "=!0.3").

[matklad]

What makes the sys crates issue so difficult is that it breaks this roul. Eche of my dependents can build with minimal-versions but they pick different compatible versions of cul-sys. I can not build,

This happens because old versions of curl-sys were published with old Cargo's right? In that case, I think the fix is:

• get the maintainer of curl-sys to publish a new minor version with a new Cargo, so that the registry has links.
• Send PR to your dependencies which bump minimal curl-sys requirements to the newly published version, with comment along the lines "although --minimal-versions works for you, it doesn't work for your reverse-dependencies because a problem in an old version of Cargo with which the old version of curl-sys was published". After PR is merged, ask the maintainer to make a new minor release
• Bump minimal versions of said dependencies in your Cargo.toml.

That is, I think the problem is solvable by sending some PRs upstream, without modifying the registry?

[Eh2406]

You are technically correct. The best kind of correct.
So your solution involves every reverse dependency of all well established sys crates bumping requirements to the "post links in registry" version? That feels to me like a lot of chern, and a very long tail of things to be fixed.

[matklad]

So your solution involves every reverse dependency of all well established sys crates bumping

Hm, yeah... Like, every transitive rev-dep should be bumped, which I think in practice means every crate. At least, we can't have worse churn :-)

OTOH, I think the amount of churn will be massive even if we somehow fix the links issue: #5657 (comment) says that anything that depends on log 0.3 is broken, and that alone I think is a very significant chunk of the ecosystem.

[Eh2406]

But for non sys crates like log, there are workarounds. If anything in my dependency tree uses a log 0.3.4 then I can build, no matter how many dependents incorrectly claim log 0.3. If somehow all of my dependency tree uses a problematic formulation then I can just add a dependency on log-that-works-with-minimal-versions which is a crate (that I have not yet published) that has one dep log 0.3.4.

[alexcrichton]

Er well so I personally continue to feel that there's no need to mass update the registry, I feel like just not enough work has gone into fixing this in the ecosystem. If something is blocked on core sys crates like curl-sys I can always publish a new version!

[alexcrichton]

I think I've published enough crates now that if libgit2-sys's dependency version is updated to 0.7.5 in Cargo that should do the trick and we can build with minimal-versions

[Eh2406]

You claim the sys mess can be fix by new releases, I am sceptical so you demonstrate that it works. 😄 Well the proof is in the pudding. If it works it works and I am convinced. 😄

I was also thinking, how can a dependency tree depend on two semver versions of an sys crate? Why can that build at all? The answer the sys crates needs to be imported by a non semver respecting version_req. So this problem should be much smaller than I was thinking. So ya if you dep on an sys don't do it like 0.*, and if you have in the past then release a new version. 😄

[matklad]

Confirmed that Cargo builds with --minimal-versions with the following patch applied:

index e9d45290..23a930b2 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -27,7 +27,7 @@ failure = "0.1.1"
 filetime = "0.2"
 flate2 = "1.0"
 fs2 = "0.4"
-git2 = "0.7.0"
+git2 = "0.7.2"
 git2-curl = "0.8.1"
 glob = "0.2.11"
 hex = "0.3"
@@ -37,7 +37,6 @@ lazy_static = "1.0.0"
 jobserver = "0.1.9"
 lazycell = "1.0"
 libc = "0.2"
-libgit2-sys = "0.7.1"
 log = "0.4"
 num_cpus = "1.0"
 same-file = "1"
@@ -54,6 +53,8 @@ toml = "0.4.2"
 url = "1.1"
 clap = "2.31.2"
 unicode-width = "0.1.5"
+libssh2-sys = "0.2.8"
+libgit2-sys = "0.7.5"
 
 # Not actually needed right now but required to make sure that rls/cargo build
 # with the same set of features in rust-lang/rust

EDIT: 🎉

[Eh2406]

@klausi want to push the update @matklad tested and let's get this merged!

[Eh2406]

With that change cargo build -Z minimal-versions worked but it did not do what I intended. There was a lock file so it did not need to update it. cargo +nightly generate-lockfile -Z minimal-versions && cargo test did not bild do to winapi 0.0.1 not working. I am working on identifying where to make PRs upstream.

[alexcrichton]

Ah yeah I was just testing on Linux, it may still be broken on Windows!

[Eh2406]

adding gcc = "0.3.23" looks to have fixed it!

[klausi]

Thanks for taking this over @Eh2406 ! Let's continue in #5757