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 version check for GDExtension #80591

Merged
merged 1 commit into from
Aug 16, 2023

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Aug 13, 2023

The version check only checks individual entries, not lexicographical comparison, this would cause a plugin requiring at least "4.1.1" to fail if you are on "4.2.0"

(Not technically needed for 4.1 as the condition won't be true there)

Could alternatively be written as:

if (VERSION_MAJOR != compatibility_minimum[0]) {
	compatible = VERSION_MAJOR > compatibility_minimum[0];
} else if (VERSION_MINOR != compatibility_minimum[1]) {
	compatible = VERSION_MINOR > compatibility_minimum[1];
} else {
	compatible = VERSION_PATCH >= compatibility_minimum[2];
}

@AThousandShips AThousandShips added bug topic:gdextension cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Aug 13, 2023
@AThousandShips AThousandShips added this to the 4.2 milestone Aug 13, 2023
@AThousandShips AThousandShips requested a review from a team as a code owner August 13, 2023 14:22
@AThousandShips AThousandShips removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Aug 13, 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 fine.

Could use VERSION_HEX alternatively for a simple comparison:

#define VERSION_HEX 0x10000 * VERSION_MAJOR + 0x100 * VERSION_MINOR + VERSION_PATCH

@Bromeon
Copy link
Contributor

Bromeon commented Aug 13, 2023

Also, for lexicographical comparison, std::make_tuple(l0, l1, l2) < std::make_tuple(r0, r1, r2) is usually a good trick.

In this case, the hex comparison seems simpler though.

@AThousandShips
Copy link
Member Author

AThousandShips commented Aug 13, 2023

Oh I'd have used that if I were to use the standard library

Can switch to the hex comparison if desired when I have the time, probably tomorrow

@Sslaxx
Copy link

Sslaxx commented Aug 13, 2023

Fixes #80590?

@AThousandShips
Copy link
Member Author

Don't think so, they are on 4.1.1 and the check won't fail for them

@dsnopek dsnopek added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Aug 13, 2023
Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks!

I think any of the variations discussed here would be fine, including the one currently used in the PR.

Copy link
Contributor

@Gallilus Gallilus left a comment

Choose a reason for hiding this comment

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

Seems to make sense.
Only check sub_versions if you are on the relevant branch.
If we are in a newer major version it should be compatible.

Out of the scope of this bug fix but the only thing I can think of is that we are not supporting backporting compatibility.

I will mention that in Rocket Chat.

THANKS

@AThousandShips
Copy link
Member Author

AThousandShips commented Aug 15, 2023

Switched check to my OP suggestion, to match the one used in godotengine/godot-cpp#1208

@akien-mga akien-mga merged commit 4ed3f67 into godotengine:master Aug 16, 2023
@akien-mga
Copy link
Member

Thanks!

@AThousandShips
Copy link
Member Author

Thank you!

@AThousandShips AThousandShips deleted the compat_ver branch August 16, 2023 07:26
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 21, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.2.

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.

7 participants