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: Remove godot-cpp workflow #98027

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Oct 9, 2024

godot_cpp_test.yml doesn't seem to have a purpose in this repo. All it does is build a godot-cpp test instance, without even running/validating anything afterward. This is ultimately redundant, with godot-cpp having CI for this process already, and cached at that. We also now validate gdextension_interface.h in static_checks.yml instead1, meaning the only niche it had before is now safely handled elsewhere. As such, I see no reason to keep it around anymore, and removed it in this PR. Consequently, the godot-api-dump action & references to it were also removed, as the godot-cpp workflow was their only use-case

If there's scenarios I'm not seeing/considering, please let me know. Though, given this removal speeds up total GHA time by a substantial 8-9 minutes, it'd be a pretty tough sell to keep this workflow around for any longer

Footnotes

  1. CI: Improve godot-cpp actions #97166

@Repiteo Repiteo added this to the 4.4 milestone Oct 9, 2024
@Repiteo Repiteo requested review from dsnopek and a team October 9, 2024 17:02
@Repiteo Repiteo requested a review from a team as a code owner October 9, 2024 17:02
@fire
Copy link
Member

fire commented Oct 9, 2024

is the case of new methods that conflict with existing godotcpp methods covered?

@Repiteo
Copy link
Contributor Author

Repiteo commented Oct 9, 2024

I'm not sure if that case was covered in the first place

@touilleMan
Copy link
Member

Hi,

From what I recall, this was added to the Godot repo during the development of 4.0, at that time the API was changing frequently which would silently break the bindings.

Typical scenario is as follow:

  1. User A opens a PR on the Godot repo that modifies function foo in the Godot repo (e.g. add a parameter to the function)
  2. Godot's CI runs fine, so Akien merges his 387th PR of the day ^^
  3. User B wants to update the version of Godot used on the Godot-CPP repo
  4. Godot-CPP's CI fails and complains the test project (compiled on the CI) doesn't build

So we would end with changes frequently added to the main Godot repo that would break the Godot-CPP repo, so somebody else would have to fix this :(

Again, this was a long time ago so I might miss some of the picture here.

Anyway, now that Godot 4.0 have landed, the API is supposed to be backward compatible so those frequent checks should be necessary anymore.

My only concern is, as @fire stated, how do we currently enforce this backward compatibility. Do we have some check in the CI on that (typically generating the GDExtension's extension_api.json and run a Python script on it to compare it against a previous version to ensure only backward compatible changes are done on it)

@AThousandShips
Copy link
Member

We have compatibility checks for method data like arguments and defaults, so that's covered, we should evaluate what kind of checks would be needed to reduce the risk of other issues like naming and collisions perhaps, we also have this PR which will catch some things that the godot-cpp run would otherwise catch:

We also already have checks for default arguments:

So maybe we can add some checks for reserved names in godot-cpp and similar

My concern is that CI is run relatively rarely in the godot-cpp repo as the pace of additions is much lower there, so errors could go unnoticed for weeks if we're unlucky, and could be hard to pin down, but as long as we can reduce the risk as much as possible by running additional CI checks for these things I think it's worth it to remove this build as it does take some time

One option would be to reduce the godot-cpp CI check to just building the library and not the test extension, that would reduce the runtime and lose relatively little critical things from the checks

@Repiteo
Copy link
Contributor Author

Repiteo commented Oct 10, 2024

I'm entirely open to adding specialized CI checks if need be; I'm just not convinced that was accomplished with the godot-cpp workflow

@dsnopek
Copy link
Contributor

dsnopek commented Oct 11, 2024

I'd prefer to keep some version of this in CI. We do have the checks of extension_api.json, but we don't have a way to check for bad edits to gdextension_interface.h (although, I suppose the folks who merge would ensure that the GDExtension team would have to approve any edits to that file?). I actually wish we additionally ran the godot-cpp tests here, because that would also detect breaking changes to the bits of Object and Variant that relate to GDExtension, which wouldn't be detected currently.

We don't run the godot-cpp tests all that often, really only when a new PR is created or a PR is merged. And it's also nice to detect breaks in any of these things on the Godot PR that breaks them, rather than later when the godot-cpp CI breaks, and we have to come back and fix someone else's work.

@Repiteo
Copy link
Contributor Author

Repiteo commented Oct 11, 2024

Hmm, then I suppose an alternative could be adjusting the tests to run only when needed; something similar to how certain pre-commit checks only occur when relevant files are touched. That, and integrating ThousandShip's suggestion of building the library by itself would speed things up. Throw in caching improvements (godotengine/godot-cpp#1611) & it very well could be viable to have tests on the main repo after all

Converting this to a draft while I brainstorm how best to implement this; will probably take some cues from #97742 as well.

@Repiteo Repiteo marked this pull request as draft October 11, 2024 16:16
@Repiteo Repiteo modified the milestones: 4.4, 4.x Oct 11, 2024
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.

5 participants