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: Use caching actions from godot-cpp #80

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

unvermuthet
Copy link
Contributor

@unvermuthet unvermuthet commented Mar 5, 2025

This PR switches out the existing cache step with the godot-cache-restore and godot-cache-save actions from godot-cpp. These actions have a different scheme for the cache keys. They create a new cache for every commit by appending - among other things - the commit SHA, but restore from any key that matches up until the unique identifier. Using this approach fixes the caches not being updatable.

A few notes:

  • GODOT_BASE_BRANCH doesn't seem to be set, so there is one empty part. Where is this set normally?
  • I dropped the "_cache" suffix because that's unnecessary

@Ivorforce
Copy link
Contributor

Ivorforce commented Mar 5, 2025

Do we need to switch to 2 separate actions to fix this issue? I thought it would be enough to give the cache action we currently use a key based on the current commit ID?

@unvermuthet
Copy link
Contributor Author

No. Giving the cache a unique key, requires fuzzy matching on the restore. Otherwise caches won't ever be used.

@Ivorforce
Copy link
Contributor

Ivorforce commented Mar 5, 2025

@unvermuthet
Copy link
Contributor Author

unvermuthet commented Mar 5, 2025

Oh yeah true. We could still use the "composite" actions/cache. That one takes the restore-keys too. But why duplicate the behavior when we can use the existing ones from godot-cpp?

Having a separate save action also enables you to save the cache before the "Delete Windows Compilation Files" step. Maybe that's useful. You could also enable the action to save the cache even if the build action fails by setting if: always()

@unvermuthet unvermuthet marked this pull request as ready for review March 5, 2025 10:20
@Ivorforce
Copy link
Contributor

Having a separate save action also enables you to save the cache before the "Delete Windows Compilation Files" step.

In our case, it's not so relevant, but it does become relevant if you're doing things after the main build.
I guess it wouldn't be bad if we used the godot-cpp caching logic so people could attach their unit testing steps after the cache save without needing to adjust the workflow further. As I'm gathering, separate save and restore steps are good practice right now.

@Ivorforce
Copy link
Contributor

Ivorforce commented Mar 5, 2025

I quickly looked through the two actions used for this PR (in godot-cpp).
I think they could be improved, there are a few awkward things about them. Perhaps we should do that before using them in the template workflow?

@unvermuthet
Copy link
Contributor Author

unvermuthet commented Mar 5, 2025

They are a lot more verbose than what I did in my project. That's the only thing that bothers me. Its a bit overkill but perhaps required for godot-cpp.

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.

2 participants