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

[Daylily-Zeleen]

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.

[dsnopek]

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.

[akien-mga]

Looks fine for platform code.

[akien-mga]

Thanks!

[m4gr3d]

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).

[m4gr3d]

Reverting the change for Android in #86792

[akien-mga]

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

[dsnopek]

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

[dsnopek]

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

[akien-mga]

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

[akien-mga]

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.

[bruvzg]

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.

[bruvzg]

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.

[dsnopek]

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.