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

rust/cargo: enable caching #4815

Merged
merged 2 commits into from
Sep 5, 2018
Merged

rust/cargo: enable caching #4815

merged 2 commits into from
Sep 5, 2018

Conversation

commitay
Copy link
Contributor

@commitay commitay commented Sep 4, 2018

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

The primary intention of this PR is to avoid having to download the complete crates.io-index package registry (currently a 125MB download) for each build. The formulae crate dependencies are also cached and can be reused between formulae. This will work without needing any changes to be made in formulae.

Similar to #4774 one downside is increased disk usage. Also, rust formulae aren't as common (currently 35), but we do seem to be adding them quickly (16 this year so far.)


One possible problem that I've encountered with setting CARGO_HOME occurs if the install target (i.e. "--root", prefix) isn't set as it will install by default into CARGO_HOME.

  Installing /Users/commitay/Library/Caches/Homebrew/cargo_cache/bin/bat
warning: be sure to add `/Users/commitay/Library/Caches/Homebrew/cargo_cache/bin` to your PATH to be able to run the installed binaries
Error: Empty installation

Disclaimer: My internet is terrible, so while this is a decent improvement for me locally it might not actually be that useful.

Removing cargo_cache for each formula:

/usr/local/Cellar/bat/0.6.1: 5 files, 5.2MB, built in 11 minutes 45 seconds

/usr/local/Cellar/bat/0.6.1: 5 files, 5.2MB, built in 5 minutes 34 seconds
/usr/local/Cellar/exa/0.8.0: 9 files, 1.4MB, built in 7 minutes 17 seconds

/usr/local/Cellar/exa/0.8.0: 9 files, 1.4MB, built in 2 minutes 38 seconds
/usr/local/Cellar/fd/7.1.0: 9 files, 3.1MB, built in 6 minutes 43 seconds

/usr/local/Cellar/fd/7.1.0: 9 files, 3.1MB, built in 1 minute 56 seconds
/usr/local/Cellar/ripgrep/0.9.0: 11 files, 4.9MB, built in 6 minutes 54 seconds

/usr/local/Cellar/ripgrep/0.9.0: 11 files, 4.9MB, built in 2 minutes 45 seconds
/usr/local/Cellar/watchexec/1.9.0: 7 files, 2.3MB, built in 6 minutes 52 seconds

/usr/local/Cellar/watchexec/1.9.0: 7 files, 2.3MB, built in 2 minutes 28 seconds

The cargo_cache size after building all of the above formulae again was 247MB (including the 125MB package registry.)

@ghost ghost assigned commitay Sep 4, 2018
@ghost ghost added the in progress Maintainers are working on this label Sep 4, 2018
@reitermarkus
Copy link
Member

What happens currently if "--root", prefix isn't specified?

Also, related: rust-lang/cargo#5183

@commitay
Copy link
Contributor Author

commitay commented Sep 4, 2018

What happens currently if "--root", prefix isn't specified?

warning: be sure to add `/private/tmp/fd-20180904-46716-1p1aatg/fd-7.1.0/.brew_home/.cargo/bin` to your PATH to be able to run the installed binaries
==> Cleaning
==> Finishing up
ln -s ../../Cellar/fd/7.1.0/etc/bash_completion.d/fd.bash fd.bash
ln -s ../../../Cellar/fd/7.1.0/share/fish/vendor_completions.d/fd.fish fd.fish
ln -s ../../../Cellar/fd/7.1.0/share/man/man1/fd.1 fd.1
ln -s ../../../Cellar/fd/7.1.0/share/zsh/site-functions/_fd _fd
==> Caveats
Bash completion has been installed to:
  /usr/local/etc/bash_completion.d

zsh completions have been installed to:
  /usr/local/share/zsh/site-functions
==> Summary
🍺  /usr/local/Cellar/fd/7.1.0: 7 files, 29.5KB, built in 10 minutes 55 seconds
  Installing /private/tmp/bat-20180904-47242-18uxzyh/bat-0.6.1/.brew_home/.cargo/bin/bat
warning: be sure to add `/private/tmp/bat-20180904-47242-18uxzyh/bat-0.6.1/.brew_home/.cargo/bin` to your PATH to be able to run the installed binaries
Error: Empty installation

Sometimes a successful install but will fail test as the binary isn't available, others a failed install.

@commitay
Copy link
Contributor Author

commitay commented Sep 4, 2018

We could probably do the same for cargo install and "--root", prefix as we do here:

find_method_with_args(body_node, :system, "dep", "ensure") do |d|
next if parameters_passed?(d, /vendor-only/)
problem "use \"dep\", \"ensure\", \"-vendor-only\""
end

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, thanks @commitay!

@colindean
Copy link
Member

colindean commented Sep 4, 2018

Does it make sense for an enhancement to allow a user to specify their own cache path? As in, if I'm working in Rust regularly, would it be worthwhile for me to be able to set HOMEBREW_CACHE_PATH_CARGO or something like that in order to have Homebrew use my user's CARGO_HOME path in ~/.cargo instead?

This optional centralization might save significant disk space, but we'd probably want to determine the benefit before spending the time to implement it.

@commitay
Copy link
Contributor Author

commitay commented Sep 4, 2018

Does it make sense for an enhancement to allow a user to specify their own cache path? As in, if I'm working in Rust regularly, would it be worthwhile for me to be able to set HOMEBREW_CACHE_PATH_CARGO or something like that in order to have Homebrew use my user's CARGO_HOME path in ~/.cargo instead?

I suppose we could in theory but we probably wouldn't want to go down that road as people would want similar support for go, npm and everything else. We'd also have to handle any potential conflicts that might arise from the user and Homebrew attempting to access the cache at the same time.

@MikeMcQuaid
Copy link
Member

Does it make sense for an enhancement to allow a user to specify their own cache path

@colindean FYI you can do that with HOMEBREW_CACHE for the Homebrew cache more generally but I agree with @commitay that it's not worth allowing each to be configurable. One can always use symlinks if needed.

@commitay commitay merged commit 88bf60d into Homebrew:master Sep 5, 2018
@commitay commitay deleted the cargo-cache branch September 5, 2018 11:22
@ghost ghost removed the in progress Maintainers are working on this label Sep 5, 2018
@commitay commitay removed their assignment Sep 5, 2018
@lock lock bot added the outdated PR was locked due to age label Oct 5, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants