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

Add caching to CI #168

Closed
wants to merge 1 commit into from
Closed

Conversation

thomaseizinger
Copy link
Contributor

botan-sys is quite a heavy dependency to build. To accelerate CI times, add some caching of build artifacts.

@est31
Copy link
Member

est31 commented Oct 3, 2023

I wonder, is it possible to do caching without using dependencies? From what I can tell, you can do it with just 1-2 lines as well. The issue about dependencies is that you have to upgrade them and they also sometimes can become unmaintained.

@djc
Copy link
Member

djc commented Oct 3, 2023

The Swatinem action has been around for a long time and is widely used (IIRC it's already in use in some of the other rustls repositories?). For updates you can enable Dependabot for the Actions your workflows depend on.

The actions-rs stuff was painful, but I feel like caching ends up complicated enough that taking a dependency for this is probably the right solution.

@cpu
Copy link
Member

cpu commented Oct 3, 2023

(IIRC it's already in use in some of the other rustls repositories?)

Hmm, I don't think so? I grepped across my local checkouts and didn't come up with any YAML hits. It looks like Quinn, H3 and Reqwest are, but nothing in the Rustls org.

I had been playing around with caching in an old CI cleanup branch using actions/cache@v3 based on the upstream docs for Cargo projects. I think I paused on that because it was hard to demonstrate speedup when the main branch hadn't populated the cache based on how GitHub shared caches between forks/branches (IIRC).

Would using the 1st party Cache action dependency be more palatable to you @est31 ?

@cpu
Copy link
Member

cpu commented Oct 3, 2023

For updates you can enable Dependabot for the Actions your workflows depend on.

FWIW that's already in place.

@est31
Copy link
Member

est31 commented Oct 3, 2023

I had been playing around with caching in an old CI cleanup branch using actions/cache@v3 based on the upstream docs for Cargo projects.

Mhh that looks pretty good. Of course, if a pull request is not sped up by it while using Swatinem's crate speeds up PR builds, then we should use latter, but generally I'd prefer using something from github instead.

@cpu
Copy link
Member

cpu commented Oct 3, 2023

Of course, if a pull request is not sped up by it while using Swatinem's crate speeds up PR builds, then we should use latter,

I'll have to page the context back in, but I believe the issue wasn't the action being used, but the fact that the main convergence branch cache isn't populated until we merge one of the approaches. Afterwards (based on my reading of the docs) it will be available to other branches.

@est31
Copy link
Member

est31 commented Oct 3, 2023

Okay @cpu let's go with your branch then, try it out on main, and if it has problems, then we can always come back to this PR. It is a little bit noisier but is lower in terms of supply chain risk, which I like.

@cpu
Copy link
Member

cpu commented Oct 3, 2023

Try it out on main, and if it has problems, then we can always come back to this PR

PTAL: #172

Just to be explicit; I agree strongly that speeding up CI is a worthwhile direction no matter which approach we end up taking. Thanks for getting the ball rolling @thomaseizinger

@thomaseizinger
Copy link
Contributor Author

I wonder, is it possible to do caching without using dependencies? From what I can tell, you can do it with just 1-2 lines as well. The issue about dependencies is that you have to upgrade them and they also sometimes can become unmaintained.

FWIW, I have very good experiences with this action. We use it extensively in quite a big workspace here: https://github.com/libp2p/rust-libp2p. The setup is quite elaborate in that we only write to caches on master: https://github.com/libp2p/rust-libp2p/blob/master/.github/workflows/cache-factory.yml and then use them in PRs: https://github.com/libp2p/rust-libp2p/blob/399eaa39156f756956f38fab20083293da826fdd/.github/workflows/ci.yml#L44

I've also add interactions with the maintainer and they were very responsive :)

I don't mind which way you go, personally, I just prefer not to maintain these configs if I can do it with a one-liner. For example, the action also automatically cleans your target directory of workspace artifacts which are usually useless.

@est31
Copy link
Member

est31 commented Oct 3, 2023

Closing in favour of #172. As I've said above, if the approach chosen by #172 turns out to cause major issues or limitations, we can always reconsider this one, but for now i'd like to go with #172. @thomaseizinger thanks for getting the ball rolling!

@est31 est31 closed this Oct 3, 2023
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.

4 participants