rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.66k stars 2.41k forks source link

Add an option to `cargo install` to specify a cache directory #4725

Closed jtgeibel closed 6 years ago

jtgeibel commented 6 years ago

In a CI environment it is sometimes necessary to run cargo install --force some-other-crate to produce binaries that are used later in the script. Currently, cargo install appears to create a temporary directory in which the specified crate and its dependencies are built anew for each invocation of the command. It would be nice if it was possible to cache these artifacts between CI builds.

For instance, when running CI for crates.io we install diesel_cli, rustfmt-nightly and clippy. On a recent build these took 81, 263, and 247 seconds respectively to build. It may be possible to shave nearly 10 minutes off the build time if cargo install could reuse cached artifacts from a previous build.

It would be even nicer if the same cache directory could be reused such that, for instance, serde v1.0.19 could be reused when building clippy because it was already built during the install of rustfmt-nightly. I believe the existing hashes included in the filenames for build artifacts would ensure that the 2 dependencies are truly equivalent (such as enabling the same features).

As an example, if the new flag was called --cache-dir it would be awesome if the following "just worked" and reused cached artifacts between all 3 commands when possible:

cargo install --force rustfmt-nightly --vers 0.2.15 --cache-dir=target/
cargo install --force clippy --vers 0.0.169 --cache-dir=target/
cargo build --release
ishanjain28 commented 6 years ago

Hi, I would like to work on this issue.

Can I work on this issue or is it taken?

I have been looking and fiddling with the source code for 2 days now and I understand some of it pretty well. I do have few questions before I get started. Should --cache-dir option be a flag local to build, run and maybe check sub-commands or should it be a global flag like --explain or --list?

alexcrichton commented 6 years ago

@ishanjain28 sure!

I think one way to implement this would be to perhaps get the existing CARGO_TARGET_DIR env var working perhaps? (or maybe it already works?)

ishanjain28 commented 6 years ago

@alexcrichton CARGO_TARGET_DIR and CARGO_BUILD_TARGET_DIR both work correctly.

Should the --cache-dir flag set one of these variables?

alexcrichton commented 6 years ago

Ok thanks for checking @ishanjain28! I think for now I'd personally prefer to avoid adding a command line argument just yet (more surface area to stabilize on a lot of commands) and instead just document the usage of the env vars here.

ishanjain28 commented 6 years ago

@alexcrichton Ok. Information about CARGO_TARGET_DIR exists in "Environment Variables" section on doc.crates.io.

alexcrichton commented 6 years ago

@ishanjain28 sure yeah, but perhaps we could call it out in the documentation for cargo install as well?

jtgeibel commented 6 years ago

@ishanjain28 thanks a lot for looking into this! However, I'm not seeing the intended behavior when using the environment variables. I'm on cargo 0.25.0-nightly (930f9d949 2017-12-05).

mkdir target
CARGO_TARGET_DIR=target/ cargo install --force clippy --vers "=0.0.175" --verbose

This shows the arguments --out-dir /tmp/cargo-install.wCxpmZWK1VcE/release/deps -L dependency=/tmp/cargo-install.wCxpmZWK1VcE/release/deps passed to rustc. Are you seeing the same behavior?

I never thought to check for an environment variable, so it would definitely be helpful to note this in cargo install --help as well.

ishanjain28 commented 6 years ago

@jtgeibel I tried compiling a project using cargo 0.23 and then cargo built from latest HEAD. On my PC, It uses the directory specified in CARGO_TARGET_DIR.

Command,

mkdir t; CARGO_TARGET_DIR=t/ cargo install --verbose

Full rustc command,

rustc --crate-name walnut src/main.rs --crate-type bin --emit=dep-info,link -C opt-level=3 -C metadata=c8bc6d89d285e215 -C extra-filename=-c8bc6d89d285e215 --out-dir /home/ishan/walnut/t/release/deps -L dependency=/home/ishan/walnut/t/release/deps --extern x11cap=/home/ishan/walnut/t/release/deps/libx11cap-f7cc77d334434916.rlib --extern rayon=/home/ishan/walnut/t/release/deps/librayon-946fd63ee540349e.rlib --extern x11=/home/ishan/walnut/t/release/deps/libx11-9bd4f569a49e6388.rlib --extern fern=/home/ishan/walnut/t/release/deps/libfern-0f8624bbafc12ff7.rlib --extern log=/home/ishan/walnut/t/release/deps/liblog-71bf47fabf9ca4ff.rlib --extern chrono=/home/ishan/walnut/t/release/deps/libchrono-c45231d0cd1772d5.rlib --extern gif=/home/ishan/walnut/t/release/deps/libgif-49b5a5006d59845a.rlib

I'll add documentation about CARGO_TARGET_DIR in cargo install --help later today.

jtgeibel commented 6 years ago

@ishanjain28 it looks like the difference is that I'm specifying a <crate>.

Here is the behavior I am seeing locally:

ishanjain28 commented 6 years ago

@jtgeibel yes, Now I am able to replicate this issue. It arises because of this code here.

If no path is specified with --path option, It'll create a temporary directory and pass it as an argument to Workspace::ephermal() function and because Some(dir) was passed to Workspace::ephermal(), It just uses the temporary directory as target directory instead of calling ws.config.target_dir() which is the function that reads CARGO_TARGET_DIR and CARGO_BUILD_TARGET_DIR environment variables.

From a quick look, A simple fix for this would be to pass None to Workspace::ephermal().

ishanjain28 commented 6 years ago

Passing None triggers ws.config.target_dir(), So, It'll use the value specified in Environment variables and if no value is specified, It falls back to ~/.cargo/. So, May we should check environment variable value before calling Workspace::ephermal() and then pass an appropriate value to it?

jtgeibel commented 6 years ago

Thank you so much for investigating this @ishanjain28! I've been trying some things locally, what do you think about the following?

diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs
index 2c8c405f..c85923e4 100644
--- a/src/cargo/ops/cargo_install.rs
+++ b/src/cargo/ops/cargo_install.rs
@@ -159,13 +159,17 @@ fn install_one(root: &Filesystem,
     };

     let mut td_opt = None;
+    let mut needs_cleanup = false;
     let overidden_target_dir = if source_id.is_path() {
         None
+    } else if let Some(dir) = config.target_dir()? {
+        Some(dir)
     } else if let Ok(td) = TempDir::new("cargo-install") {
         let p = td.path().to_owned();
         td_opt = Some(td);
         Some(Filesystem::new(p))
     } else {
+        needs_cleanup = true;
         Some(Filesystem::new(config.cwd().join("target-install")))
     };

@@ -311,7 +315,7 @@ fn install_one(root: &Filesystem,

     // Reaching here means all actions have succeeded. Clean up.
     installed.success();
-    if !source_id.is_path() {
+    if needs_cleanup {
         // Don't bother grabbing a lock as we're going to blow it all away
         // anyway.
         let target_dir = ws.target_dir().into_path_unlocked();
ishanjain28 commented 6 years ago

@jtgeibel Looks good to me. 👍

alexcrichton commented 6 years ago

I think this is now documented/added, so closing!

kaleidawave commented 2 years ago

Hey is there any information / documentation on how this works? Tried caching the /.cargo/bin folder but it understandably doesn't work, the command doesn't show under cargo --list after restoring the cache. How does cargo install associate the binary of the subcommand within cargo or however it works :)

Tarcontar commented 8 months ago

Hi, this still seems to be an issue for me. Our IT is blocking any installations from C:\Users}"username"\AppData\Local\Temp\ on our systems. So right now I can not update any installed packages like e.g. cargo install cargo-tarpaulin because Microsoft Defender is blocking it. It would be great to have an option to specify that intermediate path for installations.

weihanglo commented 8 months ago

@Tarcontar

CARGO_TARGET_DIR env or --target-dir flag should work.