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

Cache tarpaulin on Travis CI #311

Merged
merged 1 commit into from Dec 29, 2019
Merged

Cache tarpaulin on Travis CI #311

merged 1 commit into from Dec 29, 2019

Conversation

jyn514
Copy link
Contributor

@jyn514 jyn514 commented Dec 29, 2019

Currently the recommended instructions say to use cargo install -f, which always rebuilds tarpaulin even if it's already installed. Removing the -f caches the build if it already exists, which can speed up builds by 10 minutes or more.

Currently the recommended instructions say to use `cargo install -f`, which always rebuilds tarpaulin even if it's already installed. Removing the -f caches the build if it already exists.
@rye
Copy link
Contributor

rye commented Dec 29, 2019

It's worth noting that prior versions of cargo did not update already-installed packages. I believe this is why the -f flag is needed; this ensures that you are always running the latest version of cargo-tarpaulin, which is helpful given the current behavior of Cargo.

In rust-lang/cargo#7560, soon to be released in Cargo 1.41.0 which is scheduled for approximately a month from now, the install-upgrade feature has been stabilized. With this feature, cargo install will also upgrade existing installations, and those running beta or nightly will already have this behavior.

However, since the install-upgrade behavior hasn't reached stable Rust/stable Cargo, I'd say this change to the README should wait until it has, just to avoid confusion.

@jyn514
Copy link
Contributor Author

jyn514 commented Dec 29, 2019

I believe this is why the -f flag is needed; this ensures that you are always running the latest version of cargo-tarpaulin

As an end-user, I don't particularly care whether I'm running the latest and greatest version of tarpaulin. I do care about having builds run 10 minutes faster though.

I know this is personal preference and I can close this PR if you don't agree, but in that case the readme should mention that caching ~/.cargo doesn't help with tarpaulin.

@rye
Copy link
Contributor

rye commented Dec 29, 2019

Don't worry about what I think! 😁 I like this change, I was just adding some context to why -f is needed.

Digging further, I stand corrected. I think the main reason we needed the -f flag was actually nightly-to-nightly breakage which appears to no longer be a factor for us, as of daf432a, which removed the need to opt into the semver_exempt bits of proc-macro2.

I should expand on what I said, just for completeness, and note one other quirk with Cargo's current behavior. The current (stable) behavior of cargo install is that it exits angrily if you already have cargo-tarpaulin installed:

[kristofer@meson ~]$ cargo +stable install cargo-tarpaulin
    Updating crates.io index
error: binary `cargo-tarpaulin` already exists in destination as part of `cargo-tarpaulin v0.10.0`
Add --force to overwrite

(Here, Cargo exited with with status 101.) So if we remove the -f flag from the recommended Travis invocation right now (and people follow that change) they will start getting the above error message if they already have cargo-tarpaulin installed, e.g. if they are using a cached previous installation of cargo-tarpaulin. With beta Cargo, though, this changes to

[kristofer@meson ~]$ cargo +beta install cargo-tarpaulin
    Updating crates.io index
     Ignored package `cargo-tarpaulin v0.10.0` is already installed, use --force to override

which is printed before Cargo exits with status 0, indicating success. This is a much softer failure, and perfectly pairs with this PR.


I do see and wholeheartedly agree with the motivation here. An installation of cargo-tarpaulin with -f requires building from scratch 150+ packages, and doing this for every build is definitely wasteful.

My only nit is that if this change is released before v1.41.0 of Cargo is released, the (modified) recommended configuration would produce errors on stable (v1.40.0) Rust builds with cached cargo-tarpaulin installations until those builds get Cargo 1.41.

Perhaps a temporary change could be to

before_cache: |
  if [[ "$TRAVIS_RUST_VERSION" == stable ]]; then
    cargo install cargo-tarpaulin -f
  elif [[ "$TRAVIS_RUST_VERSION" == beta ]] || [[ "$TRAVIS_RUST_VERSION" == nightly ]]; then
    cargo install cargo-tarpaulin
  fi

Here stable still needs -f to prevent build failures, but the beta and nightly versions, with the new behavior, would skip installing if cargo-tarpaulin was already installed and up to date.

@xd009642
Copy link
Owner

The reason force was in there was because before proc-macro stabilisation when you had to run tarpaulin on nightly it would only work on projects built with the same nightly version as it was built with which would cause CI crashes when a new nightly was released and tarpaulin was cached in CI.

That's no longer the case so I'll merge this 👍

@xd009642
Copy link
Owner

Ah hadn't realised that change wasn't in cargo stable currently. 1.41 is set to be released 2020-01-30 so if I'm set to release another version before then I'll add a note to the readme

@xd009642 xd009642 merged commit 03cde19 into xd009642:develop Dec 29, 2019
@jyn514 jyn514 deleted the patch-1 branch December 29, 2019 22:53
@jyn514
Copy link
Contributor Author

jyn514 commented Dec 29, 2019

Here, Cargo exited with with status 101

This doesn't cause a build failure because it's in before_cache.

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.

None yet

3 participants