search

firebelley / godot-export

Automatically exports your Godot games.
MIT License
460 stars 53 forks source link

Added the caching implementation #113

Closed Marko19907 closed 1 year ago

Marko19907 commented 1 year ago

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.

Marko19907 commented 1 year ago

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

Done!

firebelley commented 1 year ago

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 commented 1 year ago

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 https://github.com/actions/toolkit/issues/1436. I also noticed that the @actions/cache package is pulling in the Azure SDK dependency, which is not even being used.

moorbren commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

firebelley commented 1 year ago

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!