-
-
Notifications
You must be signed in to change notification settings - Fork 631
Update CI with new CMake builds. #1726
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
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
5c97ef8
to
3c7657a
Compare
I have asked in the rocketchat buildsystems channel for someone to review, and also about the shared resources. |
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.
Doubling the resources shouldn't be that bad, as it's comparatively uncommon for updates to occur on this repo. The way the actions are setup is a bit different from what I'm used to, but I also blame most of that on comparative ignorance with CMake. Assuming the CI passes, I have no issue with this being merged as-is. I can do a pass after the fact if I have any real nitpicks, but none immediately jump out at me and it'd be bikeshedding a PR that's been on the backburner for far too long
- 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
3c7657a
to
b6c0251
Compare
rebased onto master. |
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