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

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

Closed
jtgeibel opened this issue Nov 16, 2017 · 17 comments
Closed

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

jtgeibel opened this issue Nov 16, 2017 · 17 comments
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted`

Comments

@jtgeibel
Copy link
Member

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
@alexcrichton alexcrichton added the C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` label Nov 17, 2017
@ishanjain28
Copy link

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
Copy link
Member

@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
Copy link

@alexcrichton CARGO_TARGET_DIR and CARGO_BUILD_TARGET_DIR both work correctly.

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

@alexcrichton
Copy link
Member

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
Copy link

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

@alexcrichton
Copy link
Member

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

@jtgeibel
Copy link
Member Author

@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
Copy link

@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
Copy link
Member Author

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

Here is the behavior I am seeing locally:

  • Within a working directory with a Cargo.toml
    • defaults to target/
    • CARGO_TARGET_DIR works correctly
  • Trying to install a crate from crates.io, such as CARGO_TARGET_DIR=t/ cargo install cargo-tree --verbose --force
    • The crate is always built in a temporary directory.

@ishanjain28
Copy link

ishanjain28 commented Dec 20, 2017

@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
Copy link

ishanjain28 commented Dec 20, 2017

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
Copy link
Member Author

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
Copy link

@jtgeibel Looks good to me. 👍

jtgeibel added a commit to jtgeibel/cargo that referenced this issue Dec 22, 2017
This aligns the behavior of crates.io and `--git` sources with that of `--path`
regarding the `CARGO_TARGET_DIR` and `CARGO_BUILD_TARGET_DIR` environment
variables.  If neither environment variable is set, then a temporary directory
is still used when installing from crates.io or `--git`.

As discussed in rust-lang#4725, this can be used to enable caching of artifacts between
continuous integration builds.
bors added a commit that referenced this issue Dec 22, 2017
Always respect `CARGO_TARGET_DIR` during `cargo install`

This aligns the behavior of crates.io and `--git` sources with that of `--path`
regarding the `CARGO_TARGET_DIR` and `CARGO_BUILD_TARGET_DIR` environment
variables.  If neither environment variable is set, then a temporary directory
is still used when installing from crates.io or `--git`.

As discussed in #4725, this can be used to enable caching of artifacts between
continuous integration builds.

/cc @alexcrichton, @ishanjain28
@alexcrichton
Copy link
Member

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

@kaleidawave
Copy link

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
Copy link

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
Copy link
Member

@Tarcontar

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted`
Projects
None yet
Development

No branches or pull requests

6 participants