Skip to content

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

Merged
merged 1 commit into from
Apr 5, 2025

Conversation

enetheru
Copy link
Collaborator

@enetheru enetheru commented Mar 4, 2025

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

@enetheru enetheru force-pushed the continuous_integration branch 8 times, most recently from 98551bf to 502b2b5 Compare March 4, 2025 11:51
@enetheru
Copy link
Collaborator Author

enetheru commented Mar 4, 2025

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?

@Torets
Copy link

Torets commented Mar 4, 2025

... and I know how to solve it, might need @Torets #1650 rebased and updated to make it work.

rebased it, should be ok now

@dsnopek
Copy link
Collaborator

dsnopek commented Mar 4, 2025

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 :-)

@enetheru
Copy link
Collaborator Author

enetheru commented Mar 4, 2025

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.

@enetheru enetheru changed the title CMake: Update CI Update CI with new CMake builds. Mar 6, 2025
@enetheru enetheru force-pushed the continuous_integration branch 4 times, most recently from e7fc87e to 07979e6 Compare March 7, 2025 07:41
@enetheru
Copy link
Collaborator Author

enetheru commented Mar 9, 2025

I have a working iOS build in another branch: https://github.com/enetheru/godot-cpp/actions/runs/13743880227/job/38436397352
But that relies on #1733

I have to wait and see if the single target is merged before I update these to match.

@enetheru enetheru added the topic:buildsystem Related to the buildsystem or CI setup label Mar 10, 2025
@enetheru enetheru force-pushed the continuous_integration branch from 07979e6 to adc4269 Compare March 11, 2025 02:31
@enetheru enetheru marked this pull request as ready for review March 11, 2025 02:31
@enetheru enetheru requested review from a team as code owners March 11, 2025 02:31
@enetheru enetheru force-pushed the continuous_integration branch 2 times, most recently from 30c2110 to 958d0d2 Compare March 11, 2025 22:46
Copy link
Collaborator

@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.

Thanks!

The changes seem OK to me. However, I think we still need to answer two questions:

  1. 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?
  2. 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?

@enetheru
Copy link
Collaborator Author

@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.

@enetheru enetheru force-pushed the continuous_integration branch from 958d0d2 to 5c97ef8 Compare March 14, 2025 03:00
@enetheru enetheru force-pushed the continuous_integration branch from 5c97ef8 to 3c7657a Compare March 14, 2025 22:39
@enetheru
Copy link
Collaborator Author

I have asked in the rocketchat buildsystems channel for someone to review, and also about the shared resources.

@Repiteo Repiteo self-requested a review April 5, 2025 01:00
Copy link
Contributor

@Repiteo Repiteo left a 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
@enetheru enetheru force-pushed the continuous_integration branch from 3c7657a to b6c0251 Compare April 5, 2025 03:25
@enetheru
Copy link
Collaborator Author

enetheru commented Apr 5, 2025

rebased onto master.

@dsnopek dsnopek merged commit 7888c15 into godotengine:master Apr 5, 2025
18 checks passed
@enetheru enetheru deleted the continuous_integration branch April 16, 2025 00:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick:4.4 topic:buildsystem Related to the buildsystem or CI setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants