Remove unnecessary checks when exporting gdextension binaries and allow using a prefix to auto-detect files #67906

groud · 2022-10-26T16:32:29Z

Closes #63230
May help with #53958, if it's not fixed already. I don't know.

This is something we discussed with @bruvzg and @Faless, it does three things:

Remove a constraint that libs with relative paths were not copied into an exported project.
Allow specifying an autodetect_library_prefix that allows automatically finding files matching the given export features. The logic simply search for the file with the higher number of matching tags.
Likely fixes an unreported bug of [dependencies] section in the .gdextension config file not being processed at all.

Also, to avoid duplicated code between the exporter and the loader, I refactored the code to have a single function looking for the library file.

editor/plugins/gdextension_export_plugin.h

core/extension/native_extension.cpp

DmitriySalnikov · 2022-11-26T10:34:52Z

Likely fixes an unreported bug of [dependencies] section in the .gdextension config file not being processed at all.

Actually dependencies can be processed if you place it first, and then libraries.
The following config template works as intended in beta6, including exporting to android (the exported android app doesn't run for me yet, but the library is added to the apk correctly) v4.0.beta6.official [7f8ecff]

[configuration]

entry_symbol = "godot_init"

[dependencies]

macos.debug = [ ]
macos.release = [ ]
windows.debug.x86_64 = [ ]
windows.release.x86_64 = [ ]
linux.debug.x86_64 = [ ]
linux.release.x86_64 = [ ]

android.release.arm32 = [ ]
android.release.arm64 = [ ]
android.release.x86_32 = [ ]
android.release.x86_64 = [ ]

[libraries]

macos.debug = "res://..."
macos.release = "res://..."
windows.debug.x86_64 = "res://..."
windows.release.x86_64 = "res://..."
linux.debug.x86_64 = "res://..."
linux.release.x86_64 = "res://..."

android.release.arm32 = "res://..."
android.release.arm64 = "res://..."
android.release.x86_32 = "res://..."
android.release.x86_64 = "res://..."

# Repeat for other architectures to support arm64, rv64, etc.

But using the master branch (68e3f49) with this PR (there was one conflict with error printing) breaks how tags are selected.
I got this error with the same config that works in beta6:

core\extension\native_extension.cpp:523 - No GDExtension library found for current OS and architecture (windows.x86_64) in configuration file: res://...

Part of the actual config:

[configuration]

entry_symbol = "godot_qoi_library_init"

[dependencies]

windows.debug.x86_64 = [ ]
windows.release.x86_64 = [ ]

[libraries]

windows.debug.x86_64 = "res://addons/qoi/libs/win/godot_qoi.windows.editor.x86_64.dll"
windows.release.x86_64 = "res://addons/qoi/libs/win/godot_qoi.windows.template_release.x86_64.dll"

If I replace windows.debug.x86_64 with windows.x86_64, then the library loads correctly in the editor. Also, when exporting a release, the same tag is used windows.x86_64, not windows.release.x86_64.

groud · 2022-11-29T11:26:09Z

Oops, misclicked.

Actually dependencies can be processed if you place it first, and then libraries.

If you have a look to the GDExtensionExportPlugin::_export_file function, the dependencies section is retrieved into List<String> dependencies but is then never used again. So I don't think files can be exported correctly. I am not sure if your examples are what you use exactly, but with empty arrays it's not possible to test if it works or not. Dependencies should be exported correctly, which I doubt they are without this PR.

I got this error with the same config that works in beta6:

I think the error is expected. The logic is that all tags in [libraries] identifier should match the ones provided by the export. Here you have one tag more for each line, so it should not match. I think it is expected that you get an error, as the system cannot know which one to choose with not enough tags anyway.

If you did not mess your rebase, I think the fact exporting was working before may be due to the wrong code that was using libraries instead of dependencies in _export_file. But I am not 100% sure.

DmitriySalnikov · 2022-11-29T13:21:19Z

If you have a look to the GDExtensionExportPlugin::_export_file function, the dependencies section is retrieved into List<String> dependencies but is then never used again.

Yes, I've seen it. Unfortunately, dependencies probably don't work in beta6, but at least you can export with these lines.

I think the error is expected. The logic is that all tags in [libraries] identifier should match the ones provided by the export. Here you have one tag more for each line, so it should not match. I think it is expected that you get an error, as the system cannot know which one to choose with not enough tags anyway.

If I replace windows.debug.x86_64 with windows.x86_64, then the library loads correctly in the editor. Also, when exporting a release, the same tag is used windows.x86_64, not windows.release.x86_64.

Now we have editor, template_debug and template_release and different architectures. editor and template_debug seem to be compatible, but template_release is not. And my release export still used windows.x86_64, not windows.release.x86_64. This means that the same library will try to load in all 3 configurations for x86_64.

In addition, the same format is used in the example: https://github.com/godotengine/godot-cpp/blob/576bd172851932ba3db95fda8760b4f297c89efd/test/demo/example.gdextension

But according to the logic of ProjectSettings/ConfigFile, this is the expected behavior, unfortunately.
In the 3.x version, you can add several tags, but they seem to work the same way as in 4.x. (only "64 string" will be used)

Maybe it's just worth improving the handling of ConfigFiles to support multiple tags?

akien-mga · 2022-12-01T13:20:32Z

core/extension/native_extension.h

 #include "core/extension/gdnative_interface.h"
+#include "core/io/config_file.h"


Could possibly be forward declared? Not very important though.

groud

Approved in PR review meeting. Needs the "standalone" feature back, but renamed as "template".

akien-mga

Approved in GDExtension PR meeting.
We discussed that the standalone feature defined for both types of templates should likely still be kept, but likely renamed template (so e.g. on a template_debug build you have both template_debug and template defined).

akien-mga · 2022-12-01T14:41:42Z

Maybe I should implement the same "return the most matching lib" behavior I implemented for the autodetection ?

Yeah that sounds good.

But here's a potential problem:

[libraries]

windows.debug = "bin/windows.debug.dll"
windows.editor = "bin/windows.editor.dll"

Which one should be chosen? Should it raise an error?

We might need something a bit more advanced than just OS features for this if we want to differentiate between the best match and other suitable matches.

groud · 2022-12-01T14:47:37Z

Which one should be chosen? Should it raise an error?

If you only have neither "debug" or "editor", then yeah, it should fail. All tags in the identifier should be in the provided feature list for it to be selected. This is what the current behavior already implement.

The only problem is that in only matches the first match right now, while I think the "most precise" should likely be selected instead.

akien-mga · 2022-12-01T14:48:58Z

Yeah but my question is: Which one will be considered the "most precise" for an editor build between editor and debug?
If we rely only on OS::has_features(), then as far as it's concerned those two features weigh the same.

groud · 2022-12-01T14:52:11Z

Yeah but my question is: Which one will be considered the "most precise" for an editor build between editor and debug?
If we rely only on OS::has_features(), then as far as it's concerned those two features weigh the same.

Ah ok, you mean if both are present! Then I guess the first one will do, just like it is right now ? I can't think of a better way to solve it, unless we start to implement some sort of feature tags priority or something like that. But I think it would be too complex for little gain.

…dextension exports

groud · 2022-12-01T17:22:23Z

Alright, I updated the PR. I tested the algorithm for libraries and it seems to work as expected.
I think the features tags should be ok too, but I'll see if CI is ok with it.

DmitriySalnikov · 2022-12-03T17:30:16Z

Just checked the latest build and found a couple of problems. v4.0.beta.custom_build [181687847]

First I launched the editor without changing the tags in .gdextension and it started without any problems.
Here is the contents of the file

[configuration]

entry_symbol = "godot_qoi_library_init"

[libraries]

windows.debug.x86_64 = "libs/win/godot_qoi.windows.editor.x86_64.dll"
windows.release.x86_64 = "libs/win/godot_qoi.windows.template_release.x86_64.dll"

But when exporting, there was an error related to the formatting of the error string

ERROR: not all arguments converted during string formatting
   at: (.\core/variant/variant.h:820)
ERROR: Method/function failed.
   at: GDExtensionExportPlugin::_export_file (.\editor/plugins/gdextension_export_plugin.h:88)

Next I added template_ and the project was successfully exported and launched as expected

windows.template_debug.x86_64 = "libs/win/godot_qoi.windows.editor.x86_64.dll"
windows.template_release.x86_64 = "libs/win/godot_qoi.windows.template_release.x86_64.dll"

But after that, the editor can't find the right library, and I had to explicitly specify another line with windows.editor.x86_64. Actually, it's not a big problem, so it's probably even better that I need to explicitly specify all 3 configurations. So this is just for reference.

[configuration]

entry_symbol = "godot_qoi_library_init"

[libraries]

windows.editor.x86_64 = "libs/win/godot_qoi.windows.editor.x86_64.dll"
windows.template_debug.x86_64 = "libs/win/godot_qoi.windows.editor.x86_64.dll"
windows.template_release.x86_64 = "libs/win/godot_qoi.windows.template_release.x86_64.dll"

To check autodetect_library_prefix I removed libs/ from the library paths and added autodetect_library_prefix = "res://addons/qoi/libs/". In this case, the paths to the libraries can no longer be found relative to the qoi.gdextension file.

[configuration]

entry_symbol = "godot_qoi_library_init"
autodetect_library_prefix = "res://addons/qoi/libs/"

[libraries]

windows.editor.x86_64 = "win/godot_qoi.windows.editor.x86_64.dll"
windows.template_debug.x86_64 = "win/godot_qoi.windows.editor.x86_64.dll"
windows.template_release.x86_64 = "win/godot_qoi.windows.template_release.x86_64.dll"

│   .gitattributes
│   .gitignore
│   export_presets.cfg
│   icon.svg
│   icon.svg.import
│   project.godot
│
├───addons
│   └───qoi
│       │   LICENSE
│       │   qoi.gdextension
│       │   README.md
│       │
│       └───libs
│           ├───android
│           │       libgodot_qoi.android.template_debug.arm32.so
│           │       libgodot_qoi.android.template_debug.arm64.so
│           │       libgodot_qoi.android.template_debug.x86_32.so
│           │       libgodot_qoi.android.template_debug.x86_64.so
│           │       libgodot_qoi.android.template_release.arm32.so
│           │       libgodot_qoi.android.template_release.arm64.so
│           │       libgodot_qoi.android.template_release.x86_32.so
│           │       libgodot_qoi.android.template_release.x86_64.so
│           │
│           ├───linux
│           │       libgodot_qoi.linux.editor.x86_64.so
│           │       libgodot_qoi.linux.template_release.x86_64.so
│           │
│           ├───macos
│           │       libgodot_qoi.macos.editor.universal.dylib
│           │       libgodot_qoi.macos.template_release.universal.dylib
│           │
│           └───win
│                   godot_qoi.windows.editor.x86_64.dll
│                   godot_qoi.windows.template_release.x86_64.dll
|....

But the editor and the project export could not find these libraries. It looks like autodetect_library_prefix is not used at all.

ERROR: Failed to open C:/My/Projects/GE/4444/addons/qoi/win/godot_qoi.windows.template_release.x86_64.dll
   at: (core\io\dir_access.cpp:346)

Even using completely wrong paths like autodetect_library_prefix = "libssss" I don't get errors related to autodetect_library_prefix

ERROR: Can't open dynamic library: C:/My/Projects/GE/4444/addons/qoi/win/godot_qoi.windows.editor.x86_64.dll, error: Error 126: ...
   at: (platform\windows\os_windows.cpp:256)
ERROR: GDExtension dynamic library not found: C:/My/Projects/GE/4444/addons/qoi/win/godot_qoi.windows.editor.x86_64.dll
   at: NativeExtension::open_library (core\extension\native_extension.cpp:400)
ERROR: Failed loading resource: res://addons/qoi/qoi.gdextension. Make sure resources have been imported by opening the.   at: (core\io\resource_loader.cpp:215)

I also tried adding dependencies, but the specified files were not copied next to the exe, and there were no error messages.

[configuration]

entry_symbol = "godot_qoi_library_init"

[dependencies]

windows.template_debug.x86_64 = ["res://icon.svg", "res://shared_object.dll"]
windows.template_release.x86_64 = ["res://icon.svg", "res://shared_object.dll"]

[libraries]

windows.editor.x86_64 = "libs/win/godot_qoi.windows.editor.x86_64.dll"
windows.template_debug.x86_64 = "libs/win/godot_qoi.windows.editor.x86_64.dll"
windows.template_release.x86_64 = "libs/win/godot_qoi.windows.template_release.x86_64.dll"

groud · 2022-12-05T09:34:25Z

But when exporting, there was an error related to the formatting of the error string

That's weird, but I don't think it's a related to this PR. It might be worth opening a dedicated issue.

But after that, the editor can't find the right library, and I had to explicitly specify another line with windows.editor.x86_64. Actually, it's not a big problem, so it's probably even better that I need to explicitly specify all 3 configurations.

Technically, you don't need to, this should work:

[configuration]

entry_symbol = "godot_qoi_library_init"

[libraries]

windows.x86_64 = "libs/win/godot_qoi.windows.editor.x86_64.dll"
windows.template_release.x86_64 = "libs/win/godot_qoi.windows.template_release.x86_64.dll"

The algorithm should now select the most precise line out of the two, when all fits. Otherwise it will fall back to the first line.

To check autodetect_library_prefix I removed libs/ from the library paths and added autodetect_library_prefix = "res://addons/qoi/libs/". In this case, the paths to the libraries can no longer be found relative to the qoi.gdextension file.

The prefix should include also the files prefix. A prefix is not a path to a folder, it MUST include the part of the file that does not consist of acceptable tags. Your library prefix should then be:

autodetect_library_prefix = "res://addons/qoi/libs/libgodot_qoi"

This is provided you put all library files in the exact same folder. You should not use subfolders there.

DmitriySalnikov · 2022-12-05T17:04:30Z

That's weird, but I don't think it's a related to this PR. It might be worth opening a dedicated issue.

It looks like this error appeared after #68271.

The algorithm should now select the most precise line out of the two, when all fits. Otherwise it will fall back to the first line.

Yeah cool! I completely forgot about this option.

The prefix should include also the files prefix. A prefix is not a path to a folder, it MUST include the part of the file that does not consist of acceptable tags.

The whole file can now look like this:

[configuration]

entry_symbol = "godot_qoi_library_init"
autodetect_library_prefix = "res://addons/qoi/libs/libgodot_qoi"

Now I understand how it works. Looks useful.

What about dependencies?

groud · 2022-12-06T09:24:11Z

What about dependencies?

I don't know. maybe try to open an issue with a MRP maybe. This was not really the original goal of this PR, so it might deserve another one.

DmitriySalnikov · 2022-12-06T10:02:03Z

Thanks. I'll open new issues if needed.

akien-mga · 2022-12-06T10:07:05Z

Thanks!

bruvzg · 2022-12-06T13:43:10Z

I also tried adding dependencies, but the specified files were not copied next to the exe, and there were no error messages.

I think dependencies are expected to be in a Dictonary form to allow setting target location (relevant for macOS, non-executable files should be stored in a different location, it was added to allow embedding JRE). Have not tested if it's working, so it might be still broken:

[dependencies]

macos.release = [
    { "res://source/non_exefile.cfg" : "/Content/Resources/" },
    { "res://source/exe_file.dylib" : "/Content/Frameworks/" }
]

DmitriySalnikov · 2022-12-06T22:44:40Z

I think dependencies are expected to be in a Dictonary form to allow setting target location

@bruvzg Yes, it requires a Dictionary, but without embedding into an Array
Here is a working example:

[dependencies]

windows.x86_64 = {
    "res://icon.svg" : "Content/",
    "res://test.gd" : "",
    "README.md" : "",
}
# "README.md" is a relative path to .gdextension

This is what the folder looks like after exporting

Thanks for the tip!

akien-mga · 2022-12-09T13:17:42Z

editor/export/editor_export_platform_pc.cpp

+	bool dvalid = exists_export_template(get_template_file_name("template_debug", arch), &err);
+	bool rvalid = exists_export_template(get_template_file_name("template_release", arch), &err);


This broke looking up the templates. Is that needed for GDExtension or was just a search-and-replace mistake?

Regression from godotengine#67906.

groud added enhancement topic:gdextension topic:export labels Oct 26, 2022

groud added this to the 4.0 milestone Oct 26, 2022

fire changed the title ~~Remove uncessecary checks when exporting gdextension binaries and allow using a prefix to auto-detect files~~ Remove unnecessary checks when exporting gdextension binaries and allow using a prefix to auto-detect files Oct 26, 2022

bruvzg reviewed Oct 26, 2022

View reviewed changes

editor/plugins/gdextension_export_plugin.h Outdated Show resolved Hide resolved

groud force-pushed the simpler_gdextension_config branch from ae8fe75 to 9462e41 Compare October 27, 2022 09:41

Faless reviewed Oct 27, 2022

View reviewed changes

editor/plugins/gdextension_export_plugin.h Outdated Show resolved Hide resolved

editor/plugins/gdextension_export_plugin.h Outdated Show resolved Hide resolved

groud force-pushed the simpler_gdextension_config branch from 9462e41 to 447779e Compare October 27, 2022 09:55

Faless reviewed Oct 27, 2022

View reviewed changes

editor/plugins/gdextension_export_plugin.h Outdated Show resolved Hide resolved

groud force-pushed the simpler_gdextension_config branch 3 times, most recently from 66638a4 to 5c5c042 Compare October 27, 2022 15:37

groud marked this pull request as ready for review October 27, 2022 15:52

groud requested review from a team as code owners October 27, 2022 15:52

touilleMan reviewed Oct 28, 2022

View reviewed changes

core/extension/native_extension.cpp Outdated Show resolved Hide resolved

groud force-pushed the simpler_gdextension_config branch from 5c5c042 to 37d9a8d Compare October 28, 2022 08:29

DmitriySalnikov mentioned this pull request Nov 25, 2022

Request for migrating project to godot4.0 DmitriySalnikov/godot_qoi#5

Closed

mhilbrunner added the bug label Nov 29, 2022

groud closed this Nov 29, 2022

groud reopened this Nov 29, 2022

groud force-pushed the simpler_gdextension_config branch from 37d9a8d to e4624a9 Compare November 29, 2022 11:29

akien-mga reviewed Dec 1, 2022

View reviewed changes

groud commented Dec 1, 2022

View reviewed changes

akien-mga approved these changes Dec 1, 2022

View reviewed changes

groud added 2 commits December 1, 2022 17:28

Remove uncessecary checks when exporting gdextension binaries

36bcb82

Allow specifying a prefix to automatically detect library files for g…

fa4143c

…dextension exports

groud force-pushed the simpler_gdextension_config branch from e4624a9 to fa4143c Compare December 1, 2022 17:20

mihe mentioned this pull request Dec 3, 2022

Add feature tags to signify engine float precision #69538

Merged

akien-mga approved these changes Dec 6, 2022

View reviewed changes

akien-mga merged commit 83b426b into godotengine:master Dec 6, 2022

akien-mga mentioned this pull request Dec 6, 2022

Exporting GDExtensions doesn't work #53958

Closed

akien-mga reviewed Dec 9, 2022

View reviewed changes

akien-mga mentioned this pull request Dec 9, 2022

Editor: Fix regression fetching Windows/Linux export templates #69805

Merged

akien-mga added a commit to akien-mga/godot that referenced this pull request Dec 9, 2022

Editor: Fix regression fetching Windows/Linux export templates

dc1a3fb

Regression from godotengine#67906.

cridenour mentioned this pull request Dec 10, 2022

Update to beta 8 and update demo .gdextension. 2shady4u/godot-sqlite#113

Merged

rohanrhu pushed a commit to rohanrhu/godot that referenced this pull request Dec 28, 2022

Editor: Fix regression fetching Windows/Linux export templates

12466f3

Regression from godotengine#67906.

rohanrhu pushed a commit to rohanrhu/godot that referenced this pull request Dec 28, 2022

Editor: Fix regression fetching Windows/Linux export templates

c024e35

Regression from godotengine#67906.

DmitriySalnikov mentioned this pull request Jan 12, 2023

GDExtensions are not exported correctly: No suitable library found for GDExtension: res://webrtc/webrtc.gdextension #71090

Closed

Streq pushed a commit to Streq/godot that referenced this pull request Feb 9, 2023

Editor: Fix regression fetching Windows/Linux export templates

463a77a

Regression from godotengine#67906.

akien-mga mentioned this pull request Feb 13, 2023

GDExtension: Remove extraneous dependency getter and fix export error #63233

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove unnecessary checks when exporting gdextension binaries and allow using a prefix to auto-detect files #67906

Remove unnecessary checks when exporting gdextension binaries and allow using a prefix to auto-detect files #67906

groud commented Oct 26, 2022 •

edited

Loading

DmitriySalnikov commented Nov 26, 2022

groud commented Nov 29, 2022

DmitriySalnikov commented Nov 29, 2022

akien-mga Dec 1, 2022

groud left a comment

akien-mga left a comment

akien-mga commented Dec 1, 2022 •

edited

Loading

groud commented Dec 1, 2022

akien-mga commented Dec 1, 2022 •

edited

Loading

groud commented Dec 1, 2022

groud commented Dec 1, 2022

DmitriySalnikov commented Dec 3, 2022 •

edited

Loading

groud commented Dec 5, 2022

DmitriySalnikov commented Dec 5, 2022 •

edited

Loading

groud commented Dec 6, 2022

DmitriySalnikov commented Dec 6, 2022

akien-mga commented Dec 6, 2022

bruvzg commented Dec 6, 2022

DmitriySalnikov commented Dec 6, 2022

akien-mga Dec 9, 2022

		#include "core/extension/gdnative_interface.h"
		#include "core/io/config_file.h"

		bool dvalid = exists_export_template(get_template_file_name("template_debug", arch), &err);
		bool rvalid = exists_export_template(get_template_file_name("template_release", arch), &err);

Remove unnecessary checks when exporting gdextension binaries and allow using a prefix to auto-detect files #67906

Remove unnecessary checks when exporting gdextension binaries and allow using a prefix to auto-detect files #67906

Conversation

groud commented Oct 26, 2022 • edited Loading

DmitriySalnikov commented Nov 26, 2022

groud commented Nov 29, 2022

DmitriySalnikov commented Nov 29, 2022

akien-mga Dec 1, 2022

Choose a reason for hiding this comment

groud left a comment

Choose a reason for hiding this comment

akien-mga left a comment

Choose a reason for hiding this comment

akien-mga commented Dec 1, 2022 • edited Loading

groud commented Dec 1, 2022

akien-mga commented Dec 1, 2022 • edited Loading

groud commented Dec 1, 2022

groud commented Dec 1, 2022

DmitriySalnikov commented Dec 3, 2022 • edited Loading

groud commented Dec 5, 2022

DmitriySalnikov commented Dec 5, 2022 • edited Loading

groud commented Dec 6, 2022

DmitriySalnikov commented Dec 6, 2022

akien-mga commented Dec 6, 2022

bruvzg commented Dec 6, 2022

DmitriySalnikov commented Dec 6, 2022

akien-mga Dec 9, 2022

Choose a reason for hiding this comment

groud commented Oct 26, 2022 •

edited

Loading

akien-mga commented Dec 1, 2022 •

edited

Loading

akien-mga commented Dec 1, 2022 •

edited

Loading

DmitriySalnikov commented Dec 3, 2022 •

edited

Loading

DmitriySalnikov commented Dec 5, 2022 •

edited

Loading