Skip to content

Refactor actions cache to be build on main #20144

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

mockersf
Copy link
Member

Objective

  • Cache is not reused across days as we produce too much cache and it gets cleaned by github

Solution

  • create cache from main
  • reuse cache in other jobs

@mockersf mockersf force-pushed the refactor-actions-cache branch from 3389f8b to f893744 Compare July 14, 2025 22:28
@alice-i-cecile alice-i-cecile added A-Build-System Related to build systems or continuous integration C-Performance A change motivated by improving speed, memory usage or compile times S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 14, 2025
Copy link
Contributor

@SkiFire13 SkiFire13 left a comment

Choose a reason for hiding this comment

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

LGTM, this looks like an improvement over the current status. There are a couple of other nits I would change, but I would prefer to discuss them after this PR is merged in order to not block it on secondary concerns.

Comment on lines 35 to 39
# key won't match, will rely on restore-keys
key: ${{ runner.os }}-stable--${{ hashFiles('**/Cargo.toml') }}-
restore-keys: |
${{ runner.os }}-stable--${{ hashFiles('**/Cargo.toml') }}-
${{ runner.os }}-stable--
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a nit: I would document that these have to follow the keys in update-caches.yml

Copy link
Member

@BD103 BD103 left a comment

Choose a reason for hiding this comment

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

Just one issue I found with update-caches.yml, the rest are nits :)

Comment on lines +130 to +131
path: |
~/.cargo/bin/
Copy link
Member

Choose a reason for hiding this comment

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

In the future, we should look into caching ~/.cargo/.crates.toml and ~/.cargo/.crates2.json, as they track which executables were installed with cargo install. Not something we need to do now, though, since we don't install anything with cargo install :)

@BD103 BD103 added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 15, 2025
mockersf and others added 6 commits July 15, 2025 20:02
Co-authored-by: BD103 <59022059+BD103@users.noreply.github.com>
Co-authored-by: BD103 <59022059+BD103@users.noreply.github.com>
Co-authored-by: BD103 <59022059+BD103@users.noreply.github.com>
@mockersf mockersf added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jul 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Build-System Related to build systems or continuous integration C-Performance A change motivated by improving speed, memory usage or compile times S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants