jMonkeyEngine / jmonkeyengine

A complete 3-D game development suite written in Java.
http://jmonkeyengine.org
BSD 3-Clause "New" or "Revised" License
3.84k stars 1.13k forks source link

NativeLibraryLoader wrong libraries path #1965

Closed Ali-RS closed 1 year ago

Ali-RS commented 1 year ago

https://github.com/jMonkeyEngine/jmonkeyengine/blob/375015f14c5f3cbda18302b3aa990e5f023e9da1/jme3-desktop/src/main/java/com/jme3/system/NativeLibraryLoader.java#L123-L200

SEVERE: Uncaught exception thrown in Thread[jME3 Main,5,main]
java.lang.UnsatisfiedLinkError: The required native library 'lwjgl' was not found in the classpath via 'native/linux/liblwjgl.so'. Error message: no lwjgl in java.library.path
    at com.jme3.system.NativeLibraryLoader.loadNativeLibrary(NativeLibraryLoader.java:590)
    at jme3test.helloworld.HelloAudio.simpleInitApp(HelloAudio.java:27)
    at com.jme3.app.SimpleApplication.initialize(SimpleApplication.java:240)
    at com.jme3.system.lwjgl.LwjglWindow.initInThread(LwjglWindow.java:601)
    at com.jme3.system.lwjgl.LwjglWindow.run(LwjglWindow.java:704)
    at java.lang.Thread.run(Thread.java:750)

Thoughts?

stephengold commented 1 year ago

I'm fine with jme3-lwjgl not supporting ARM architectures.

NativeLibraryLoader should be cleaned up so it doesn't register non-existent and/or unnecessary libraries.

pavly-gerges commented 1 year ago

Do you want to clean-up this or just fix-to-work ?

Ali-RS commented 1 year ago

I was thinking of cleaning all the lwjgl3-related stuff and fixing the wrong path for lwjgl2-related stuff.

Do you have another opinion?

Ali-RS commented 1 year ago

I do not know about BulletJme though! Should they be removed?

Does Minie use NativeLibraryLoader for loading natives? Is the bulletjme natives' path correct?

Edit:

Looks like used in JmeDesktopSystem

https://github.com/jMonkeyEngine/jmonkeyengine/blob/375015f14c5f3cbda18302b3aa990e5f023e9da1/jme3-desktop/src/main/java/com/jme3/system/JmeDesktopSystem.java#L288

Ali-RS commented 1 year ago

Also found these usages in jme3-vr

https://github.com/jMonkeyEngine/jmonkeyengine/blob/375015f14c5f3cbda18302b3aa990e5f023e9da1/jme3-vr/src/main/java/com/jme3/system/lwjgl/LwjglContextVR.java#L122-L132

Curious about how does the above code run without exception!? In my case, I am getting UnsatisfiedLinkError when loading lwjgl3-related libraries!

stephengold commented 1 year ago

Does Minie use NativeLibraryLoader for loading natives?

Yes, though seldom explicitly. Most Minie apps simply assume JME will automatically load the native libraries during initialization.

Is the bulletjme natives' path correct?

Yes.

pavly-gerges commented 1 year ago

Do you have another opinion?

I don't know much about the internal design of how jme deals with native libraries apart from just extracting them from the jar file, but i think it's better to keep all the native libraries, as far as we can, inside one directory in the jar file, for example:

Or:

And, if we are forced to have multiple directories for natives, then we should build a new abstract NativeLibraryLoader and extend the utility to create for example, BulletLibraryLoader and LwjglLibraryLoader that would just change the native directory path.

EDIT: For example on the NativeBinaryLoader from the jme-alloc project, i am delegating the name to a LibraryInfo enum class with a basename, if i ever want to load another library, i will just do this in my static initializer without playing around with directories (as there are great chances of writing wrong hardcoded directories):

static {
     LibraryInfo.LIBRARY.setBaseName("mylib"); // set lib name to "libmylib.so"
     NativeBinaryLoader.loadLibrary();
}
Ali-RS commented 1 year ago

but i think it's better to keep all the native libraries, as far as we can, inside one directory in the jar file, for example:

Each library puts them in a different path for example jinput and lwjgl2 put them in jar root while lwjgl3 puts them in <os>.<arch>.org.lwjgl.<libraryname> for example linux.x64.org.lwjgl.opengl.liblwjgl_opengl.so.

pavly-gerges commented 1 year ago

And, if we are forced to have multiple directories for natives, then we should build a new abstract NativeLibraryLoader and extend the utility to create for example, BulletLibraryLoader and LwjglLibraryLoader that would just change the native directory path.

It was best to consider this while designing the code, but idk if we now have time to refactor, or just want to go with the fix-to-work solution, if i ever got enough time, i will redesign this, however, if you would like, you can open another issue at any time to redesign and assign me....

Ali-RS commented 1 year ago

I mean JME NativeLibraryLoader already can handle this. We register a native library by specifying a name for that library and a platform and the path to the file inside the jar then we can load it with NativeLibraryLoader.loadNativeLibrary(libraryName);

https://github.com/jMonkeyEngine/jmonkeyengine/blob/375015f14c5f3cbda18302b3aa990e5f023e9da1/jme3-desktop/src/main/java/com/jme3/system/NativeLibrary.java#L39

I can not see why we need to redesign it.

Ali-RS commented 1 year ago

But we can improve it by fixing these hardcoded hacks:

https://github.com/jMonkeyEngine/jmonkeyengine/blob/375015f14c5f3cbda18302b3aa990e5f023e9da1/jme3-desktop/src/main/java/com/jme3/system/NativeLibraryLoader.java#L577

https://github.com/jMonkeyEngine/jmonkeyengine/blob/375015f14c5f3cbda18302b3aa990e5f023e9da1/jme3-desktop/src/main/java/com/jme3/system/NativeLibraryLoader.java#L666

for example, when it can not find the file inside the jar instead of immediately failing it tries to load it as a system library using System.loadLibrary(libName); which shouldn't in my opinion.

Ali-RS commented 1 year ago

I opened a new issue for that. See https://github.com/jMonkeyEngine/jmonkeyengine/issues/1968

pavly-gerges commented 1 year ago

I can not see why we need to redesign it.

I don't prefer large functions with too many blocks, there are a lot of hidden bugs in them, plus yeah the hardcorded directories and names may fail in some other places (according to the condition, the names may change or written wrongly, so you will have to go over each dependency and refactor it), I haven't glanced the NativeLibraryLoader in details yet, give me sometime to glance it and I will reply back on what should be done from my opinion on the newly created issue, but it indeed looking at it from here, it needs a lot of work.

stephengold commented 1 year ago

Regarding the "bulletjme" native libraries (originally for "jme3-bullet" but now used only by Minie), there was a previous proposal to change their names: #1422

Ali-RS commented 1 year ago

Should I change the names of bulletjme natives?

If you want, I can update my PR to include the architecture suffix (32/64 bit) for bulletjme extracted files. I think we just need to specify it in extractedAsFileName

https://github.com/jMonkeyEngine/jmonkeyengine/blob/375015f14c5f3cbda18302b3aa990e5f023e9da1/jme3-desktop/src/main/java/com/jme3/system/NativeLibrary.java#L44

https://github.com/jMonkeyEngine/jmonkeyengine/blob/375015f14c5f3cbda18302b3aa990e5f023e9da1/jme3-desktop/src/main/java/com/jme3/system/NativeLibraryLoader.java#L92-L96

I think no change will be required on Minie.

Please let me know if you want me to do this.

stephengold commented 1 year ago

Should we change the names of bulletjme natives?

I'd rather not rename physics natives because that would require changes on the Minie side and probably cause some temporary breakage of application builds.

I think no change will be required on Minie.

Minie added those files to the JAR to begin with! So if the names change, I think Minie must be built differently.

At that point, old versions of Minie won't work with new versions of jme3-desktop and new versions of Minie won't work with old versions of jme3-desktop. That's a lot of pain for (what looks to me like) slight benefit.

Extracting natives from the classpath into the working directory is a risky hack. In the long term, JME should probably use a native-library loader analogous with that of LWJGL v3.

Ali-RS commented 1 year ago

Minie added those files to the JAR to begin with! So if the names change, I think Minie must be built differently.

Specifying extractAsName should not require any changes to Minie, I am sure of it. It will still look for libbulletjme inside the jar but will extract it as libbulletjme32 or libbulletjme64.

At that point, old versions of Minie won't work with new versions of jme3-desktop and new versions of Minie won't work with old versions of jme3-desktop.

Using extractAsName it should work fine with any version.

Extracting natives from the classpath into the working directory is a risky hack. In the long term, JME should probably use a native-library loader analogous with that of LWJGL v3.

It first looks working directory if it is writable then chooses that else it looks for user cache directory inside user.home directory, if found, will extract there, if not found then it will extract to userHome/.jme3.

LWJGL v3 extracts its natives inside the temp directory. (on Linux it extracts to /tmp/lwjglali/ for me). Do not know about other operation systems but afaik on linux temp directory gets cleared immediately on system reboot.

stephengold commented 1 year ago

It will still look for libbulletjme inside the jar but will extract it as libbulletjme32 or libbulletjme64.

Ah! I didn't understand that. That should be okay then.

LWJGL v3 extracts its natives inside the temp directory.

What bothers me is JME only extracts to a file if the file doesn't already exist. Ideally, natives should be extracted each time the application is run. Otherwise you might reuse natives from a different platform or version, or (paranoia follows) you might use natives maliciously planted on your filesystem.

Ali-RS commented 1 year ago

What bothers me is JME only extracts to a file if the file doesn't already exist.

Or the file lastModified date is different. https://github.com/jMonkeyEngine/jmonkeyengine/blob/375015f14c5f3cbda18302b3aa990e5f023e9da1/jme3-desktop/src/main/java/com/jme3/system/NativeLibraryLoader.java#L628-L641

Ideally, natives should be extracted each time the application is run.

I like this idea. It is easy to remove the above check so it will extract on every load. This way we won't need to append Architecture Suffix to the extracted file name either. Should I apply this?

Otherwise you might reuse natives from a different platform or version

It is impossible to be reused from a different OS because their format is different (.dll, .so, .dylib), and for different architectures appending Architecture Suffix (32/64/arm32/arm64) to the extracted file name will solve this.

About the different "versions", they will have different lastModified dates so the new one should overwrite the old one.

stephengold commented 1 year ago

Should I apply this?

I think so. The only downside I see is performance.

About the different "versions", they will have different lastModified dates so the new one should overwrite the old one.

Sometimes one actually wants the older version.

Ali-RS commented 1 year ago

Ideally, natives should be extracted each time the application is run.

I decided to not include this change in PR #1967. I think it should be done in a separate PR.

I reopened https://github.com/jMonkeyEngine/jmonkeyengine/issues/1422 to keep track of it.