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

CI: Use Rust cache #901

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

CI: Use Rust cache #901

wants to merge 2 commits into from

Conversation

oeb25
Copy link
Contributor

@oeb25 oeb25 commented Jan 3, 2024

This adds Swatinem/rust-cache@v2 to all CI jobs, with the intention of speeding them up. Hopefully we can compare some runs to get empirical data of the improvements it gives.

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

This adds caching to all CI jobs that use cargo
… install mdbook using `taiki-e/install-action@v2`
Copy link
Collaborator

@Lorak-mmk Lorak-mmk left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

Do you know why the CI failed? Is it just a one-time fluke or some problem with PR / cache action / cargo?

Regarding the changes:

  • First commit looks fine
  • I don't understand the purpose of second commit. Could you describe in the body of commit message why is it beneficial to add the usage of dtolnay/rust-toolchain / taiki-e/install-action? Also, could you rewrite the message to make it's title shorter? Right now it's wrapped around by GitHub.

Also, could you rebase on latest main?

After you push the changes, the CI will run again so we'll know if the failure was just a one time error or some real problem.

Comment on lines +30 to +32
- uses: dtolnay/rust-toolchain@master
with:
toolchain: stable
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's wrong with the toolchain that is already on the runners?

Comment on lines +35 to +37
uses: taiki-e/install-action@v2
with:
tool: mdbook
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the advantage of this?

@Lorak-mmk
Copy link
Collaborator

Lorak-mmk commented Feb 27, 2024

Regarding timings: comparing durations here vs the last CI run from other PR:

  • Authenticate: 2:12 vs 2:34
  • Book: 2:44 vs 4:20
  • Cassandra tests: 7:51 vs 8:50
  • Rust: 6:53 vs 10:33
  • SSL: 1:24 vs 2:37
  • min_rust: 0:39 vs 4:27
  • cargo_docs: 0:54 vs 0:30
  • Serverless: FAIL vs 5:25

It seems there are visible gains, so it would be nice to merge this PR.
I'd like to see some more timings, after rebasing on main, to make sure the differences are consistent.

@Lorak-mmk Lorak-mmk added the waiting-on-author Waiting for a response from issue/PR author label Mar 14, 2024
@wprzytula
Copy link
Collaborator

@oeb25 What's the status of this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-author Waiting for a response from issue/PR author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants