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

Fix and simplify CI caching #7627

Closed
wants to merge 1 commit into from

Conversation

TimJentzsch
Copy link
Contributor

@TimJentzsch TimJentzsch commented Feb 11, 2023

Objective

Currently, the CI cache is only renewed when the Cargo.toml files are updated. However, when a patch dependency is released which is compatible with the Cargo.toml, the CI cache will be outdated and the dependencies will need to be recompiled every time.

Additionally, our CI workflow definitions are quite verbose, because the cache paths need to be defined every time.

Solution

Use the Leafwing-Studios/cargo-cache action.
It generates the Cargo.lock file before determining the cache key, such that the exact dependencies can be used to keep the cache up-to-date and avoid recompiles. The cache falls back to similar previous caches to avoid completely starting from scratch.
This also reduces a lot of boilerplate in the CI definitions.

Additional Context

  • The cache is only updated when the key is not already available. This is why the Cargo.lock file should be part of the cache key, because it determines exactly which dependencies are used.
  • The Cargo.lock file is not committed to the repository, because this is not recommended for libraries. This is why the Rust toolchain action must be used before the cargo-cache action, because it uses the cargo update command to generate the Cargo.lock file.
  • For Leafwing-Studios/Emergence, this reduced our CI times from ~15 min to ~2 min.

Security Considerations

Adding a new third-party action increases the attack vector and should be considered carefully.
I recommend to audit the action first, it should be easy enough to understand (it's very similar to the current system we use) and I tried to add as many comments as possible.
I also published the action under the Leafwing org to get additional eyes on every PR (e.g. Alice).
If you'd prefer, we can also target an explicit commit of the action instead of a tag, to ensure that updates can be audited properly.

@TimJentzsch
Copy link
Contributor Author

As a small example, notice how in this CI run, there was a cache hit, but still some dependencies had to be downloaded and recompiled.

@rparrett
Copy link
Contributor

rparrett commented Feb 11, 2023

At a glance, doesn't this break caching of the cargo registry, cargo binaries, etc if cargo update is run before the cache is set up?

Downloading the full registry often takes a non-trivial amount of time.

The cache falls back to similar previous caches to avoid completely starting from scratch.

Is this behavior explained somewhere?

@TimJentzsch
Copy link
Contributor Author

TimJentzsch commented Feb 11, 2023

At a glance, doesn't this break caching of the cargo registry, cargo binaries, etc if cargo update is run before the cache is set up?

In theory yes, but in practice this doesn't seem to matter compared to the compile times that are saved. For example, in Emergence, the toolchain action takes 12s and the cache action with cargo update takes ~1m.
Probably not ideal, but the compile step then only takes 4s which is a huge win.
I also experimented with caching the binaries before the cargo update step, but that didn't seem to make a big difference.

The cache falls back to similar previous caches to avoid completely starting from scratch.

Is this behavior explained somewhere?

This is implemented via the restore-keys attribute of the cache action, see the caching docs.
It basically allows you to fall back of prefixes of cache keys.
We first fall back to the same Cargo.toml file when the Cargo.lock file is outdated and then to the same workflow job if the Cargo.toml file is outdated.
You can view it in the action definition here:
https://github.com/Leafwing-Studios/cargo-cache/blob/e9e0c6671e9ecbb4e8c516c2e1563a543eb7f2b4/action.yml#L61-L63

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Build-System Related to build systems or continuous integration C-Performance A change motivated by improving speed, memory usage or compile times labels Feb 11, 2023
@alice-i-cecile
Copy link
Member

This has been working great for us in practice, but I'll abstain from reviewing this myself.

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-Bug An unexpected or incorrect behavior C-Performance A change motivated by improving speed, memory usage or compile times
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants