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

Check that GDExtensionCompatHashes are valid when generating extension_api.json #84973

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Nov 16, 2023

Our CI uses the extension_api.json to check that we haven't broken GDExtension binary compatibility by removing a method with a particular hash. We're also using GDExtensionCompatHashes to map from some old broken hashes to some new ones.

However, there isn't anything presently that checks that these mapped hashes are actually valid, which could hide compatibility breakages until runtime.

This PR attempts to validate the mapped hashes when generating the extension_api.json.

@dsnopek dsnopek force-pushed the gdextension-fix-compat-hashes branch from 2c94571 to 5cf6d08 Compare November 16, 2023 12:40
@dsnopek
Copy link
Contributor Author

dsnopek commented Nov 16, 2023

Note: this adds a warning, but the most important part is probably the continue which will not add the compat hash to the extension_api.json if the target hash doesn't exist, which should trigger the usual errors we already have setup in CI. The warning will just help the person trying to fix this find the hash that's no longer valid.

@akien-mga akien-mga modified the milestones: 4.x, 4.2 Nov 16, 2023
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Seems confirmed working by #84906 failing CI with this PR.

@akien-mga akien-mga merged commit 47c7abc into godotengine:master Nov 16, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

if (p_check_valid) {
MethodBind *mb = ClassDB::get_method_with_compatibility(p_class, p_method, mapping.current_hash);
if (!mb) {
WARN_PRINT(vformat("Compatibility hash %d mapped to non-existent hash %d. Please update gdextension_compat_hashes.cpp.", mapping.legacy_hash, mapping.current_hash));
Copy link
Member

Choose a reason for hiding this comment

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

One point of potential confusion here is that we use hexadecimal representation of the hash in the other check, didn't realize previously, might be worth improving to unify

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, however, we use plain integers in the source code, so if someone is trying to find the hash in the code it's more useful in integer form. It would be good to unify this in some way, though.

@dsnopek dsnopek deleted the gdextension-fix-compat-hashes branch July 22, 2024 15:30
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.

3 participants