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: Add a cache for cargo to speed up actions #322

Merged
merged 2 commits into from
Jul 6, 2022

Conversation

mjpieters
Copy link
Contributor

The cache step here caches the cargo output as per the recommended
settings
,
but with the cross-compilation target as part of the cache key, and a restore key
to take advantage of a partial cache should the cargo lock change only marginally.

Checklist

  • I picked the correct source and target branch.
  • I included a new entry to the CHANGELOG.md.
  • I checked cargo clippy and cargo fmt. The CI will fail otherwise anyway.
  • (If applicable) I added tests for this feature or adjusted existing tests.
  • (If applicable) I checked if anything in the wiki needs to be changed.

@mjpieters
Copy link
Contributor Author

While we are here: Is there any particular reason why you have left strategy.fail-fast set to true? If we were to set that to false then any issue with a specific platform won't stop the builds on other platforms, which would allow you to distinguish between an issue specific to one platform or another, and at the same time a toolchain issue for, say, iOS won't block you from detecting other issues that require a build to progress further.

@codecov-commenter
Copy link

codecov-commenter commented Jul 6, 2022

Codecov Report

Merging #322 (6246ab7) into main (de8080f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #322   +/-   ##
=======================================
  Coverage   53.36%   53.36%           
=======================================
  Files          70       70           
  Lines        4722     4722           
=======================================
  Hits         2520     2520           
  Misses       2202     2202           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de8080f...6246ab7. Read the comment docs.

@mjpieters
Copy link
Contributor Author

mjpieters commented Jul 6, 2022

And because I rebased this PR onto main after @Nukesor merged my other CI PR, we get a neat demo of how effective the cache is :-)

The initial test workflow run for this PR, the cargo build steps took up to 5 minutes, and after a force push the same workflow took 20 seconds in the best case, 1 min 40 in the worst. Total time went from 11 min 5 seconds to 5 min 47 seconds.

This is of course an extreme case as there are no code changes in this PR, but you should see similar speed-ups for regular PRs and commits.

@Nukesor
Copy link
Owner

Nukesor commented Jul 6, 2022

Haha :D

You're having a field day over here! Thanks for bringing the CI up to speed :)
Caching looks good. A good 50% time reduction between runs seems reasonable.

Regarding fail-fast, there's no specific reason why this isn't used yet.
To be honest, I remember looking this up when migrating from travis to github actions, but IIRC they didn't have this feature back than :D Maybe it got added in the meantime,

Feel free to add it to all action definitions! That would definitely be quite handy.

@mjpieters
Copy link
Contributor Author

To be clear: you have fail-fast now, because that's the default. It's why in this build, all the steps except Test on macos-latest for aarch64-apple-ios were cancelled; the ios build failed, and that triggered a fast fail path that cancelled the other matrix members.

Setting fail-fast to false would mean those other builds are permitted to continue. Like when the coverage upload for one of the builds fails (not always a fatal issue!), you'd still see if the other builds at least can complete.

I'll add the switch to this PR.

@mjpieters
Copy link
Contributor Author

Also: I don't think the linter steps need to be run across all targets. cargo fmt and cargo clippy output won't vary, will they? Or are parts of the codebase conditionally ignored based on platform?

@Nukesor
Copy link
Owner

Nukesor commented Jul 6, 2022

Also: I don't think the linter steps need to be run across all targets. cargo fmt and cargo clippy output won't vary, will they? Or are parts of the codebase conditionally ignored based on platform?

That's the case. At least clippy only looks at the pieces of code/features that're currently included and actually compiled.

I would really like to keep this in the CI, as I otherwise won't notice any linting issues in PRs, as I'm always on Linux.

@Nukesor
Copy link
Owner

Nukesor commented Jul 6, 2022

For now, clippy and fmt isn't actually executed on all platforms, but it'll be in the future, as soon as all features are implemented for the apple platforms. Right now, there're a few linting issues that can only be resolved by actually using that code.

We could of course add conditional linting exceptions (if that's possible), but I didn't find the motivation to do so yet.

@Nukesor Nukesor merged commit d98df3a into Nukesor:main Jul 6, 2022
@mjpieters
Copy link
Contributor Author

That's the case. At least clippy only looks at the pieces of code/features that're currently included and actually compiled.

Right you are! Then the matrix there is necessary and helpful.

For now, clippy and fmt isn't actually executed on all platforms, but it'll be in the future, as soon as all features are implemented for the apple platforms.

For what it's worth: cargo fmt --all -- --check and cargo clippy --tests --workspace -- -D warnings pass on my Macbook Pro (target=aarch64-apple-darwin`).

I haven't quite managed to cross-compile to aarch64-apple-ios or x86_64-apple-darwin yet, so I can't confirm if this applies to those as well. That's because the ring crate needs to cross-compile C++ code too, and that's not something I've done before. Not as big a challenge as cross-compilation from Mac to Linux, though.

However, I don't see why the codebase wouldn't pass on those platforms right now.

@mjpieters mjpieters deleted the ci_cargo_cache branch July 6, 2022 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants