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

Distinguish between dynamic library not found and can't be opened. #86682

Conversation

Daylily-Zeleen
Copy link
Contributor

Sometimes dynamic libraries can be found but cannot be opened successful (for example, Goodot's type static variables in dynamic library can't be initialized correctly and lead to return an invalid handle).
In this case, the output will print a error message "GDExtension dynamic library not found: xxxx" confuse users, this pr will clearfy it.

@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/distinguish_between_dynamic_libaray_not_found_and_can't_open branch from f6a722b to fe6b073 Compare January 1, 2024 12:02
@Daylily-Zeleen Daylily-Zeleen marked this pull request as ready for review January 1, 2024 12:02
@Daylily-Zeleen Daylily-Zeleen requested review from a team as code owners January 1, 2024 12:02
Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks!

This is a great idea, and the code looks good to me.

I tested on Linux, both the case where the library file can't be found, and where it's not a valid library (I just put an empty file where the library should have been), and everything seemed to work as it should.

@AThousandShips AThousandShips added this to the 4.3 milestone Jan 1, 2024
@AThousandShips AThousandShips changed the title Distinguishs between dynamic library not found and can't be opened. Distinguish between dynamic library not found and can't be opened. Jan 1, 2024
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 fine for platform code.

@akien-mga akien-mga merged commit 37df2ff into godotengine:master Jan 2, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@@ -170,6 +170,8 @@ Error OS_Android::open_dynamic_library(const String p_path, void *&p_library_han
so_file_exists = false;
}

ERR_FAIL_COND_V(!FileAccess::exists(path), ERR_FILE_NOT_FOUND);
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, this breaks dynamic library loading on Android since on that platform the original path may be inaccessible which requires moving the library around (see logic below).

Copy link
Contributor

Choose a reason for hiding this comment

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

Reverting the change for Android in #86792

Copy link
Member

Choose a reason for hiding this comment

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

Actually I reviewed this a bit fast, this may also fail on other platforms. So the whole PR might need to be reverted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Eep, sorry, guys! I thought it would be fine

Copy link
Contributor

@dsnopek dsnopek Jan 4, 2024

Choose a reason for hiding this comment

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

It looks like we could report a different error code if the file doesn't exist on Android too, it would just need to be handled later, I guess as an else on the 2nd if (internal_so_file_exists) check?

EDIT: No, that doesn't make sense. I'm actually a little confused as to why the current version causes problems. The copying branch only happens if so_file_exists is true, so it doesn't sound like missing that branch is what's causing the problem? Is there some case where loading a library should work even when so_file_exists is false?

EDIT 2: Ah, looking at this PR #68362 I think I understand this better

Copy link
Member

Choose a reason for hiding this comment

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

So my worry here is that this function is currently only used for GDExtension, OpenXR (for Android), and .NET (using absolute paths), so we might be forgetting about the fact that dynamic libraries can be in C:\Windows\System32 or /usr/lib//usr/lib64.

We don't use it currently (Linux does a bunch of dynamic loading but doesn't seem to rely on this method, it uses dlopen directly), but in theory I would expect this function to succeed if I do OS::get_singleton()->open_dynamic_library("libSDL2.so", sdl_handle); on Linux to load /usr/lib64/libSDL2.so on my system (with /usr/lib64 being part of my distro's default library paths).

While I feel we're treating it more as a open_gdextension_library currently, and only taking this use case into account.

Am I missing something?

CC @bruvzg

Copy link
Member

Choose a reason for hiding this comment

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

Actually looking further at the OS_Unix implementation I see it's not the generic wrapper around dlopen I thought it would be. It does seem focused only on trying to load a library next to the binary or in the parent ../lib folder if it exists.

So I guess the other checks added for Unix/Windows are ok. But there should likely be an explicit _MSG that refers back to the original p_path and not just the generic error that would mention the modified path.

Because currently trying to open mylib.so if it doesn't exist will fail with an error referring to ../lib/mylib.so.

Copy link
Member

Choose a reason for hiding this comment

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

Currently, (for GDExtension and, .NET) it is OK to assume we are using full path, but for something like #85741 it won't be, also GDExtension can try using it to load its dependencies.

Maybe we should check if path is absolute path and only do this check (and path substitution). So open_dynamic_library("res://libSDL2.so") will check do it and open_dynamic_library("libSDL2.so") will skip the checks and let system look for the lib.

Copy link
Member

Choose a reason for hiding this comment

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

FYI, this breaks dynamic library loading on Android since on that platform the original path may be inaccessible which requires moving the library around (see logic below).

For this, we probably should try substituting path for the expected one, like on macOS it's trying to replace executable folder with ../Frameworks/ if lib is not found.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about something like PR #86794? It could certainly be improved with a message, but I think it should give the correct error code when the file doesn't exist.

m4gr3d added a commit to m4gr3d/godot that referenced this pull request Jan 4, 2024
GuybrushThreepwood-GitHub pushed a commit to GuybrushThreepwood-GitHub/godot that referenced this pull request Jan 27, 2024
@Daylily-Zeleen Daylily-Zeleen deleted the daylily-zeleen/distinguish_between_dynamic_libaray_not_found_and_can't_open branch March 1, 2024 09:38
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.

6 participants