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: Improve godot-cpp actions #97166

Merged
merged 1 commit into from
Sep 19, 2024
Merged

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Sep 18, 2024

Improves the GitHub Actions for godot-cpp in two primary ways:

  • Relocated running C compiler on gdextension_interface.h to static_checks. This check was pushed waaaaay further back than it should've been, especially when the check itself takes less than a second. It's structurally and logistically much closer to the other static checks, which is where the action now resides.
  • godot-cpp is now cached. There's no real reason for this SCons build to not have dedicated caching like the other workflows, so this adds exactly that. While this file would likely have to be retired entirely if other artifacts want to use godot-cpp, the current implementation means that the cache name can be safely hardcoded.

Comment on lines +53 to +57
env: # Keep synced with godot-build.
SCONS_CACHE: ${{ github.workspace }}/.scons-cache/
SCONS_CACHE_LIMIT: 7168
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't really affect anything but the godot-cpp build system doesn't have any cache limiting, so we'd have to see how this can be handled safely, but we do use the cache in godot-cpp so it shouldn't matter but just need to make sure it doesn't tax our CI too much by filling up our limits

Copy link
Member

@AThousandShips AThousandShips Sep 19, 2024

Choose a reason for hiding this comment

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

Note that just from this PR the cache upload size was 825 mb for a single run, that's a lot for the 10 gig cache limit, and might cause important other cache entries to be dropped and hurt general compile times

I think that, until we've implemented a cache limiting system in godot-cpp we should avoid using the cache here, especially since the build time is just 8 minutes at least in this build, compared to if we evict a linux build which is 30+ minutes without cache

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, fair point; haven't considered any cache limiting techniques used here. For now, I'll keep those sections commented out and add a TODO note.

Copy link
Member

Choose a reason for hiding this comment

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

I'll take a look at adding cache pruning to godot-cpp over the next few days and we can see how it works out

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!

@akien-mga akien-mga merged commit 6a1ab24 into godotengine:master Sep 19, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

@Repiteo Repiteo deleted the ci/godot-cpp branch September 19, 2024 15:41
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.

4 participants