-
Notifications
You must be signed in to change notification settings - Fork 4.5k
support cargo install in our docker environment #4102
Conversation
@@ -41,15 +41,17 @@ ARGS=( | |||
--rm | |||
) | |||
|
|||
if [[ -n $CI ]]; then | |||
# Share the real ~/.cargo between docker containers in CI for speed | |||
ARGS+=(--volume "$HOME:/home") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at buildkite-agent's $HOME/.cargo/bin and it had rustc, cargo, etc., which combined with the BASH_ENV stuff below would mean that buildkite-agent's version of cargo would be run instead of the docker image's version...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the tricks we use to speed up CI is to share the cache of stuff that gets downloaded into ~/.cargo across jobs. The lack of this cache is quite visible in the CI run for this PR, check out how the “check” step is much slower than usual and it’s now downloading a bunch of stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not seeing the download part, is it visible in this PR's CI? Once a .cargo for a working tree is set up, it should stick (via the .cargo entry in .gitignore), right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as an alternative, we could designate another place for CARGO_HOME that's shared by all CI, but that doesn't overlap the user's $HOME/.cargo
as yet another alternative, we could clean up buildkite-agent's ~/.cargo to exclude those things that come with the docker image
then again, adding CARGO_HOME/bin to the PATH is really what this PR is all about, and maybe we haven't figured out how to do that, and maybe we don't really have a good reason to do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I see the clippy downloads, but that looks to be part of the tree
there are cargo audit downloads as a result of the install, but maybe cargo audit should probably be part of our docker-rust image
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm..so in general I think we want to not “cargo install” in a rust docker container. Better to install via the Dockerfile that creates the docker image, into /usr. I know I know, we haven’t been good about that thus far. But if we are good about that then we still have ~/.cargo available for the cache of crates that keeps CI running fast(er).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we're only running stuff from Dockerfile installations, what will slow down CI? Registry stuff for the source tree? Is this a wart on rust or are we doing something wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s another cargo wart I think. Cargo home also stores git repos for the dependent crates of solana, can’t seem to separate that into another location from .cargo/bin. Compare the overall run times of this PR against another recent PR. The test-stable job in this PR is also slower than usual due to a cold cargo home dir.
closing in favor of #4113 |
Problem
current docker environment doesn't support "cargo install X && X", which would be nice to have for parts of the build
using real $HOME/.cargo, CARGO_HOME, and PATH=CARGO_HOME/bin will
alias/hide rustc, cargo, etc. from the docker image (the whole point
of the image)
Summary of Changes
use a workspace-based CARGO_HOME, add it's bin directory to the path for scripts and interactive shells
Fixes #4099