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: Update mymindstorm/setup-emsdk to v14, should fix cache folder conflicts #87575

Merged
merged 1 commit into from
Jan 25, 2024

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga added enhancement topic:buildsystem cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Jan 25, 2024
@akien-mga akien-mga added this to the 4.3 milestone Jan 25, 2024
@akien-mga akien-mga requested a review from a team as a code owner January 25, 2024 11:14
@akien-mga akien-mga changed the title CI: Update mymindstorm/setup-emsdk to v14, should fix cache folder conflicts CI: Update mymindstorm/setup-emsdk to v14, should fix cache folder conflicts Jan 25, 2024
@YuriSizov
Copy link
Contributor

Oh, that's awesome!

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

So this doesn't help. They simply added the workflow name as a prefix to the old naming scheme. But our CI has one entry point workflow. So both builds still have the same name for the cache:

🔗 GHA-3.1.39-linux-x64

The second-to-run build still throws an error, but for some reason it's just not caught as a problem anymore:

Failed to save: Unable to reserve cache with key 🔗 GHA-3.1.39-linux-x64, another job may be creating this cache. More details: Cache already exists. Scope: refs/pull/87575/merge, Key: 🔗 GHA-3.1.39-linux-x64, Version: bb287b8d239dae54b2b69d026bd1284e06aff2037346bb5491e7f80dd3ba6b57

So it seems we still need to do something manually to resolve this. Seems like they've added cache-key to the configuration options. We should populate it with something fitting. See here.

with:
version: ${{env.EM_VERSION}}
actions-cache-folder: ${{env.EM_CACHE_FOLDER}}
cache-key: ${{ matrix.cache-name }}-emsdk
Copy link
Contributor

@YuriSizov YuriSizov Jan 25, 2024

Choose a reason for hiding this comment

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

Oops, I forgot this variable only contains a prefix, and we actually add more to it when creating the actual cache name. So this works, but it's still not unique enough for multiple runs:

image

This should match our main cache naming scheme.

Suggested change
cache-key: ${{ matrix.cache-name }}-emsdk
cache-key: ${{ matrix.cache-name }}-${{env.GODOT_BASE_BRANCH}}-${{github.ref}}-${{github.sha}}-emsdk

Sorry!

Copy link
Member Author

@akien-mga akien-mga Jan 25, 2024

Choose a reason for hiding this comment

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

Thanks, changed.

Overall, I wonder why caching is so convoluted. What we'd want typically is to just cache based on the Emscripten version, and that's it. But somehow it keeps trying to recreate the cache each time it's using it, instead of just using the version that's already there?

IMO, if the cache worked as I think it should, we'd download Emscripten 3.1.39 once, and never again. All builds on all Godot branches would pull from the same cache and never need to write to it concurrently, unless we happen to bump the Emscripten version to all branches at once.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm should the -emsdk part come earlier in the cache key? IIRC substrings are removed from right to left to find the closest cache hit. It should maybe be emsdk-${{ matrix.cache-name }}-${{env.GODOT_BASE_BRANCH}}-${{github.ref}}-${{github.sha}} instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good point. I guess if it's added at the end, we can accidentally download it when trying to find cache for scons itself.

It's not that substrings are removed, we do it manually with restore-keys (and setup-emsdk doesn't, AFAICT). But we do indeed risk a collision because the first part is the same. So yeah, should probably add it somewhere earlier, or at the very start. Good catch!

Copy link
Contributor

Choose a reason for hiding this comment

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

Overall, I wonder why caching is so convoluted. What we'd want typically is to just cache based on the Emscripten version, and that's it. But somehow it keeps trying to recreate the cache each time it's using it, instead of just using the version that's already there?

IMO, if the cache worked as I think it should, we'd download Emscripten 3.1.39 once, and never again. All builds on all Godot branches would pull from the same cache and never need to write to it concurrently, unless we happen to bump the Emscripten version to all branches at once.

Yes, something feels wrong about it, but I'll need to look closer into it. I also noticed that before trying to restore the cache it looks for a pre-existing folder with it, as a manual step. This suggests we may be able to cache it manually however we want. But if the override attempt happens in actions/cache, then I'm not sure how to counteract that.

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

LGTM!

@YuriSizov YuriSizov merged commit 8d5c4b3 into godotengine:master Jan 25, 2024
16 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@akien-mga akien-mga deleted the ci-emsdk-14 branch January 30, 2024 13:24
@akien-mga
Copy link
Member Author

Cherry-picked for 3.6.
Cherry-picked for 3.5.4.

@akien-mga akien-mga removed cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release labels Jan 30, 2024
@akien-mga
Copy link
Member Author

Cherry-picked for 4.2.2.
Cherry-picked for 4.1.4.

@akien-mga akien-mga removed cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Mar 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants