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

Use godot-cpp 4.1 for the "Godot CPP" CI workflow to prevent circular dependency #81238

Merged
merged 1 commit into from
Sep 2, 2023

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Sep 1, 2023

At the moment, we have an issue where any PR that updates any of the new structs added to gdextension_interface.h will fail CI on the "Godot CPP" workflow.

This is because godot-cpp on its master branch has already been updated to use the new structs, and so if we change them, we end up with a circular dependency: Godot's CI won't pass without an updated godot-cpp, and godot-cpp's CI won't pass without an updated Godot.

Once Godot 4.2 is release, we won't change these structs any more -- we'll have to start worrying about backward compatibility, and so even newer structs would need to be added in order to make changes. However, until then, we can update these structs - if it weren't for this CI issue :-)

There's a couple of options:

  1. We decide it's OK to merge these handful of affected PRs even though tests are failing. This isn't ideal, because until godot-cpp is updated, there is a (probably very short) window of time when other PRs that fork from master at the wrong time will fail too. Given how short this window of time would probably be, maybe that's OK?
  2. We ensure that godot-cpp's master branch doesn't update to use the new structs until after Godot 4.2 is released. I'm not crazy about this option, though, because we need time to work on the companion Godot 4.2 changes, and I don't want to have to save those PR's until Godot 4.2's release.
  3. We switch to using the Godot 4.1 version of godot-cpp. This doesn't have any of the downsides of the previous two options, and it's what this PR does, but it does mean we'll have to make some extra PRs around each Godot release to change the version of godot-cpp that's used in the tests. Personally, I think this is the smallest downside - we're making PRs to change stuff everyday, what's one more thing ;-)

But I'd like feedback on what other folks think is the best option, or if there's an option that I didn't consider!

@dsnopek dsnopek added this to the 4.x milestone Sep 1, 2023
@dsnopek dsnopek requested a review from a team as a code owner September 1, 2023 21:24
Copy link
Member

@rburing rburing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will work because of forward compatibility of the extension API, right? Makes sense to me.

@dsnopek
Copy link
Contributor Author

dsnopek commented Sep 1, 2023

This will work because of forward compatibility of the extension API, right?

Yep! Since gdextension_interface.h maintains source compatibility, and since using the older APIs will continue to work in newer versions of Godot, the 4.1 branch of godot-cpp should work just fine with Godot 4.2 (just without using any of the gdextension_interface.h features that are added in Godot 4.2).

@akien-mga
Copy link
Member

akien-mga commented Sep 2, 2023

I think that's an OK compromise for now.

The main question is: What do we want from the godot-cpp CI workflow on this repo?

The step was added in #54015 by @BastiaanOlij, with the aim to ensure that changes in upstream Godot do not break godot-cpp (and other extensions, at least as far as godot-cpp covers the extension API). But it's been mostly a hassle due to intentional breaking changes that had to be made in both repos.

What do we test by using the 4.1 branch, and what might we miss on? Since the current test copies the gdextension_interface.h from this repo to godot-cpp's, won't we still have issues even with godot-cpp's 4.1 branch?

Edit: Re-read the OP and I understand better, currently we get breakage when changing structs which are new in 4.2, but already used in godot-cpp master. Since we're not using those in godot-cpp 4.1 doing the check on the stable branch lets us make sure we don't break compatibility for stable users.

@akien-mga akien-mga merged commit d2ae309 into godotengine:master Sep 2, 2023
@akien-mga
Copy link
Member

Thanks!

@dsnopek
Copy link
Contributor Author

dsnopek commented Sep 2, 2023

The main question is: What do we want from the godot-cpp CI workflow on this repo?

Yeah, that's a really good question!

What do we test by using the 4.1 branch, and what might we miss on?

We're testing that the older compatibility bits in gdextension_interface.h haven't been broken. We're also testing that any changes to extension_api.json are compatible and won't break older GDExtensions.

We miss out on testing that changes to newer structs/functions don't break something, but that's impossible to distinguish from when we're changing those structs/functions on purpose. (Which is essentially the problem we're running into.)

Since the current test copies the gdextension_interface.h from this repo to godot-cpp's, won't we still have issues even with godot-cpp's 4.1 branch?

No, we shouldn't, because we also try to ensure that gdextension_interface.h is source compatible, so a GDExtension written against the gdextension_interface.h header from Godot 4.1 should still work with any future version of the header (unless we make a mistake or purposefully decide to break source compatibility).

The big difference here is that godot-cpp's 4.1 branch won't use any of the new stuff that was added to gdextension_interface.h after 4.1, so we can freely change the new stuff without any problems.

Once Godot 4.2 is released, though, we should definitely switch to testing the 4.2 branch of godot-cpp so that we don't break the new structs/functions from that point onward.

@YuriSizov
Copy link
Contributor

unless we make a mistake or purposefully decide to break source compatibility

And if we do decide that, won't it be a problem? We wouldn't be able to change 4.1/4.n-1 godot-cpp to allow those breaking changes, only 4.n would be compatible. Thus the CI would fail.

@dsnopek
Copy link
Contributor Author

dsnopek commented Sep 2, 2023

And if we do decide that, won't it be a problem?

I'm hoping we'll never break source compatibility again until Godot 5, but, yes, if we do decide to break source compatibility then it would be a problem.

@dsnopek dsnopek deleted the godot-cpp-version branch July 22, 2024 15:27
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.

4 participants