Fix Android dynamic library loading, while still distinguishing between not found and can't open #86794
Conversation
…en not found and can't open
| @@ -199,6 +197,7 @@ Error OS_Android::open_dynamic_library(const String p_path, void *&p_library_han | |||
| } | |||
| } | |||
|
|
|||
| ERR_FAIL_COND_V(!p_library_handle && !so_file_exists, ERR_FILE_NOT_FOUND); | |||
There was a problem hiding this comment.
@dsnopek I think this check should be within the if block above if internal_so_file_exists is false.
The logic in this function is used to open shared libraries that are bundled with the apk / aab and shared libraries that are on the device but 'external' to the apk / aab binary.
In the first scenario, the path is irrelevant as (1) it doesn't match the location of the shared library embedded in the apk / aab (not sure if there's an actual usable path for it) and (2) the apk / aab is able to open and load the embedded shared library given only its name.
The loading can still fail however for example if the shared library doesn't match the device architecture (e.g: x86 library on arm device). With your current update, the function would return ERR_FILE_NOT_FOUND for that scenario.
In the second scenario, the 'external' path should point to the shared library, but Android security mechanisms prevent from loading external shared libraries. We work around that by copying the shared library to an internal path before attempting to open and load it (this is how the Android editor loads and uses GDExtension plugins).
For that case, we could return the ERR_FILE_NOT_FOUND error if the copy attempt is unsuccessful (unless there's another error that matches better).
ps: feel free to add this comment to the function for documentation / clarification.
There was a problem hiding this comment.
I think this check should be within the
ifblock above ifinternal_so_file_existsis false.
I don't think we could ever get to that if block if so_file_exists is false, because the outer most if block would only be entered if so_file_exists is true. That whole section will only run if the file does exist, whereas we only want to report ERR_FILE_NOT_FOUND if it doesn't.
Although, I may be misunderstanding, because this is quite confusing :-)
There was a problem hiding this comment.
No that sounds correct. It may just mean this error cannot be used for the Android platform:
- If we enter the
ifblock, it means that the file exists - If we don't enter the
ifblock, it only tells us the file is not available externally from the apk. The file could exist and be embedded within the apk, or could be missing from the apk as well..
The Android api (System.loadLibrary(...)) typically specifies whether the library is available but invalid, or missing.. any idea if dlopen provides such capabilities?
There was a problem hiding this comment.
Well, I figured if we got all the way past that block, and we failed to load it and initial file didn't exist... it probably doesn't exist? That's what the line I added is attempting to do. :-)
If that logic isn't sound, yeah, I guess maybe we can't use this error on Android, and your PR is the way to go!
The Android api (
System.loadLibrary(...)) typically specifies whether the library is available but invalid, or missing.. any idea ifdlopenprovides such capabilities?
Not that I'm aware of.
There was a problem hiding this comment.
Well, I figured if we got all the way past that block, and we failed to load it and initial file didn't exist... it probably doesn't exist? That's what the line I added is attempting to do. :-)
There is one edge case where the given path is not valid because the file is embedded in the apk, so the file exists, but the embedded version doesn't match the architecture the device is running on (arm file for an x86 device).
In that case, we'd get past that block, failed to load, and the so_file_exists would report the given path doesn't exist, but the failure would be because of the mismatched architecture.
If that logic isn't sound, yeah, I guess maybe we can't use this error on Android, and your PR is the way to go!
Yes, sounds like we should go with #86792 for the time-being, especially to avoid introducing a regression in the dev2 build.
Can you approve the other PR?
cc @akien-mga
There was a problem hiding this comment.
Can you approve the other PR?
Sure, I can do that.
|
Superseded by #86792 |
Possible alternative to PR #86792, but which continues to distinguishing between the library not found and being unable to open it.
Currently marked as draft because I haven't had a chance to test it.