-
Notifications
You must be signed in to change notification settings - Fork 112
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: add caching w/ actions/cache@v3 #172
Conversation
This commit adds a caching step to the `build-windows`, `build` and `coverage` jobs of the GitHub actions CI configuration. We key based on: 1. Runner OS 2. Rust toolchain 3. Hash of Cargo.lock
You are essentially doing the same as swatinem's action if you look at the details, just with more manual configuration 🤷♂️ https://github.com/Swatinem/rust-cache?tab=readme-ov-file#cache-details If you are worried about supply-chain attacks from third-party actions, you could always pin by hash and let dependabot upgrade the action when there is a new release. |
True, but there's something to be said for an explicit and minimal configuration. It's also a language agnostic solution which may make future work like #34 easier. I don't feel strongly in either case. If you and Djc prefer the swatinem approach and can convince Est31 I'm happy to close this branch. |
Wouldn't you want separate caches for different languages? The hardest bit is cache invalidation and mixing caches only makes that harder. |
Fair point, phrased alternatively: we can learn actions/cache and use it for both languages as opposed to using rust-cache for rust and a separate action for other languages 🤷 |
Yeah personally I prefer using more "official" dependencies even if it means we have a few more lines to add and maintain. In other words, IMO the line overhead this PR is adding is not worth adding a dependency from a third party. The hacking risk is one of the smaller issues, I'm more generally concerned about additional components that can break and that we only have a poor understanding of. If I see the config file, I know precisely what it is doing. Not the same if there is a dependency. |
~/.cargo/git/db/ | ||
target/ | ||
$VCPKG_DEFAULT_BINARY_CACHE | ||
key: ${{ runner.os }}-cargo-stable-${{ hashFiles('Cargo.lock') }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to only notice this now but you probably want a separate restore-key
here to use the cache even if Cargo.lock
changes otherwise every dependency bump will completely invalidate the cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I'm fine with invalidating the cache a little bit too often rather than too rarely. Also, Cargo.lock is ideally only touched very rarely. I.e. I don't want to set up dependabot or run cargo update
too regularly. It's just for testing and development purposes.
A possible alternative to #168. This commit adds a caching step to the
build-windows
,build
andcoverage
jobs of the GitHub actions CI configuration.We key based on:
This is largely 1:1 with the Cache action Rust Cargo docs, but for the Windows build we also include the
$VCPKG_DEFAULT_BINARY_CACHE
to cache the result ofvcpkg
installation.