Make cargo-chef effective for CI builds #763
Conversation
athoscouto
force-pushed
the
optimize-docker-build-4
branch
from
38a60b4 to
e21dc73
Compare
athoscouto
force-pushed
the
optimize-docker-build-4
branch
from
6204f87 to
fa94d1b
Compare
athoscouto
changed the title
| # We need to install and set as default the toolchain specified in rust-toolchain.toml | ||
| # Otherwise cargo-chef will build dependencies using wrong toolchain | ||
| # This also prevents planner and builder steps from installing the toolchain over and over again | ||
| COPY rust-toolchain.toml rust-toolchain.toml | ||
| RUN cat rust-toolchain.toml | grep "channel" | awk '{print $3}' | sed 's/\"//g' > toolchain.txt \ | ||
| && rustup update $(cat toolchain.txt) \ | ||
| && rustup default $(cat toolchain.txt) \ | ||
| && rm toolchain.txt rust-toolchain.toml | ||
| && rm toolchain.txt rust-toolchain.toml \ | ||
| && cargo install cargo-chef |
There was a problem hiding this comment.
This is actually a regression. If rust-toolchain.toml changes cargo install cargo-chef will be re-run. Previously it would be cached.
There was a problem hiding this comment.
If the toolchain changes we'll have to rebuild everything anyway, the cost of running cargo install cargo-chef is marginal.
I think the benefit of having all rust tools built by the same toolchain outweighs the downsides.
Let me know if you still want me to move it back.
There was a problem hiding this comment.
That's fine. Just don't name the commit "Optimize ... layers" when in fact you're de-optimizing it.
If you have named your intend of "cleaning up" the layers, I wouldn't add such comment at all.
| COPY docker-entrypoint.sh /usr/local/bin | ||
|
|
||
| COPY --from=builder /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/ca-certificates.crt |
There was a problem hiding this comment.
It seems that reverse order would be better since docker-entrypoint.sh probably changes more often than ca-certificates.
Also don't you need to add apt-get install -y ca-certificates to builder stage?
I think it would be better to add apt-get install -y ca-certificates to chef stage and copy certificates from there so that changed dependencies wouldn't trigger reinstallation of certificates.
There was a problem hiding this comment.
It seems that reverse order would be better since docker-entrypoint.sh probably changes more often than ca-certificates.
That is true, but the entrypoint changes pretty rarely, and when that happens we'll only save a layer that weights the size of ca-certificates.crt. I preferred to keep it tidy by grouping together COPYs from the same place.
Let me know if you still want me to reorder.
Also don't you need to add apt-get install -y ca-certificates to builder stage?
I think it would be better to add apt-get install -y ca-certificates to chef stage and copy certificates from there so that changed dependencies wouldn't trigger reinstallation of certificates.
We don't have to because our Rust base image already does it.
There was a problem hiding this comment.
Thanks for the explanation about the rust base image containing ca-certificates.
Regarding the order. I'm fine with whatever order you pick. Just please name your intend correctly. If the commit was saying "reorder the layers to make them more readable at the expense of build speed" I wouldn't even add such comment. Saying that you're speeding something up when in fact you're doing the opposite is confusing.
|
I'll merge that, we'll improve on it if we're unhappy |
We need to cache Docker layers to
cargo-chefto work properly.This uses the Github Actions cache to save Docker layers, making docker build faster when no deps are changed.
It also changes the order of docker layers, to optimize cache usage when rebuilding and layer reutilization when pushing to the registry.