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

Fix or workaround recent extension API compatibility issues #80168

Merged
merged 1 commit into from
Aug 3, 2023

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented Aug 2, 2023

Draft as I'm still getting this error despite the compat method I added:

Validate extension JSON: Error: Field 'classes/RenderingDevice/methods/shader_create_from_bytecode/arguments': size changed value in new API, from 1 to 2.

@akien-mga akien-mga added this to the 4.2 milestone Aug 2, 2023
@akien-mga akien-mga requested review from dsnopek and RedworkDE August 2, 2023 13:16
@akien-mga akien-mga requested review from a team as code owners August 2, 2023 13:16
@akien-mga akien-mga marked this pull request as draft August 2, 2023 13:16
@akien-mga
Copy link
Member Author

So #78328 added compat code for TileMap.get_used_rect which was made const, but previous compat checks added in 4.1 all handled const changes as a non-breaking change. That's also what I did in this PR.

So which is it? Do we need compat bindings for non-const methods which were made const?

And why doesn't the compat binding work for RenderingDevice.shader_create_from_bytecode?

Comment on lines -33 to -36
#include "core/object/object.h"

#include "core/object/class_db.h"

Copy link
Member Author

Choose a reason for hiding this comment

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

These didn't seem necessary, so I simplified. Otherwise it looked like these were ordered this way with a separation to solve a recursive inclusion issue (since object is already included in class_db), but since it compiles fine without it I guess it was a remnant from previous architectures.

@dsnopek
Copy link
Contributor

dsnopek commented Aug 2, 2023

So which is it? Do we need compat bindings for non-const methods which were made const?

We do need a compatibility method for changes to const-ness, because it changes the hash which breaks GDExtension.

It appears godot --validate-extension-api will emit two separate messages for this change: one about the hash (which adding the compatibility method will fix) and another separate one about the change from non-const to const.

From the perspective of GDExtension, all that matters is the hash. We could remove the specific check for is_const and just rely on the hash mismatch? Although, then it might be hard to figure out why the hash doesn't match. :-) If it were possible to make it not emit any other messages if the hashes were good, that would be ideal, but looking at how this is implemented, that doesn't seem easy to add, since it's basically just comparing the raw data from the extension_api.json.

As it is currently, it seems that you need to do what you did, which is add an exception for the additional message about const-ness change.

@akien-mga
Copy link
Member Author

I'll try adding the non-const compat methods and see if that solves both errors.

@akien-mga
Copy link
Member Author

OK, so I added the two compat methods which were missing for the hash mismatches reported by the tool.

And indeed it seems the tool still reports compatibility breaking changes even after adding compat methods, so those reports need to be silenced manually by adding them to the file anyway. Did we get it right @RedworkDE?

@akien-mga akien-mga marked this pull request as ready for review August 2, 2023 17:49
@akien-mga akien-mga requested a review from a team as a code owner August 2, 2023 17:49
Copy link
Member

@RedworkDE RedworkDE left a comment

Choose a reason for hiding this comment

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

And indeed it seems the tool still reports compatibility breaking changes even after adding compat methods, so those reports need to be silenced manually by adding them to the file anyway. Did we get it right @RedworkDE?

Well the tool reports all API changes compared to a given version (with some minor exceptions). Adding the compatibility method only suppresses the error about the hash change of the method as that is the only thing that adding a compat method fixes. The underlying method still has changed and that change is still reported as it might still be an issue for use-cases other than godot-cpp.

TL/DR: Yes.


Changes look correct and as expected to me.
I still have Ideas for general improvements (primarily actually checking against 4.1, without duplicating everything) but I'll get to them soon(tm) in a different PR.

- Add compatibility methods for `RenderingDevice::shader_create_from_bytecode`
  and `CodeEdit::get_text_for_symbol_loopup`.
- Silence errors which now have compatibility methods.
- Acknowledge GraphEdit/GraphNode compat breakage, intended and WIP.
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