-
-
Notifications
You must be signed in to change notification settings - Fork 620
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
Update CI with new CMake builds. #1726
base: master
Are you sure you want to change the base?
Conversation
98551bf
to
502b2b5
Compare
Boy oh boy do I dislike the worlflow of troubleshooting github actions. Anyway I couldn't have done this in a single day without the pre-existing work of @IvanInventor's #1355 and #1452 The only remaining configuration that is not at least compiling is the iOS one, and I know how to solve it, might need @Torets #1650 rebased and updated to make it work. Or I'll have to ditch the way I generate the godot-cpp targets altogether @Ivorforce You have some experience with macos, ,do you have any with iOS? |
Thanks! I'm not super knowledgeable with regard to GitHub Actions, so we'll probably need to get review from others who are (like @Repiteo) Also, we need to decide if it's OK to basically double the number of CI builds we're doing. I'm not sure if I recall correctly, but I vaguely remember discussions in the past about us sharing resources with the main Godot project, and not wanting clog up the CI? Even if that is the case, we trigger way fewer builds than Godot, so it could be fine. But something to discuss if this is actually a thing :-) |
There are a lot of variations, perhaps have one target for regular commit CI and then some pre-merge workflows that do a more comprehensive check. |
e7fc87e
to
07979e6
Compare
I have a working iOS build in another branch: https://github.com/enetheru/godot-cpp/actions/runs/13743880227/job/38436397352 I have to wait and see if the single target is merged before I update these to match. |
07979e6
to
adc4269
Compare
30c2110
to
958d0d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
The changes seem OK to me. However, I think we still need to answer two questions:
- Is it OK to double the number of jobs we have? I vaguely recall something about us sharing CI resources with the main Godot project and not wanted to use too much, but I don't actually know if that's actually true. @Repiteo do you know if this is a thing?
- Does this look OK to GitHub Actions experts? I'm a novice with GitHub Actions, so it'd be great if someone who is more experienced could review. Maybe also @Repiteo?
@dsnopek I've added this topic to the meeting as I think there is opportunity to optimise both the number of builds and the time spent on each build. I think the discussion would be worth having. Currently we build all generated sources(~1000) for every platform, but the test project only needs a small subset and using the build_profile adds another test case. Perhaps a full build is not necessary for all things? I've setup the cmake builds to use the build profile because we are already doing a full build using scons, seemed a waste of time to do it for both since the build logic is all that is being tested. |
958d0d2
to
5c97ef8
Compare
- Duplicated the ci.yml into ci-scons.yml and ci-cmake.yml - Replaced some manual shell commands to detect changes with tj-actions/changed-files@v45 - Conditionally run the CI depending on what files change. - Add ios toolchain from leetal/ios-cmake - Added a cmake-minimum of 3.10 to silence errors from ios toolchain
5c97ef8
to
3c7657a
Compare
This has been rebased onto #1730
Replaced some manual shell commands to detect changes with tj-actions/changed-files@v45
Duplicated the ci.yml into ci-scons.yml and ci-cmake.yml
conditionally run the CI depending on what files change.
This is still a WIP