Fix reload of GDExtension libraries in framework package on macOS #95143
Conversation
There was a problem hiding this comment.
Thanks!
This looks good to me. I tested on Windows and it seems to be working just fine: creation and clean-up of the temporary copy looks good, and a quick test of reload worked fine. I also tested on Linux, just to ensure there wasn't any regression there.
It's probably too late to get this into Godot 4.3-stable, but this is something we should definitely cherry-pick for Godot 4.3.1-stable.
dsnopek
added
platform:windows
cherrypick:4.3
|
Thanks for making the change I suggested! However, you'll need to squash this down to one commit, per Godot's development workflow: https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html#the-interactive-rebase |
`GDExtension::open_library` has a check in it to see if the library was loaded from a temp file, and if it was to restore the original name as that is the one we actually care about. This check is breaking extension reloading on Mac when the library path is to a framework folder, as the file inside the framework will not generally be the same name as the folder. This check also shouldn't be necessary even on Windows, which is the only platform that uses `generate_temp_files`, since disposal of the created temp file is handled within `OS_Windows::open_dynamic_library`, and `GDExtension::open_library` (which is the only function to call `open_dynamic_library` with a `p_data` argument) only cares about the original library file path and has to do extra work to remove the name of the temp file. Instead, I have removed that check and set `OS_Windows::open_dynamic_library` to return the name of the original file and not the name of the copy. This fixes GDExtension reloading on macOS. I do not have a Windows machine available to test that it still works properly on Windows, so someone should check that before merging this.
TV4Fun
force-pushed
the
fix_non_windows_library_load
branch
from
2ffe454 to
f44d6a2
Compare
|
@dsnopek fixed now. |
|
Thanks! |
akien-mga
changed the title
|
Thanks! And congrats for your first merged Godot contribution 🎉 |
|
Cherry-picked for 4.3.1. |
akien-mga
removed
the
cherrypick:4.3
GDExtension::open_libraryhas a check in it to see if the library was loaded from a temp file, and if it was to restore the original name as that is the one we actually care about. This check is breaking extension reloading on Mac when the library path is to a framework folder, as the file inside the framework will not generally be the same name as the folder.This check also shouldn't be necessary even on Windows, which is the only platform that uses
generate_temp_files, since disposal of the created temp file is handled withinOS_Windows::open_dynamic_library, andGDExtension::open_library(which is the only function to callopen_dynamic_librarywith ap_dataargument) only cares about the original library file path and has to do extra work to remove the name of the temp file. Instead, I have removed that check and setOS_Windows::open_dynamic_libraryto return the name of the original file and not the name of the copy.This fixes GDExtension reloading on macOS. I do not have a Windows machine available to test that it still works properly on Windows, so someone should check that before merging this.