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

Added the caching implementation #113

Merged
merged 8 commits into from
Jun 26, 2023
Merged

Added the caching implementation #113

merged 8 commits into from
Jun 26, 2023

Conversation

Marko19907
Copy link
Contributor

Added template and executable caching to speed up builds. Caching is done using @actions/cache.

It is enabled by default. To disable it, set the cache input to false as shown in the readme. From my testing, caching the Godot executable and export templates reduces the build time from around ~2 minutes to about a minute depending on the runner.

Copy link
Owner

@firebelley firebelley left a comment

Choose a reason for hiding this comment

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

Hello! This looks good overall. Can you please run yarn build and commit the output index.js?

@Marko19907
Copy link
Contributor Author

Hello! This looks good overall. Can you please run yarn build and commit the output index.js?

Done!

@firebelley
Copy link
Owner

Thanks! Unfortunately this seems to have ballooned the index file quite a bit. How necessary is the caching? How does it compare in terms of speed to supplying GitHub download urls in the workflow setup? Tuxfamily downloads are slow ish, but downloading binaries from the Godot GitHub is much faster. I wonder if that's a better workaround?

This feature would be great to have, but I just don't know if it's desirable to have the workflow file balloon like it has. Do you have any thoughts?

@Marko19907
Copy link
Contributor Author

From the tests I ran, downloading from GitHub shows the same improvement in workflow runtime as caching.

Here's a comparison of the workflow runtime with caching on and off, using downloads from tux:
tux cache

And here's a comparison using downloads from GitHub:
github cache

The bundle size has definitely exploded. I tried to reduce it with rollup but that didn't work. There seems to be an issue with tree shaking in that package, as discussed here actions/toolkit#1436. I also noticed that the @actions/cache package is pulling in the Azure SDK dependency, which is not even being used.

@Marko19907 Marko19907 changed the title Added the caching implementation, closes #112 Added the caching implementation Jun 16, 2023
@moorbren
Copy link
Contributor

moorbren commented Jun 18, 2023

I say merge it in anyways, the bundle size is negligible compared to the templates being downloaded. An extra MB or two is totally worth cutting build times by 1 minute.

@moorbren
Copy link
Contributor

Been working on my builds recently, you think about caching the unzips as well @Marko19907? That's another reasonably hefty chunk of my pipeline, but worried it might be tricky with the Github caching.

I take it you guys are using the cloud runners? I'm using a self-hosted one ATM, which has it's own issues, but caching files locally isn't one of them

@Marko19907
Copy link
Contributor Author

I agree with @moorbren that it’s worth increasing the size of the index.js in the workflow. The benefits of cutting build times by 1 minute outweigh the negligible increase in bundle size. We could also wait for the tree shaking issues to be resolved upstream in the @actions/cache package.

As for caching the unzips, I think it’s definitely worth considering. I’ve noticed that it takes quite some time even on the runners managed by GitHub. It might be tricky to implement with GitHub caching and the current approach is definitely simpler.

@moorbren
Copy link
Contributor

@Marko19907 I might work on caching for the unzips if I have some time this weekend/next, big fan of fast builds.

How'd you test the Workflow without having to update the package though? Dug around a bit in the Custom action documentation, but couldn't find out how to test the code with my runner before I submitted a merge.

@Marko19907
Copy link
Contributor Author

Sorry for the delayed response, I'm currently on holiday. To test the Workflow, I created a private repository with a Godot project and placed the index.js file (result of the build) in the /dist folder. I also added an action.yml file in the root directory. This allowed me to test the Workflow by pushing the new index.js file. Hope this helps!

@moorbren
Copy link
Contributor

@Marko19907 Thanks! I'll give it a shot sometime this week, wanted to add a timeout for the builds as well. Had some issues with the imported files that was causing the Godot build to hang perpetually.

@firebelley Think this is good to merge in though, thoughts?

@firebelley
Copy link
Owner

Sounds good, yep I will go ahead and merge this. Thanks for the contribution!

@firebelley firebelley merged commit 48ea6c4 into firebelley:develop Jun 26, 2023
firebelley pushed a commit that referenced this pull request Jun 26, 2023
* Updated the .gitignore to exclude Webstorm files and the yarn-error.log

* Added the @actions/cache package

* Added caching with @actions/cache

* Added a CACHE_ACTIVE constant to prepare for the optional input

* Added the cache input

* Added the cache info to the README.md

* Code cleanup

* Chore: build and update index.js
firebelley added a commit that referenced this pull request Jun 26, 2023
* Added the caching implementation (#113)

* Updated the .gitignore to exclude Webstorm files and the yarn-error.log

* Added the @actions/cache package

* Added caching with @actions/cache

* Added a CACHE_ACTIVE constant to prepare for the optional input

* Added the cache input

* Added the cache info to the README.md

* Code cleanup

* Chore: build and update index.js

* update readme

---------

Co-authored-by: Marko <[email protected]>
Co-authored-by: Firebelley <[email protected]>
@firebelley
Copy link
Owner

Just another note - I made the cache flag value false by default both for consistency with the other flags but also because I think it's more appropriate for cache behavior to be opt-in, as caching can be the result of some unpredictable behavior sometimes. We can potentially change that default in a future version but just wanted to let you know in case you wonder why the caching isn't working on the latest release!

Thanks again for the contribution!

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