CI: Extract godot-cpp testing into its own job #80091
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
YuriSizov
added
enhancement
topic:buildsystem
cherrypick:4.0
cherrypick:4.1
akien-mga
approved these changes
Aug 1, 2023
This ensures that the godot-cpp job has plenty of resources to run its build and avoid being affected by the main build. Additionally: - Extract test tasks into dedicated actions. - Upload artifacts as early as possible. - Ensure that we check master cache before random cache.
YuriSizov
force-pushed
the
ci-compartmentalization
branch
from
692e7c4
to
deb6025
Compare
|
Just some adjustments to comments. Also rebased for good measure, we can consider this a control run to make sure there are no regressions to the CI :) |
|
Thanks! |
|
Cherry-picked for 4.0.4. |
|
Cherry-picked for 4.1.2. Note that I didn't mark it for 3.x because I don't think it would be trivial, but a backport should be possible if needed. |
YuriSizov
removed
the
cherrypick:4.1
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements some of the changes discussed in #79919. I picked one for the title, as it's the most noticeable one, but it's a combination that's going to help us here.
1. Extract tasks to test
godot-cppinto their own workflow/job.This allows us to separate building Godot and building
godot-cppwith upstream changes applied. Technically, thegodot-cppdoesn't depend on any particular platform, it just needs us to dump the API and generate a header. So that's what we do in our Linux SAN build now, and put those files into artifacts.The separate, but dependent
godot-cpp-testjob then downloads those and does its own thing, same as before. While this doesn't address the disk space issue, with this change thegodot-cppbuild will no longer be capable of triggering it, since it gets its own run without any extra files present.godot-cppis currently the main source of CI failures, as it runs out of disk space.I also tried to add caching to it, but I failed to locate where scons stores its cache in
godot-cpp/testbuilds. Maybe there is no cache at all? Something that can be improve later.2. Ensure that we check for "master" cache before considering a random matching cache.
Our checks for existing cache have 3 steps: the exact match, a partial match (same base branch + same PR), and a random match for the same base branch. This means that any PR can pick cache from any other PR, which doesn't seem useful. It does sound actually harmful, as PRs can start sharing extra cache file that they don't need, never needed in the first place. This inevitably contributes to the cache bloat that is one of the sources of the problem.
It also can slow down builds, as instead of comparing to the base/target branch we may end up considering some cached results from a PR that affected different files — files which are not affected in either master or the current PR. So this can trigger rebuild for no good reason, on top of bloating the scons cache folder.
So I've added a new step before we fall back to just any cache: try to find an existing cache for the same base branch and the matching base branch git reference. This basically means that if you PR against
master, we'll first check for previous caches from your PR, then for any valid cache frommaster, and only then for a cache from any other PR againstmaster.3. Upload artifacts as early as possible.
This is not directly related to the issue, so I can revert this change. It was however requested by @QbieShay and others that we provide artifacts even if checks after builds fail. I wanted to use artifacts to run all tests in separate jobs, so I made this change, before eventually omitting the idea for technical reasons. But the change itself is kept as useful.
It does come with a caveat, though. We have to run strip and delete debug files on Windows before uploading the artifacts. This means that all tests run without them. This means that in case of a test crashing we won't be able to see a detailed stack trace, as far as I'm aware. So maybe it's not a great idea. It can reduce the disk space usage somewhat though.
I'm open to discussion and to change this either way.
There are also some cosmetic changes as I've moved more steps into reusable actions. Some because they are multi-step and it's easier to maintain this way. And some because they could be technically applied to other builds which are currently not doing the same checks.
Tagging @Faless for a review since he was responsible for at least some of the current CI implementation.
Before this PR finished building you can also see the results of all the changes here: https://github.com/YuriSizov/godot/actions/runs/5716722251