Skip to content

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
Member

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
Member

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
Member

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
Member

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.

Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Copy link
Member

@Ivorforce Ivorforce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good if it works!

@paddy-exe paddy-exe merged commit db25e65 into godotengine:main Apr 2, 2025
23 checks passed
@paddy-exe
Copy link
Collaborator

Merged! Thank you!

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