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

Change GDExtension's library_path back to an absolute path #84620

Merged

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Nov 8, 2023

In Godot 4.1, the library path is an absolute path to the library.

After PR #82603, it's no longer guaranteed to be absolute, but matches whatever path was given in the .gdextension file, which could be a res:// style path.

This PR should change it back to it being an absolute path.

Marking as a draft, so that the folks experiencing this problem can test and let me know if it fixes the issue for them!

@dsnopek dsnopek added this to the 4.2 milestone Nov 8, 2023
@dsnopek dsnopek requested a review from a team as a code owner November 8, 2023 15:08
@dsnopek dsnopek marked this pull request as draft November 8, 2023 15:15
Copy link
Contributor

@fuzzybinary fuzzybinary left a comment

Choose a reason for hiding this comment

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

Tested and this fixes my issue.

@maiself
Copy link
Contributor

maiself commented Nov 9, 2023

This seems to fix things when running the editor, but an exported project now receives an invalid path. open_dynamic_library() has a fourth argument that returns the true path to the library, but is unused after #82603. I think that needs to be reincorporated somehow to fix this properly.

@maiself
Copy link
Contributor

maiself commented Nov 9, 2023

diff of my attempt
diff --git a/core/extension/gdextension.cpp b/core/extension/gdextension.cpp
index 6c3d0a6148..908a7e8547 100644
--- a/core/extension/gdextension.cpp
+++ b/core/extension/gdextension.cpp
@@ -678,15 +678,16 @@ GDExtensionInterfaceFunctionPtr GDExtension::get_interface_function(StringName p
 }
 
 Error GDExtension::open_library(const String &p_path, const String &p_entry_symbol) {
-	library_path = p_path;
-
 	String abs_path = ProjectSettings::get_singleton()->globalize_path(p_path);
+
+	bool library_path_set = false;
+
 #if defined(WINDOWS_ENABLED) && defined(TOOLS_ENABLED)
 	// If running on the editor on Windows, we copy the library and open the copy.
 	// This is so the original file isn't locked and can be updated by a compiler.
 	if (Engine::get_singleton()->is_editor_hint()) {
 		if (!FileAccess::exists(abs_path)) {
-			ERR_PRINT("GDExtension library not found: " + library_path);
+			ERR_PRINT("GDExtension library not found: " + abs_path);
 			return ERR_FILE_NOT_FOUND;
 		}
 
@@ -701,7 +702,7 @@ Error GDExtension::open_library(const String &p_path, const String &p_entry_symb
 
 		Error copy_err = DirAccess::copy_absolute(abs_path, copy_path);
 		if (copy_err) {
-			ERR_PRINT("Error copying GDExtension library: " + library_path);
+			ERR_PRINT("Error copying GDExtension library: " + abs_path);
 			return ERR_CANT_CREATE;
 		}
 		FileAccess::set_hidden_attribute(copy_path, true);
@@ -709,12 +710,16 @@ Error GDExtension::open_library(const String &p_path, const String &p_entry_symb
 		// Save the copied path so it can be deleted later.
 		temp_lib_path = copy_path;
 
+		// Save the original path of the library.
+		library_path = abs_path;
+		library_path_set = true;
+
 		// Use the copy to open the library.
 		abs_path = copy_path;
 	}
 #endif
 
-	Error err = OS::get_singleton()->open_dynamic_library(abs_path, library, true);
+	Error err = OS::get_singleton()->open_dynamic_library(abs_path, library, true, library_path_set ? nullptr : &library_path);
 	if (err != OK) {
 		ERR_PRINT("GDExtension dynamic library not found: " + abs_path);
 		return err;

Caveat here is during editor runs on Windows library_path will point to the original library instead of the loaded copy. Not sure if that's actually an issue.

@dsnopek dsnopek force-pushed the gdextension-library-path-absolute branch from c04608d to 2ee612d Compare November 9, 2023 13:33
@dsnopek
Copy link
Contributor Author

dsnopek commented Nov 9, 2023

open_dynamic_library() has a fourth argument that returns the true path to the library, but is unused after #82603. I think that needs to be reincorporated somehow to fix this properly.

Ah, thanks, I didn't notice the change with regard to the 4th argument.

I haven't had a chance to test it yet, but my latest push attempts to incorporate that.

@dsnopek dsnopek force-pushed the gdextension-library-path-absolute branch from 2ee612d to 74ccc82 Compare November 9, 2023 13:48
@maiself
Copy link
Contributor

maiself commented Nov 9, 2023

I haven't actually tested it, but the latest version looks good!

@dsnopek dsnopek force-pushed the gdextension-library-path-absolute branch from 74ccc82 to 09fcc3a Compare November 9, 2023 17:25
@dsnopek
Copy link
Contributor Author

dsnopek commented Nov 9, 2023

Alright, I finally got around to testing this! On Windows, it's now giving me the expected library path AND I've confirmed that this doesn't regress the fix from PR #82603. This one should be ready now!

@dsnopek dsnopek marked this pull request as ready for review November 9, 2023 17:26
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.

Looks good to me.

@akien-mga akien-mga merged commit b06fe0d into godotengine:master Nov 9, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

4 participants