Require 'api_version' and allow extensions to provide a default - #2034
Require 'api_version' and allow extensions to provide a default#2034dsnopek wants to merge 1 commit into
Conversation
e29a4d6 to
a986acf
Compare
| `SConstruct`, for example: | ||
|
|
||
| ```python | ||
| env = SConscript("godot-cpp/SConstruct", {"default_api_version": "4.7"}) |
There was a problem hiding this comment.
I think we don't need to differentiate between default_api_version and api_version.
If the user SConstruct places api_version="4.7" into the env, I think the one running the command can still override it with api_version=4.6 in the bash command. I think that's how it works, anyway.
There was a problem hiding this comment.
Oh, good point! I don't know why I thought I needed to have a separate default variable. I've updated this in my latest push!
Unfortunately, I couldn't figure out how to do the same thing with CMake, so it still uses a GODOTCPP_DEFAULT_API_VERSION to set the default.
@enetheru Is there an easy way to make it so that the developer can set GODOTCPP_API_VERSION, which is the default for the extension, and that gets overridden by -DGODOTCPP_API_VERSION=... passed on the command-line? If the answer is "no" or making that work is sufficiently more complicated, I'll just leave the CMake changes as they are
There was a problem hiding this comment.
in a plugin developers project, before they consume godot-cpp they can do this
if( GODOTCPP_API_VERSION STREQUAL "" )
set(GODOTCPP_API_VERSION "4.7")
endif()a986acf to
4f12588
Compare
Ivorforce
left a comment
There was a problem hiding this comment.
lgtm!
Can't judge the CMake code.
enetheru
left a comment
There was a problem hiding this comment.
I get it now. yeah its all good.
| if(GODOTCPP_ENABLE_TESTING) | ||
| set(GODOTCPP_DEFAULT_API_VERSION "4.7") | ||
| endif() | ||
|
|
There was a problem hiding this comment.
whatever the godot-cpp is compiled with, so will the test. so this isn't necessary. setting it in the github yaml takes care of both.
There was a problem hiding this comment.
So, the idea is that the test project is an extension, and the imaginary author of that extension wants to set the default API version to "4.7". If you look in test/SConstruct we're setting the default to "4.7" there as well.
Similarly, I would love for the code referenced above to be in test/CMakeLists.txt, but I couldn't make that work because the test project is built weird, and I had to put it in the top-level CMakeLists.txt. If you know a way to put the default in test/CMakeLists.txt that would be awesome!
In a real project, I think the extension developer can set GODOTCPP_DEFAULT_API_VERSION before they include godot-cpp's CMakeLists.txt and it should work?
This doesn't really relate to what we're doing on CI (where we may test multiple different versions). This is attempting to set the default used for tests when the user doesn't explicitly set an API version, including when building locally
There was a problem hiding this comment.
I just added an additional comment by this code to explain that this only done this way because the test project is built weird :-)
There was a problem hiding this comment.
For cmake the integration testing is just another target in the godot-cpp project.
4f12588 to
9d050a9
Compare
At a few GDExtension meetings, we've discussed not defaulting the
api_versionto the latest Godot API that godot-cpp is aware of, but instead requiring the extension developer to provide the default.This PR attempts to implement that!
AI disclosure: I used AI to help with the CMake code, and to double check if I forgot anything that needed to be updated.
@enetheru I'd really appreciate review of the CMake stuff. I had to put the
GODOTCPP_DEFAULT_API_VERSIONdefine in the top-level CMakeLists.txt, because I couldn't figure out a better way to do it for how the test project is built. But I think a normal extension should be able to define that in their CMakeLists.txt before including the godot-cpp one, right?