AVRO-3870: [Rust][Build] Speed up CI for Rust #2517
Conversation
|
|
|
||
| - name: Cache Cargo for 1.65.0 (Restore Only) | ||
| if: matrix.rust == '1.65.0' && matrix.target != 'x86_64-unknown-linux-gnu' | ||
| uses: actions/cache/restore@v3 |
There was a problem hiding this comment.
This looks weird.
actions/cache@v3 does two things - first it restores the cache and later in the post-processing saves the cache.
Now you introduce the new actions/cache/restore@v3 which gives you more control when to restore but it should be used with actions/cache/save@v3 to have a better control when to save.
Using both actions/cache with any of the new more granular actions does not make sense to me.
There was a problem hiding this comment.
Having only restore for some cache is intended.
For lang/rust/target, if a target is wasm32-unknown-unknown, cargo test doesn't run. So if wasm32-unknown-unknown finishes earlier than x86_64-unknown-linux-gnu, the dependencies built for tests will not be cached right?
For ~/.cargo, it might not be necessary to separate by conditions.
But, at least web-assembly job should be configured restore only.
See https://github.com/apache/avro/actions/runs/6277247580/job/17048597779#step:7:12
All the jobs download dependencies except for web-assembly job.
I guess this means ~/.cargo of web-assembly was cached because web-assembly job finished first but ~/.cargo of web-assembly doesn't contain all the necessary dependencies for other jobs.
There was a problem hiding this comment.
I am still very confused about these changes :-/
IMO we should add ${{ matrix.target }} to the cache key. This way x86_64 caches won't interfere with the wasm ones.
There was a problem hiding this comment.
Also the handling of pre-1.70 and post-1.70 adds to the confusion.
Let's just disable the new sparse protocol until MSRV is 1.70.0 via https://doc.rust-lang.org/cargo/reference/config.html#registriescrates-ioprotocol (i.e. a global env variable for the whole workflow)
There was a problem hiding this comment.
I'll open a new PR with the proposed simplifications!
There was a problem hiding this comment.
I am still very confused about these changes :-/
IMO we should add ${{ matrix.target }} to the cache key. This way x86_64 caches won't interfere with the wasm ones.
As I explained here, I didn't intentionally use ${{ matrix.target }}.
Also the handling of pre-1.70 and post-1.70 adds to the confusion.
Let's just disable the new sparse protocol until MSRV is 1.70.0 via https://doc.rust-lang.org/cargo/reference/config.html#registriescrates-ioprotocol (i.e. a global env variable for the whole workflow)
This idea seems awesome!
|
As I explained here, defaults.run.working-directory seems not to affect to |
|
The latest change is a combination of solutions of mine and @martin-g . The degree of the performance improvement is not so large, but cost effective. |
|
Do you see any issues with #2538 ? |
|
Thank you, @sarutak ! |
AVRO-3870
What is the purpose of the change
This PR aims to make the CI for Rust faster.
In the current master, there are something wrong about actions/cache in
test-lang-rust-ci.yml.First, a directory
targetis tend to be cached but the path is wrong. The correct path islang/rust/target, not~/target.https://github.com/apache/avro/actions/runs/6277247580/job/17048599038#step:22:2
Second, as of Rust 1.70.0, Cargo changes the way to download dependencies.
https://blog.rust-lang.org/2023/06/01/Rust-1.70.0.html#sparse-by-default-for-cratesio
So, it's better not to share the cache for
~/.cargoVerifying this change
Done by CI.
Documentation