-
Notifications
You must be signed in to change notification settings - Fork 49
GH-657: Set JNA library loading symbol visibility #658
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
base: main
Are you sure you want to change the base?
Conversation
Added jna library which allows more control over native code loading
This comment has been minimized.
This comment has been minimized.
@@ -69,6 +75,9 @@ private static void loadGandivaLibraryFromJar(final String tmpDir) | |||
final String libraryToLoad = | |||
LIBRARY_NAME + "/" + getNormalizedArch() + "/" + System.mapLibraryName(LIBRARY_NAME); | |||
final File libraryFile = moveFileFromJarToTemp(tmpDir, libraryToLoad, LIBRARY_NAME); | |||
NativeLibrary.getInstance( | |||
libraryFile.getAbsolutePath(), | |||
Collections.singletonMap(Library.OPTION_OPEN_FLAGS, new Integer(RTLD_LAZY | RTLD_GLOBAL))); | |||
System.load(libraryFile.getAbsolutePath()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still necessary if we're using the JNA version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is strictly required with JNA imho.
@@ -69,6 +75,9 @@ private static void loadGandivaLibraryFromJar(final String tmpDir) | |||
final String libraryToLoad = | |||
LIBRARY_NAME + "/" + getNormalizedArch() + "/" + System.mapLibraryName(LIBRARY_NAME); | |||
final File libraryFile = moveFileFromJarToTemp(tmpDir, libraryToLoad, LIBRARY_NAME); | |||
NativeLibrary.getInstance( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't the NativeLibrary instance need to be disposed somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not also leveraging java.library.path
system prop ?
What's Changed
Set the Lazy and Global flags when loading the Gandiva library for JNI so that system functions are visible to the compiled LLVM code.
See also apache/arrow#41497 for much past discussion on this same fix.
Closes #657 and apache/arrow#40839.