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

Use Leafwing-Studios/cargo-cache in CI #13040

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

Conversation

BD103
Copy link
Member

@BD103 BD103 commented Apr 20, 2024

Objective

  • Our CI caches the ~/.cargo and target folders, to take advantage of faster, incremental, compilation.
  • The use of caches is quite inconsistent, with different formats for keys. It also does not take advantage of cache fallback (using an old cache if the current one does not exist).
  • Caches are being created in the merge queue, but can never be used.
    • To verify this, go to any validation job and check the actions/cache@v4 step. It will always say Cache not found for input keys: ....

Solution

  • Switch to using Leafwing-Studios/cargo-cache, which automatically handles cache keys, fallback, dependency updates, and toolchain updates.
  • Caches are no longer used in the merge queue.
    • Github Actions has something called cache isolation. Long story short, caches cannot be used if they were not created from the same branch or base branch.
    • For instance, a cache created on main can be used by any feature branch, but a cache created by feature-a branch cannot be used by the unrelated feature-b branch.

Changelog

@BD103 BD103 added C-Enhancement A new feature A-Build-System Related to build systems or continuous integration C-Code-Quality A section of code that is hard to understand or change labels Apr 20, 2024
Add a concurrency group, disable fail-fast, and install dependencies on Linux.
@TimJentzsch
Copy link
Contributor

You might be interested in this: https://github.com/Leafwing-Studios/cargo-cache

It uses a the hash of the updated lock file instead of the current date to calculate the hash key, also with several levels of fallbacks.

Honestly, debugging caching actions and figuring out why cargo keeps recompiling dependencies is super annoying, I hope that you'll get it to work!

@BD103
Copy link
Member Author

BD103 commented Apr 20, 2024

You might be interested in this: https://github.com/Leafwing-Studios/cargo-cache

Thank you! I'll definitely take inspiration from that. I chose not to hash the lockfile because I figured that the amount of dependency change in 1 day cannot be so great that it would hurt performance, but that assumption will definitely require testing. It's good to see an alternate approach, though!

Actually export cache keys to Github output, only use a cache when not in a merge group, customize Rust installation for WASM, and use proper command for docs.
Also split it out into a separate job.
This should hopefully speed up the doc job by ~3 minutes. Since `cargo test` needs to build the entire workspace but `cargo doc` only needs to check it, `cargo doc` can take advantage of the work that `cargo test` has already done.
A few other driveby fixes too. I removed the unecessary `components: rustfmt, clippy` specification. I also made the doc job use nightly Rust.
This bug caused the cache to never be restored.
This is a just in case, and likely will never become a problem. Github's cache isolation policy prevents caches created in merge queues from being used by any other branch.
@BD103
Copy link
Member Author

BD103 commented Apr 20, 2024

So far things are working well. I'm having some issues with the cache not being found, so I may have to move my testing over to my fork, where I can actually delete caches and cancel runs.

Building on MacOS and docs are the two longest jobs. I'm going to see if I can shorten their duration, if possible.

This may prove easier to setup and maintain than the homespun implementation used earlier.
@BD103 BD103 changed the title Redo CI caching Use Leafwing-Studios/cargo-cache in CI Apr 22, 2024
@BD103
Copy link
Member Author

BD103 commented Apr 22, 2024

Following @TimJentzsch's interesting link, I've decided to switch this PR to use Leafwing-Studios/cargo-cache for everything. I think it's a lot more maintainable than my custom solution (and it's easy to use, too!).

Performance seems roughly the same, except for run-examples-macos-metal which didn't use caching before. It's hard to tell, since Github has two different MacOS runners with 3 and 4 CPU cores.

@BD103 BD103 removed the C-Enhancement A new feature label Apr 22, 2024
@BD103 BD103 marked this pull request as ready for review April 22, 2024 02:39
@TimJentzsch
Copy link
Contributor

Previously attempted in #7627, but the action has also improved since then

@BD103
Copy link
Member Author

BD103 commented Apr 24, 2024

Previously attempted in #7627, but the action has also improved since then

Is there a reason the original PR was closed?

@BD103 BD103 added D-Straightforward Simple bug fixes and API improvements, docs, test and examples and removed D-Trivial Nice and easy! A great choice to get started with Bevy labels May 17, 2024
@janhohenheim janhohenheim 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-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 25, 2024
@BD103
Copy link
Member Author

BD103 commented Jun 25, 2024

If / when this gets merged, it would be good to wipe all of the existing caches here. This will make the migration a little quicker, and hopefully show the performance improvements sooner.

@alice-i-cecile alice-i-cecile added S-Needs-SME Decision or review from an SME is required X-Controversial There is active debate or serious implications around merging this PR and removed X-Contentious There are nontrivial implications that should be thought through labels Jul 8, 2024
@alice-i-cecile
Copy link
Member

Swapping to Controversial; I want to get Francois's sign-off here.

@janhohenheim
Copy link
Member

Update to the newly released v2 before merging.

@BD103 BD103 added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Jul 17, 2024
@BD103 BD103 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 17, 2024
@TimJentzsch
Copy link
Contributor

TimJentzsch commented Jul 20, 2024

I think I would prefer an approach where we create cache only from the main branch, those should be available to every job

We can now do this via the save-if option, like this:

- uses: Leafwing-Studios/cargo-cache@v2
  with:
    save-if: ${{ github.ref == format('refs/heads/{0}', github.event.repository.default_branch) }}

@BD103
Copy link
Member Author

BD103 commented Jul 20, 2024

Done!

@mockersf, could you please review this? You're the last step before this can be merged :)

@mockersf
Copy link
Member

@mockersf, could you please review this? You're the last step before this can be merged :)

I was waiting for my previous review to be addressed! Now unless there's an urgent need I'll do it after the Jam 🙂

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-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-SME Decision or review from an SME is required S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants