-
Notifications
You must be signed in to change notification settings - Fork 323
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
Native libraries are searched in lib/polyglot project dir #11874
base: develop
Are you sure you want to change the base?
Conversation
It was a mistake to try to create a different
Polyglot_Utils was loaded by a HostClassLoader for Base pkg. Let's just stick with a single HostClassLoader that will override findLibrary .
Reverting in 1649ddf and 05851f5
|
So that no extraction is done unnecessarily.
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.
Looks very nice. Looking forward to see the CI green.
engine/runtime/src/main/java/org/enso/interpreter/runtime/util/TruffleFileSystem.java
Show resolved
Hide resolved
* @param file | ||
* @return | ||
*/ | ||
def getAbsolutePath(file: F): String |
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.
As usually I have to note: It would be nice to rewrite FileSystem
into Java. Not necessarily in this PR.
What is the size of the reverted back? |
The current printing is not visible on CI
Native image build of
|
Size of |
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.
Ideally we should not be doing anything special in EnsoLibraryFeature
.
assert dir.exists() && dir.isDirectory(); | ||
nativeLibPaths.add(dir.getAbsolutePath()); | ||
var current = System.getProperty("java.library.path"); | ||
RuntimeSystemProperties.register( |
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 necessary neither good idea.
Please remember that NI build runs in build time, while the .so
, .dll
files are about to be located in runtime. The paths are going to be different.
Can you tell me: What was failing and why you think this change is helpful? We don't need anything like this to load .so
for enso_parser
...
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.
For enso_parser
, we have full control over explicit search for the library in Parser$Worker$initializeLibraries - in that method, we try to explicitly load the enso_parser
native library first with System.loadLibrary
, and then with System.load
.
For opencv, we don't have full control. Its native library opencv_java470
is loaded in the static initializer of nu.pattern.OpenCV
with System.loadLibrary("opencv_java470")
. OpenCV
class is used by classes in our std-bits/image
. In JVM mode, all classes from std-bits/image
(classes in package org.enso.image
) are loaded by HostClassLoader
, therefore, also OpenCV
class is loaded by HostClassLoader
, and so, System.loadLibrary("opencv_java470")
delegates to HostClassLoader.findLibrary. In native image, there is no HostClassLoader
, and we have to deal with the System.loadLibrary
call inside OpenCV
differently.
The solution to include path to the native library inside RuntimeSystemProperties
from NI build time was the only solution I could think of. It points to a path like distribution/lib/Standard/Image/0.0.0-dev/polyglot/lib/...
where the native libraries are located. All the tests are currently passing, because we test NI only via cmdline, and we don't try to package the NI inside the AppImage. It is definitely not the most robust solution. I am opened to any alternatives.
TL;DR; Is there any other way how to hook into System.loadLibrary
calls during runtime in NI other than setting java.library.path
via RuntimeSystemProperties
during NI build?
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.
In native image, there is no HostClassLoader,
Really? How comes there is no HostClassLoader
? Of course it cannot load any new classes, but the classloader can/should still be there...
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.
This is an example of the use of HostClassLoader. Still some problems there, but this way we should be able to get HostClassLoader
"into the game".
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.
The build time vs. runtime classpath issue has to be solved.
|
||
- Names for `<os>` are `linux`, `macos`, `windows`. | ||
- Note that for simplicity we omit the versions of the operating systems. | ||
- Names for architectures `<arch>` are `amd64`, `x86_64`, `x86_32`. |
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.
When I buildEngineDistribution
with 05197f5 I see:
Standard/Image/0.0.0-dev/polyglot/lib/amd64/windows/opencv_java470.dll
Standard/Image/0.0.0-dev/polyglot/lib/amd64/linux/libopencv_java470.so
Standard/Image/0.0.0-dev/polyglot/lib/amd64/macos/libopencv_java470.dylib
Standard/Image/0.0.0-dev/polyglot/lib/ARMv8/macos/libopencv_java470.dylib
Standard/Image/0.0.0-dev/polyglot/lib/x86_32/windows/opencv_java470.dll
I don't think the directory names are as they should be! When check my Mac OS X M2:
jshell> System.getProperty("os.name")
$1 ==> "Mac OS X"
jshell> System.getProperty("os.arch")
$2 ==> "aarch64"
amd64
is nice (although x86_64
would be more standard), but I would prefer aarch64 as the architecture for ARMv8
.
I would flatten the directories to "necessary minimum". Instead of:
Standard/Image/0.0.0-dev/polyglot/lib/amd64/windows/opencv_java470.dll
Standard/Image/0.0.0-dev/polyglot/lib/amd64/linux/libopencv_java470.so
Standard/Image/0.0.0-dev/polyglot/lib/amd64/macos/libopencv_java470.dylib
Standard/Image/0.0.0-dev/polyglot/lib/ARMv8/macos/libopencv_java470.dylib
Standard/Image/0.0.0-dev/polyglot/lib/x86_32/windows/opencv_java470.dll
I would just:
Standard/Image/0.0.0-dev/polyglot/lib/opencv_java470.dll
Standard/Image/0.0.0-dev/polyglot/lib/libopencv_java470.so
Standard/Image/0.0.0-dev/polyglot/lib/amd64/libopencv_java470.dylib
Standard/Image/0.0.0-dev/polyglot/lib/aarch64/libopencv_java470.dylib
which matches what I see when downloading Enso IGV Plugin from latest develop build.
assert dir.exists() && dir.isDirectory(); | ||
nativeLibPaths.add(dir.getAbsolutePath()); | ||
var current = System.getProperty("java.library.path"); | ||
RuntimeSystemProperties.register( |
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.
This is an example of the use of HostClassLoader. Still some problems there, but this way we should be able to get HostClassLoader
"into the game".
Fixes #11483
Pull Request Description
Native libraries used via JNI from polyglot Java code can be located in
polyglot/lib
directories in projects. New docs is inenso/docs/polyglot/java.md
Lines 68 to 93 in a9c60a5
Important Notes
Standard.Image
depends onopencv.jar
which contains all native libraries for all platforms:All native libraries have 352 MB, but we only need a single one. For example:
In this PR, sbt extracts all the native libraries from
opencv.jar
and puts them intoStandard/Image/.../polyglot/lib
directory. In subsequent PRs, we may want to drop all the native libraries for different platforms.Native image building modification
EnsoLibraryFeature called during native image build scans all the native libraries inside std libraries, and explicitly registers them for usage in
java.library.path
during runtime:enso/engine/runner/src/main/java/org/enso/runner/EnsoLibraryFeature.java
Lines 92 to 105 in 5a44328
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
or the Snowflake database integration, a run of the Extra Tests has been scheduled.