Minimal versions build #5757

Eh2406 · 2018-07-20T21:19:01Z

This is a conceptual rebase of #5275, to reiterate:
Big thanks to @klausi for doing most of the work!
Thanks to @matklad for pointing out that we could finish it.

I don't know if I have the Travis config quite correct, advice definitely wellcome!

edit: closes #5275

@klausi

Big thanks to @klausi for doing most of the work! Thanks to @matklad for pointing out that we could finish it.

rust-highfive · 2018-07-20T21:19:05Z

r? @matklad

(rust_highfive has picked a reviewer for you, use r? to override)

klausi · 2018-07-20T22:54:08Z

Tests are failing with minimal versions, but at least cargo builds with minimal versions. Yay!

We could leave out the test run as a first step and just ensure that cargo builds? How good/fast are you in tracking down the test fails to the responsible dependency that is broken in its minimal version? :-D

Eh2406 · 2018-07-20T23:23:40Z

I just pushed a commit that replaces && cargo test with && cargo check --test

Now I am digging into the windows problems, which I can reproduce locally.

Eh2406 · 2018-07-21T00:53:01Z

Good news, I have a Cargo.toml that passes cargo generate-lockfile -Z minimal-versions && cargo check --tests I did this by updating anything that gave me trouble to the latest version. So next is to see how many of thouws can be removed. Then how far back can I push each one.

The bad news, one of the things I needed to pin is winapi="0.2.8" and we already import winapi="0.3" and use it. I got around this by adding cargo-features = ["rename-dependency"] and foo = { version = "0.2.8", package = "winapi" } but that can't fly long term. The correct solution is for one of our dependencies to correct from winapi="0.2" => winapi="0.2.x". When I have identified x I will shop around for a dependency that wants to get on the minimal-versions train. If I don't find one I will publish a get-winapi2-working-with-minimal-versions-hack and depend on it. If that is too much of a hack I will back out the AppVeyor part of this PR and open an issue for adding it later, so we can get Travis working.

Eh2406 · 2018-07-21T02:13:16Z

A lot can be removed. Then I looked into what was importing each of the problem crates and a bunch more can be removed by bumping to the latest version of the thing we use. Importantly the thing that need winapi="0.2.8" so we are good!

alexcrichton · 2018-07-21T15:40:41Z

.travis.yml

@@ -32,6 +32,9 @@ matrix:
      install:
        - mdbook --help || cargo install mdbook --force
      script:
+        - cargo +nightly generate-lockfile -Z minimal-versions
+        - cargo check --tests
+        - cargo +nightly generate-lockfile


Our "best practices" I think for this was to set this on a new travis matrix entry with the minimum supported Rust version, right? Should that be done here?

I'm somewhat hesitant to do this for Cargo though in the sense that we don't really have a minimum supported Rust version, it's always stable and forward for Cargo.

Ya I was figuring that since we don't have a MRV we could just combine it.

alexcrichton · 2018-07-21T15:41:11Z

Cargo.toml

@@ -55,6 +54,9 @@ 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"
+gcc = "0.3.23"


Could a comment be added saying that some of these deps are synthetic? Also I think gcc should be in [build-dependencies], right?

good thought. I added comments.

alexcrichton · 2018-07-23T14:01:32Z

Do others on @rust-lang/cargo have thoughts on whether or not we should add this to CI? My worry is that we will spend a good deal of time maintaining this for Cargo without much benefit. Cargo itself will always compile on the latest stable/beta/nightly, but that's all we guaranteed and I think all that we really need to guarantee. Cargo is primarily a project for rustc and we're only guaranteeing that the Cargo works with the rustc that it shipped with, so if we're constantly having to bump the minimum supported Rust version for compiler fixes and such I don't think it's really buying us much in practice.

I'm curious what others think though!

withoutboats · 2018-07-23T14:21:39Z

@alexcrichton This seems like its about cargo's library dependencies being accurate (e.g. saying it depends on libc ^1.0 but it actually requires ^1.1); how does it relate to rustc versioning?

That said, I think the fact that making cargo's minimum versions accurate is so difficult is not a great sign for the --minimum-versions feature as a whole. I don't think its a great idea to encourage --minimum-versions in CI for all library authors if its too much work to maintain.

klausi · 2018-07-23T14:30:39Z

I don't think the benefits are small: if you have --minimum-versions then you notice backwards compatibility breaks as soon as you use a new function from a library that is not there in an old version. This is good for stabilizing and increasing compatibility in the Rust crate ecosystem.

I see cargo itself as a role model for other Rust crates. If we can make it work here (in this relatively large project) then that sends a signal that --minimum-versions is usable and CI maintainable. Eating our own dog food.

If it turns out that --minimum-versions is a constant painful effort to keep up to date then we could remove it again from CI.

klausi · 2018-07-23T14:31:53Z

Cargo.toml

+
+[target.'cfg(windows)'.dev-dependencies]
+gcc = "0.3.23" # required only for minimal-versions on windows. brought in by flate2 and others.
+cmake = "0.1.24" # required only for minimal-versions on windows. brought in as build-dependencie on libgit2-sys.


typo "build-dependencie"

Eh2406 · 2018-07-23T14:35:03Z

I am not on the cargo team, but here are my 2c anyway.

This is a form of dogfooding for Dependencies resolution with --minimal-versions #5657. If we can't do it ourselves then that is an important data point as to whether we should recommend it for others. So let us give it a try, like we did for cargo fmt, we can always remove it if it is to onerous.
This adds cargo check not cargo test for the minimal-versions configuration. We did this because there are tests that fail! That means that users of cargo as a crate can have a valid lockfile that builds that hits bugs that we test for, that is not great customer service. What do we tell them if they report throws bugs? "please update your lockfile, are cargo.toml is a way for us to do that for you but we cant be bothered." (note I have hit this when trying to build clippy from source each day.) On the other hand legitimately, cargo as a crate may not be a customer we care about.

Since I started typing,

@withoutboats yes, agreed. But we don't know how hard it is to maintain, we haven't tried yet.
@klausi looks like we agree.

alexcrichton · 2018-07-23T15:08:09Z

Ah yes good points @klausi and @Eh2406! And yeah @withoutboats to be clear (which I don't think I was!) I'm speculating right now that it'll be difficult to maintain this, although I'm not certain.

In light of that though I think it makes sense to try it out here and see what happens! @Eh2406 if you want to make this a separate Travis and AppVeyor builder, should be good to go!

withoutboats · 2018-07-23T15:41:46Z

I don't think the benefits are small: if you have --minimum-versions then you notice backwards compatibility breaks as soon as you use a new function from a library that is not there in an old version. This is good for stabilizing and increasing compatibility in the Rust crate ecosystem.

I edited my post to avoid saying the benefits are small, because its not exactly that I think they are small. I think there's a trade off between increasing the maintenance burden on library authors and making niche uses easier.

Its not a backwards compatibility or stability issue: the problem is that if someone ceilings their version, say =2.1.3, and stops accepting higher versions, they will have a bad time if other libraries that claim to depend on ^2.0 actually depend on 2.2 and so on.

But putting a ceiling on your dependency version is uncommon and discouraged by cargo's design. If its relatively easy for everyone to run --minimum-versions in CI, then its no problem to encourage them to do that so that when the need to ceiling your version does arise, their users don't have any issues. But I wouldn't want to tell people that they need to adopt a really big maintenance burden to support this use case.

Anyway, that's what testing it in cargo is all about. If our dogfooding shows that its burdensome, we'll keep that in mind when making recommendations about what other libraries should do.

Eh2406 · 2018-07-23T16:02:53Z

Travis is a separate builder. Trying to figure out how to shoehorn AppVeyor matrix into this use case. Suggestions/Pointers very welcome!

Eh2406 · 2018-07-23T17:40:10Z

I got it working! @alexcrichton how does that look?

alexcrichton · 2018-07-23T21:35:48Z

.travis.yml

@@ -26,6 +26,13 @@ matrix:
           ALT=i686-unknown-linux-gnu
      rust: beta

+    - env: TARGET=x86_64-unknown-linux-gnu
+           ALT=i686-unknown-linux-gnu
+      rust: nightly


I think this should list an explicit Rust version, right? (the minimum supported rust version?)

For projects that have a msrv. But as you pointed out cargo doesn't. Maybe stable?

Oh sure yeah Cargo technically doesn't but if we want to emulate "best practices" here then I think we should probably attempt it here as well. For now I'd just pick 1.27 as the current stable

alexcrichton · 2018-07-23T21:36:15Z

Cargo.toml

 git2-curl = "0.8.1"
+libgit2-sys = "0.7.5" # required only for minimal-versions. brought in by git2 and git2-curl.
+libssh2-sys = "0.2.8" # required only for minimal-versions. brought in by libgit2-sys.


I think I published new versions of libgit2/git2-rs, so could those new versions be used instead of adding synthetic deps here?

Eh2406 · 2018-07-24T00:16:50Z

So small hiccup. @alexcrichton pointed out that we can bump git2 to = "0.7.3" and should for #5733. Unfortunately after doing that winapi v0.2.5 comes back into the picture. 😢 (with/out the synthetic deps on libgit2-sys and libssh2-sys). The only thing with all ver having winapi="0.2" is curl so we are waiting on a new release of that with this PR includedd. Or me to publish a get-winapi2-working-with-minimal-versions-hack crate.

alexcrichton · 2018-07-24T14:05:48Z

Ok sure thing, published new versions of curl! Want to update to those here too?

Eh2406 · 2018-07-24T14:55:25Z

Done ❗️ Thanks! While we are using curl = "0.4.13+" we are using a valid version of winapi2. So we don't need to update the other thing depending on it.

alexcrichton · 2018-07-24T15:11:32Z

@bors: r+

Looks great!

bors · 2018-07-24T15:11:33Z

📌 Commit 4a8c70e has been approved by alexcrichton

bors · 2018-07-24T15:11:41Z

⌛ Testing commit 4a8c70e with merge f102a24...

@klausi

Minimal versions build This is a conceptual rebase of #5275, to reiterate: Big thanks to @klausi for doing most of the work! Thanks to @matklad for pointing out that we could finish it. I don't know if I have the Travis config quite correct, advice definitely wellcome! edit: closes #5275

bors · 2018-07-24T16:10:09Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing f102a24 to master...

run some tests with minimal-versions on CI In #5757 we discovered that sum test don't pass with minimal-versions, and so only added CI for `cargo check`. This PR is to see if that is still needed, and if it is then which test rely on upstream bugfix.

Eh2406 added 2 commits July 20, 2018 17:13

update min dependencies to build with minimal-versions

adc8b5d

Big thanks to @klausi for doing most of the work! Thanks to @matklad for pointing out that we could finish it.

add to CI

f5ced93

rust-highfive assigned matklad Jul 20, 2018

klausi mentioned this pull request Jul 20, 2018

test(travis): Test build with minimal versions #5275

Closed

test is ambitious let's start with check

17e0140

update min dependencies to build with minimal-versions on windows

8a7123c

alexcrichton reviewed Jul 21, 2018

View reviewed changes

clarify synthetic nature of dep

f6a29a7

klausi reviewed Jul 23, 2018

View reviewed changes

typo

96d3c05

separate Travis builder

16bbb95

separate AppVeyor builder

3fa032b

alexcrichton reviewed Jul 23, 2018

View reviewed changes

Use nightly for -Z but use MSRV=stable for test

2797f6c

Eh2406 force-pushed the minimal-versions-build branch from 94e3dbd to 2797f6c Compare July 24, 2018 02:43

update curl so we don't need so may hacks

4a8c70e

bors merged commit 4a8c70e into rust-lang:master Jul 24, 2018

Eh2406 deleted the minimal-versions-build branch July 24, 2018 17:49

matklad mentioned this pull request Jul 25, 2018

Upgrade to crossbeam 0.4 #5790

Merged

Eh2406 mentioned this pull request Jul 25, 2018

Dependencies resolution with --minimal-versions #5657

Open

Eh2406 mentioned this pull request Aug 2, 2018

run some tests with minimal-versions on CI #5856

Merged

HeroicKatora mentioned this pull request Apr 28, 2019

[Don't merge yet] Update glob dependency image-rs/image-png#119

Closed

ehuss added this to the 1.29.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minimal versions build #5757

Minimal versions build #5757

Eh2406 commented Jul 20, 2018 •

edited

Loading

rust-highfive commented Jul 20, 2018

klausi commented Jul 20, 2018

Eh2406 commented Jul 20, 2018

Eh2406 commented Jul 21, 2018

Eh2406 commented Jul 21, 2018

alexcrichton Jul 21, 2018

Eh2406 Jul 21, 2018

alexcrichton Jul 21, 2018

Eh2406 Jul 21, 2018

alexcrichton commented Jul 23, 2018

withoutboats commented Jul 23, 2018 •

edited

Loading

klausi commented Jul 23, 2018 •

edited

Loading

klausi Jul 23, 2018

Eh2406 commented Jul 23, 2018

alexcrichton commented Jul 23, 2018

withoutboats commented Jul 23, 2018

Eh2406 commented Jul 23, 2018

Eh2406 commented Jul 23, 2018

alexcrichton Jul 23, 2018

Eh2406 Jul 23, 2018

alexcrichton Jul 23, 2018

alexcrichton Jul 23, 2018

Eh2406 commented Jul 24, 2018

alexcrichton commented Jul 24, 2018

Eh2406 commented Jul 24, 2018

alexcrichton commented Jul 24, 2018

bors commented Jul 24, 2018

bors commented Jul 24, 2018

bors commented Jul 24, 2018

Minimal versions build #5757

Minimal versions build #5757

Conversation

Eh2406 commented Jul 20, 2018 • edited Loading

rust-highfive commented Jul 20, 2018

klausi commented Jul 20, 2018

Eh2406 commented Jul 20, 2018

Eh2406 commented Jul 21, 2018

Eh2406 commented Jul 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Jul 23, 2018

withoutboats commented Jul 23, 2018 • edited Loading

klausi commented Jul 23, 2018 • edited Loading

Choose a reason for hiding this comment

Eh2406 commented Jul 23, 2018

alexcrichton commented Jul 23, 2018

withoutboats commented Jul 23, 2018

Eh2406 commented Jul 23, 2018

Eh2406 commented Jul 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eh2406 commented Jul 24, 2018

alexcrichton commented Jul 24, 2018

Eh2406 commented Jul 24, 2018

alexcrichton commented Jul 24, 2018

bors commented Jul 24, 2018

bors commented Jul 24, 2018

bors commented Jul 24, 2018

Eh2406 commented Jul 20, 2018 •

edited

Loading

withoutboats commented Jul 23, 2018 •

edited

Loading

klausi commented Jul 23, 2018 •

edited

Loading