CI: Check for GDExtension API compatibility breakage

[RedworkDE]

CI integration for #76446

Compares the output of the validation command against a provided reference and creates Error / Warning annotations if they mismatch.

The API can be validated against any number of references, but they must be tagged versions, with a corresponding tag in godot-cpp.

CI is will not fail because of this in any case, but things are setup so that it can easily be made to fail if any new errors appear.

[dsnopek]

Thanks, this is great!

Alternative: Don't fail the build when the check fails and just annotate the issue: As seen with the spellcheck, this will probably just get overlooked and breaks happen unintentionally.

I think it could make sense to have this PR just be an "advisory" like with spellcheck for now, and then make it an actual failure after it's been running for a bit, and we're closer to 4.1's release

[RedworkDE]

This now uses a previous editor binary instead of a dedicated artifact; The download a useful artifact for regession/compat checking is now its own action; if the PR has the breaks compat label this check will no longer fail the CI; and I added a bit of a message for when the check fails (exact wording is TDB)

I think it could make sense to have this PR just be an "advisory" like with spellcheck for now, and then make it an actual failure after it's been running for a bit, and we're closer to 4.1's release

IMO that is just not visible enough unless I make it comment on the PR directly and that is pretty annoying from a pull request action. Also with the breaks compat label support it should no longer be necessary to merge PR with a failing CI.

---

Test PR with the various possible results: RedworkDE#1 (Note that the CI ran twice for each commit, once in the context of the PR and once in the context of a branch; the last commit was pushed after the break compat label was removed again, even to the UI says otherwise) The results (both branch and PR run) are as intended by me.

[dsnopek]

Discussed at the GDExtension meeting:

• We think it makes sense for this to start as a warning, because many existing PRs would immediately fail this test. After we've had some time to adjust to the new compatibility API (and allow the community to learn how to use it), we can change it to an error.
• There is still the code in the yaml interfacing with the GitHub API via gh api - this should be replaced with an action or script as discussed above

We'd also like @akien-mga to review the GitHub Actions yaml changes, since he maintains the CI.

[RedworkDE]

* We think it makes sense for this to start as a warning, because many existing PRs would immediately fail this test. After we've had some time to adjust to the new compatibility API (and allow the community to learn how to use it), we can change it to an error.

Still don't think that is is a good idea, esp since I can't actually put the annotation at a useful location (I do no know which change actually causes the break so I just put it on README.md (not actually pushed to this branch yet))

* There is still the code in the yaml interfacing with the GitHub API via `gh api` - this should be replaced with an action or script as discussed above

Still what is the actual issue with using that command?
While I could translate the code to github script, it won't really simplify the interactions with the GH Api and instead make interacting with the git commands a pain.
And as mentioned above there is (as far as i can tell) no third-party action that can be used instead. (It has already been moved to a local action as I intended above)

[dsnopek]

Still what is the actual issue with using that command?
While I could translate the code to github script, it won't really simplify the interactions with the GH Api and instead make interacting with the git commands a pain.
And as mentioned above there is (as far as i can tell) no third-party action that can be used instead. (It has already been moved to a local action as I intended above)

Ah, sorry, I misinterpreted your previous comment. I had thought you were planning to switch from the gh shell commands in the YAML, to a script, but had just not gotten around to it yet.

If @akien-mga is OK with having this as shell commands in the YAML, then it's fine!

[akien-mga]

I have no particular problems with using the gh API (we use it too in static checks to get a list of changes files).

But I'm not sure this approach of attempting to download the artifact of the previous commit is going to be reliable, and the script is quite convoluted. Tracking only differences between commits may not be that useful either as we're missing the big picture.

I think it could make more sense to check against the last stable release, and have a system to keep track of which compatibility breakages have been made intentionally - which doubles as a starting point for writing a relevant section in the changelog, release notes, or migration guide, for the next release. Using a stable release means that we can just download the extension_api.json from the relevant tag on godot-cpp and avoid having to download a binary just to regenerate this file.

We discussed this with @YuriSizov a few weeks ago. It could be a .json file checked in this repository that keeps track of the intentional compat breakages. This would require some logic to make use of this list of compat breakages to make the check pass.

Another easier option suggested by @reduz would be to commit the .json of the reference stable API, and hack it when needed to remove/change the APIs which have been intentionally broken. This has the advantage of having everything necessary in this repo, but I'm not fond of committing a Frankenstein API of release N+dirty in the N+1 release's development branch.

A third option is to commit only a .diff file of the accepted compat breaking changes between release N and the current master branch, so this diff can be applied on top of the release N's API before checking for compat breakage with master / the PR.

[myaaaaaaaaa]

Using a stable release means that we can just download the extension_api.json from the relevant tag on godot-cpp and avoid having to download a binary just to regenerate this file.

Downloading a binary to regenerate this file is preferable, as that would make this job much more reusable as a foundation for other types of regression tests, such as @Calinou 's https://github.com/Calinou/godot-rendering-tests.

One way this can be accomplished would be to download the latest stable/dev snapshot from https://downloads.tuxfamily.org/godotengine/ ( or preferably https://github.com/godotengine/godot/releases/download/ ) to check against, and adopt a policy of waiting until right before new builds are released before merging compat-breaking changes.

[YuriSizov]

Downloading a binary to regenerate this file is preferable, as that would make this job much more reusable as a foundation for other types of regression tests, such as @Calinou 's https://github.com/Calinou/godot-rendering-tests.

These tests are not going to be a part of our regular CI. But this is, so it must be robust and as simple as possible. Downloading a binary file and generating it on the fly hits both of those qualifiers. We only need the API description to perform the test, so the ideal approach is to get that API description directly from its source, instead of regenerating it every time. There shouldn't be a circumstance when the JSON in godot-cpp repo has deviated from the generated result of the same release's binary. So there is really no good reason to overcomplicate things like that.

And we still need some way to track and apply allowed discrepancies. Which is the most complicated and annoying part of the setup.

[myaaaaaaaaa]

We only need the API description to perform the test, so the ideal approach is to get that API description directly from its source, instead of regenerating it every time.

Unless GDExtension API checks are the only regression test that Godot will ever have, this seemingly-ideal approach would quickly become the most complicated and error-prone method as more regression tests are added, as each new test would involve an additional description file that needs to be committed and updated regularly. These description files would also need to be re-committed every time regression tests need to be changed due to bugfixes/new features.

On the other hand, a test that downloads the previous snapshot/stable binary can easily be reused for other regression tests. It is also more robust to changes in test behavior, which would allow more flexibility to be able to update regression tests more often.

Downloading a binary to regenerate this file is preferable, as that would make this job much more reusable as a foundation for other types of regression tests, such as Calinou's https://github.com/Calinou/godot-rendering-tests.

These tests are not going to be a part of our regular CI.

Even if this is the case, there may be room for other types of more lightweight rendering tests. Not downloading the previous binary would completely preclude the inclusion of rendering tests into CI at all, which seems like a very premature decision.

And we still need some way to track and apply allowed discrepancies. Which is the most complicated and annoying part of the setup.

One way this can be accomplished is by adopting a policy of waiting until right before new snapshot/stable builds are released before merging compat-breaking PRs, to minimize the window where CI is failing for master due to those discrepancies.

[YuriSizov]

@myaaaaaaaaa As a rule of thumb, we don't work with hypotheticals. If such a need arises that requires us to use a binary of another build, then we can discuss it.

would completely preclude the inclusion of rendering tests into CI at all, which seems like a very premature decision.

We have a very heavy CI. Everything added to it must be weighed very carefully, and this seems to be on the excessive side. We would need to implement a robust system to only run such complex tests when relevant changes were made, otherwise our CI will just die. CI congestion is a huge issue for Godot.

One way this can be accomplished is by adopting a policy of waiting until right before new snapshot/stable builds are released before merging compat-breaking PRs

That's the opposite of how compat-breaking changes should be merged. Such changes must be done as early as possible.

[RedworkDE]

TBH no matter how this PR end up going, it won't interfere with adding any further regression tests even if completely removing this PR ends up necessary / better for whatever future tests there will be. So there is little to no point in worrying about hypothetical future tests.

---

Personally I like the option of keeping a file with whitelisted output from the validate command, possibly with some justification / explanation.

One open question I have is what version should be the reference: Should this be made explicitly configurable somewhere or should it be computed from the current version number? Should 4.1-dev be compared to 4.0 or 4.0.x? I'd just assume that the 4.0 branch should be compared to 4.0.x.

[YuriSizov]

One open question I have is what version should be the reference

That is a good question. When we last discussed it, the idea was to probably check against the last minor and the closest patch version. So 4.0.3 can be checked against 4.0 and 4.0.2 (or likely just 4.0.2), and 4.1 can be checked against 4.0 and the latest 4.0.x version at the time of 4.1's release.

[RedworkDE]

Ok, new design for this: add misc/scripts/validate_extension_api.sh to make it easier to run things locally. That script validates the API against all iles misc/extension_api_validation/*.expected where the basename is the tag to compare against and the content are the expected validation errors for that version. The CI then just calls that script.

If this design is acceptable there some things still to clean up and the CI integration doesn't signal errors correctly currently.

[akien-mga]

I really like this design! It makes it very convenient for us to document the expected compat breakage and the rationale for the change.

[YuriSizov]

This is a cool approach! I'm a bit concerned if this can lead to issues in the future where some new compatibility error shadows pre-existing one. Perhaps messages should be more explicit and verbose, somehow?

I would also change "Validate extension JSON:" to something that looks more like a command, so it's not confused with the comments and clarifications. Something like "validate-json:" or anything else in that same format.

[RedworkDE]

I'm a bit concerned if this can lead to issues in the future where some new compatibility error shadows pre-existing one. Perhaps messages should be more explicit and verbose, somehow?

Same, I have already started looking at improving the validation to prevent such issues, but that is for a different PR.

I would also change "Validate extension JSON:" to something that looks more like a command, so it's not confused with the comments and clarifications. Something like "validate-json:" or anything else in that same format.

That prefix comes from the validation code inside godot and not my scripts. I would prefer changing it there instead of starting to do string manipulations in shell scripts again.

[RedworkDE]

Not sure whats happening here.

[RedworkDE]

From my end the new implementation is done now and can be reviewed again.

[akien-mga]

Looks like a great start to me. Let's merge and iterate from there :)

[akien-mga]

Thanks!