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 cache action to speed up compilation #7

Merged
merged 6 commits into from
Apr 14, 2022

Conversation

neuromancer
Copy link
Contributor

No description provided.

@neuromancer neuromancer marked this pull request as ready for review April 13, 2022 16:42
@chkuendig
Copy link
Owner

chkuendig commented Apr 13, 2022

I think this is generally a great idea. Since I activated all engines a full build takes more then an hour.

I'm wondering though how this will work with the fact that we cache the full submodule which is often the reason for the update? Would the checkout part of the action require to be rewritten to perform a pull if the folder is cached?

Alternatively we might want to redirect the object files into a separate folder (with OBJDIR or -o, see https://stackoverflow.com/questions/5178125/how-to-place-object-files-in-separate-subdirectory) which is cached

For emsdk and the game/demo files this could be done manually as part of the build. (but that's only a few minutes of the build)

@neuromancer
Copy link
Contributor Author

I'm wondering though how this will work with the fact that we cache the full submodule which is often the reason for the update? Would the checkout part of the action require to be rewritten to perform a pull if the folder is cached?

Good point. I'm not sure how this interacts with the submodule. I guess we should try it and see if it works..

@chkuendig
Copy link
Owner

I approved your run, so I assume you can test this by adding commits to the PR?

@neuromancer
Copy link
Contributor Author

Exactly. I'm finishing my day here, if you feel you want to take care of this sooner than later (perhaps because you saw a better way to do it), feel free to close the PR and open a new one. Otherwise, I will experiment with this tomorrow.

@neuromancer
Copy link
Contributor Author

As expected, it failed in the deployment since it should only work for your own user. I temporary disabled the deployment so we can test the caching, but this needs manual approvals at each run from your side.

@chkuendig
Copy link
Owner

Wouldn't you be able to test this on https://github.com/neuromancer/scummvm-demo ? (you can activate github pages + actions on your own repo as well)

@neuromancer
Copy link
Contributor Author

I just enabled it, let's see what happens.

@neuromancer
Copy link
Contributor Author

The execution generated a 3GB cache. Let's see if this is faster than re-download and recompile everything..

@neuromancer
Copy link
Contributor Author

neuromancer commented Apr 14, 2022

Good news: this is working fine in my repository. It takes around 15 minutes instead of more than one hour. To really test this PR, I recommend to add more commits from scummvm master into the Emscripten PR to make sure the new changes are actually updated.

@chkuendig
Copy link
Owner

I'll merge this as it looks good. If something breaks we can always fix it later.

@chkuendig chkuendig merged commit 58d11ed into chkuendig:main Apr 14, 2022
@neuromancer neuromancer deleted the patch-1 branch April 15, 2022 17:39
@chkuendig
Copy link
Owner

FYI: This didn't work. None of the changes done in the scummvm repo are updated/pulled in.

@neuromancer
Copy link
Contributor Author

Uhm according to the CI tests, compilation is taking around 17minutes, which is very fast to recompile everything, however, it could be interfering with the git modules as you said. In my branch, I tried to use ccache, but emscripten requires a special fork of it, so finally I implemented a specific cache manually, take a look to this:

https://github.com/neuromancer/scummvm-demo/blob/main/.github/workflows/main.yml#L48-L65

I'm also rebasing automatically with scummvm master, but that's another topic of discussion.

@chkuendig
Copy link
Owner

chkuendig commented Apr 21, 2022 via email

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