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

Allow GDExtensions to set a compatibility_maximum #88417

Merged

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Feb 16, 2024

At the GDExtension meeting on December 1st, 2023 (see notes), we agreed on a number of changes to help with upgrading Godot on projects using GDExtensions.

One of those changes was implementing a compatibility_maximum that can be used in .gdextension files - which is what this PR adds!

While this doesn't solve all the problems, it is simple to add, and is another tool in the toolbox for use by the maintainers of GDExtensions.

In fact, Godot Jolt is already implementing a custom version of this in its register_types.cpp:

	if (internal::godot_version.major != GDJ_GODOT_VERSION_MAJOR ||
		internal::godot_version.minor != GDJ_GODOT_VERSION_MINOR)
	{
		char error_message[4096] = {'\0'};

		snprintf(
			error_message,
			(size_t)count_of(error_message),
			"Godot Jolt failed to load due to not supporting Godot %d.%d. "
			"This version of Godot Jolt (%d.%d.%d) only supports Godot %d.%d.",
			internal::godot_version.major,
			internal::godot_version.minor,
			GDJ_VERSION_MAJOR,
			GDJ_VERSION_MINOR,
			GDJ_VERSION_PATCH,
			GDJ_GODOT_VERSION_MAJOR,
			GDJ_GODOT_VERSION_MINOR
		);

		ERR_PRINT_EARLY(error_message);

		success = 0;
	}

So, this is a pretty good sign that this feature would be useful.

However, I think it would actually be even better if Godot Jolt used compatibility_maximum instead of hard-coding this in the C++, because that would make it easier for advanced users to test it with new versions.

(Selfishly, I would like to be able to grab old binary releases of Godot Jolt and try them with Godot master during development, as a way to test that we didn't break GDExtension's binary compatibility - if it used compatibility_maximum, I could just delete that line ;-))

Please let me know what you think!

/cc @mihe

@dsnopek dsnopek added this to the 4.3 milestone Feb 16, 2024
@dsnopek dsnopek requested a review from a team as a code owner February 16, 2024 21:17
@dsnopek dsnopek force-pushed the gdextension-compatibility-maximum branch from e28ea58 to 9b0ca78 Compare February 16, 2024 21:17
Copy link
Contributor

@mihe mihe left a comment

Choose a reason for hiding this comment

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

I'm all for it, and the change looks good to me. The ability to omit parts of it is a nice touch.

I would frankly like to see this be added to the example .gdextension file in the godot-cpp README.md as well, and have it be the same value as its compatibility_minimum.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Looks great otherwise, nice to see this idea implemented!

core/extension/gdextension.cpp Outdated Show resolved Hide resolved
@Bromeon
Copy link
Contributor

Bromeon commented Feb 16, 2024

I think I was against this in the meeting, but doesn't seem to be in the notes 🤔
My reasons were:

  1. This is a bit against the spirit of GDExtension's backwards compatibility. Ideally, extensions are compatible with newer Godot versions.

    • Now I'm not naive to think we live in a perfect world. Yet I'm still not sure if we should actively encourage this as a feature, as it reinforces the perception that newer versions may not be supported.
    • If extensions break even though they rely on stable API, that should be appropriately treated as a bug. I would avoid "just set compatibility_maximum" to become common advice.
  2. There are already many checks that extensions need to perform:

    • Single/double precision
    • 32/64 bit
    • Other features
    • Maybe specific patch versions to work around.

    So this is no different -- compatibility maximum is not a widely used enough feature that warrants special treatment in .gdextension. It can easily be checked by existing means.

compatibility_minimum on the other hand is very different, this is fundamentally required to know which features an extension uses, and if the engine needs to enable compat interfaces.

@dsnopek dsnopek force-pushed the gdextension-compatibility-maximum branch from 9b0ca78 to 2afa355 Compare February 16, 2024 22:03
@AThousandShips
Copy link
Member

AThousandShips commented Feb 16, 2024

While we aim to have complete forward compatibility, that breaks at times, and it's not always possible to ensure a fully compatible version of a plugin when that happens, so this allows developers to avoid having a version that breaks on some targets, and can then make a new version but keep support for older versions as well, when necessary

I agree that it shouldn't be encouraged broadly, but it prevents issues such as crashes with Jolt when it relies on something specific, including deprecated features that might not work as expected any longer etc., or broken compatibility for a bug that would still break a plugin that relies on it

And allowing plugin developers to not have to ensure forward compatibility of their plugins before they've verified they work etc.

@Bromeon
Copy link
Contributor

Bromeon commented Feb 16, 2024

Do we know why Jolt uses this check? Are they regularly facing breaking changes?

In my opinion we should look into this more closely before just providing workarounds.

@AThousandShips
Copy link
Member

AThousandShips commented Feb 16, 2024

They were relying on a functionality specific to a version AFAIK, they added their check after a crash, let me see if I can find it

CC @mihe you added this check, could you elaborate?

Edit: See here for one case at least

@dsnopek
Copy link
Contributor Author

dsnopek commented Feb 16, 2024

@mihe:

I would frankly like to see this be added to the example .gdextension file in the godot-cpp README.md as well, and have it be the same value as its compatibility_minimum.

I don't know that I'd really want to promote this too much. It's better than folks implementing a custom thing, but like Bromeon says, I think the default should be assuming that older GDExtensions will be compatible with newer versions of Godot.

@Bromeon:

I think I was against this in the meeting, but doesn't seem to be in the notes 🤔

At the meeting, we really need to have someone dedicated to taking notes! I know I miss a lot of stuff trying to simultaneously facilitate the conversation and take notes. Anyway, sorry for failing to note that.

  1. This is a bit against the spirit of GDExtension's backwards compatibility. Ideally, extensions are compatible with newer Godot versions.

I mostly agree!

However, regarding this:

So imo this is not a widely used enough feature that warrants special treatment in .gdextension and can easily be checked by existing means.

I think having a formalized way to do this is better than GDExtensions using "existing means" to check versions, as I explained in the PR description above. If people are going to do it (because it's useful sometimes), I'd rather they do it in the .gdextension file, allowing advanced users to override it if needed.

Copy link
Contributor

@Riteo Riteo left a comment

Choose a reason for hiding this comment

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

The code looks nice, simple and consistent with the minimum version logic!

In regards to the idea itself, I have pretty much the same concerns as of @Bromeon, but the arguments in favor make sense and I can see that we're all being cautious here, so count me as favorable too :D

@mihe
Copy link
Contributor

mihe commented Feb 17, 2024

They were relying on a functionality specific to a version AFAIK, they added their check after a crash, let me see if I can find it

CC @mihe you added this check, could you elaborate?

Edit: See here for one case at least

There were technically two compatibility breakages that affected Godot Jolt in 4.2. The first was an actual binary breakage related to ray-casting, which slipped past --validate-extension-api due to how GDREGISTER_NATIVE_STRUCT works, but which was caught and fixed before 4.2-stable went out. The second breakage was the one you linked, which was more of a behavioral breakage that ended up causing a crash on startup for anyone using Godot Jolt, and which technically still remains. It can certainly be argued whether the behavior in question was intended/supported or not, but in the end things worked one way before 4.2 and another way after 4.2, with no deprecation notice.

In any case, I've only ever supported the latest stable version of Godot in Godot Jolt, since I'm a bit more pessimistic when it comes forward compatibility, and I didn't want to get saddled with whatever bugs might arise from (what I perceived to be) some inevitable breakage. Unfortunately that sort of happened anyway, in part because I didn't communicate the supported version clearly enough, so the version check was just the evolution of that. The fact that it's hardcoded is unfortunate, but I didn't get the sense that people were all that receptive to compatibility_maximum when I first proposed something similar, so I just went with the entry point check instead.

I don't know that I'd really want to promote this too much. It's better than folks implementing a custom thing, but like Bromeon says, I think the default should be assuming that older GDExtensions will be compatible with newer versions of Godot.

Fair enough!

@Bromeon
Copy link
Contributor

Bromeon commented Feb 18, 2024

(@dsnopek) I think having a formalized way to do this is better than GDExtensions using "existing means" to check versions, as I explained in the PR description above. If people are going to do it (because it's useful sometimes), I'd rather they do it in the .gdextension file, allowing advanced users to override it if needed.

If we go down this route, how do you feel about other "often needed" compatibility settings (e.g. an extension requiring double precision or another feature)? Maybe in the future we can think about something like:

[configuration]
required_features = ["double", "64"]

(@mihe) There were technically two compatibility breakages that affected Godot Jolt in 4.2. The first was an actual binary breakage related to ray-casting, which slipped past --validate-extension-api due to how GDREGISTER_NATIVE_STRUCT works, but which was caught and fixed before 4.2-stable went out. The second breakage was the one you linked, which was more of a behavioral breakage that ended up causing a crash on startup for anyone using Godot Jolt, and which technically still remains. It can certainly be argued whether the behavior in question was intended/supported or not, but in the end things worked one way before 4.2 and another way after 4.2, with no deprecation notice.

To me it seems like these issues can very well be introduced in a patch version, not just a minor one.
The compatibility_maximum mechanism does not cover that however -- so you'd be back to square one with custom code.

I.e. I'm not sure if this setting is powerful enough to reliably ensure forward compatibility. Recommending to deal with this properly (custom code) is imo the better solution than recommending compatibility_maximum, which solves the problem half-way but needs custom-code fallbacks again as soon as accidental breakages land in patch versions.

@AThousandShips
Copy link
Member

AThousandShips commented Feb 18, 2024

The compatibility_maximum mechanism does not cover that however -- so you'd be back to square one with custom code.

How do you mean? That it doesn't cover if you assume to start with that you're safe on 4.2, or that it can't cover specific patch versions? If the latter this does allow 3 digits of versioning


Part of my support for this is that I believe it gives a strong tool to developers of plugins to release responsibly, to be able to control the versions they officially cover, and ensure, on their part, that the version range the plugin covers does work, therefore they can release a version with a conservative range and then shift this range when they've tested it with a newly released version

That and just being able to provide multiple versions, or a constrained range of versions, if they rely on some specific quirk or behavior that has been fixed, as we don't guarantee compatibility for fixes a lot of the time, and adapting to a change like that can be complicated, and without some compatibility code you can have a hard time to write a version that works before and after reliably, depending on the behavior that was fixed.

And to a degree, it allows developers of plugins to deal with breakages in a way that doesn't flood them with complaints if we end up breaking something, which I think is a good thing

Of course we should strive to keep compatibility as much as possible, but we've already seen that it's not always possible, so we shouldn't use this as a way to excuse ourselves from having good compatibility, but to provide standardized tools for users to deal with it when it happens

Thinking about this I think it'd be nice to add a custom compatibility message to print, which can instruct the user of the plugin, like:

This plugin was built for Godot 4.2.x, to get a 4.3.x compatible version go to insert page

So that people can keep different versions for example with different features etc.

I think it's good to allow different styles of development for people, some might write a broad support version that either only uses 4.2 features, or has code for different versions, but some might prefer to develop a 4.2 branch and a 4.3 branch to keep their code clean, while allowing themselves to leverage more advanced features etc.

@mihe
Copy link
Contributor

mihe commented Feb 18, 2024

If we go down this route, how do you feel about other "often needed" compatibility settings (e.g. an extension requiring double precision or another feature)? Maybe in the future we can think about something like:

@Bromeon Isn't this already a thing, sort of? You can already add whatever feature tags you want to the [libraries] section of .gdextension files, although the double feature tag is currently missing an important piece (#84711) in order for exporting to be less awkward. The error message generated from not finding a library with the appropriate feature tags might not be very informative right now though.

To me it seems like these issues can very well be introduced in a patch version, not just a minor one. The compatibility_maximum mechanism does not cover that however -- so you'd be back to square one with custom code.

I.e. I'm not sure if this setting is powerful enough to reliably ensure forward compatibility. Recommending to deal with this properly (custom code) is imo the better solution than recommending compatibility_maximum, which solves the problem half-way but needs custom-code fallbacks again as soon as accidental breakages land in patch versions.

I struggle to see what custom code you're envisioning that would be radically different from this compatibility_maximum. In the end there's nothing stopping you from implementing a custom check if you so choose, but surely compatibility_maximum would cover a significant percentage of the use-cases, and provides (advanced) end-users with some sort of standardized path for opting into unsupported usage, rather than having to find out how that specific extension decided to implement their version check.

But yes, you can of course technically end up with breakages in patch versions as well. In the end it's in part a system of trust that Godot isn't going to make any radical changes as part of a patch version, and certainly not intentional breaking changes. If that trust is broken then you might have to be more aggressive with your compatibility_maximum and add the patch version there as well, which as @AThousandShips points out is supported by this PR.

That and just being able to provide multiple versions, or a constrained range of versions, if they rely on some specific quirk or behavior that has been fixed, as we don't guarantee compatibility for fixes a lot of the time

That's actually a very good point. There might even be an argument for having these minimum/maximum versions be part of each individual [libraries] entry at some point, in order to be able to ship one single release of an extension that covers multiple (incompatible) version ranges of Godot, so long as the rest of the .gdextension file is compatible I guess.

@Bromeon
Copy link
Contributor

Bromeon commented Feb 18, 2024

You're both right, sorry -- I was somehow under the impression that this only covers major/minor due to the way how GDExtension's API is versioned, but I realized now that it allows patch. That makes it indeed even with a custom check.

If I interpret the code correctly, compatibility_maximum = "4.2" would mean any 4.2.x is supported (but not 4.3.0), while compatibility_maximum = "4.2.0" would only allow 4.2 and not 4.2.1?

There's a slight confusion because Godot calls 4.2.0 officially 4.2, and here 4.2 means 4.2.x.
But if documented appropriately, it should be OK.


Part of my support for this is that I believe it gives a strong tool to developers of plugins to release responsibly, to be able to control the versions they officially cover, and ensure, on their part, that the version range the plugin covers does work, therefore they can release a version with a conservative range and then shift this range when they've tested it with a newly released version

That's a good point -- maybe in the future such metadata could even be displayed (e.g. in an asset store for extensions) 🤔


Thanks everyone for taking the time to explain, I think I'm good with this now 🙂
Only question left is the versioning string, see above.

@Riteo
Copy link
Contributor

Riteo commented Feb 18, 2024

@Bromeon yup, from what I can tell that's indeed how it works. If you set 4.2 the patch field would be missing and it would thus be implicitly set to 9999:

// If a version part is missing, set the maximum to an arbitrary high value.
compatibility_maximum[i] = 9999;

If you otherwise set 4.2.0, the patch field would be 0 and it would trip this condition only if the version patch is 0:

compatible = VERSION_PATCH <= compatibility_maximum[2];

@akien-mga akien-mga merged commit dc41f25 into godotengine:master Feb 18, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Comment on lines +936 to +938
} else {
compatible = VERSION_PATCH <= compatibility_maximum[2];
}
Copy link
Member

Choose a reason for hiding this comment

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

Fixup: #88527

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.

None yet

6 participants