NativeLibraryLoader uses some hardcoded hacks

[Ali-RS]

While dealing with NativeLibraryLoader in another issue I notices NativeLibraryLoader uses some hardcoded hacks that need to be fixed.

for example, in the below code, 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.

jmonkeyengine/jme3-desktop/src/main/java/com/jme3/system/NativeLibraryLoader.java

Lines 573 to 600 in 375015f

	if (url == null) {
	// Attempt to load it as a system library.
	String unmappedName = unmapLibraryName(fileNameInJar);
	try {
	// XXX: HACK. Vary loading method based on library name.
	// lwjgl and jinput handle loading by themselves.
	if (!name.equals("lwjgl") && !name.equals("jinput")) {
	// Need to unmap it from library specific parts.
	System.loadLibrary(unmappedName);
	logger.log(Level.FINE, "Loaded system installed "
	+ "version of native library: {0}", unmappedName);
	}
	} catch (UnsatisfiedLinkError e) {
	if (isRequired) {
	throw new UnsatisfiedLinkError(
	"The required native library '" + unmappedName + "'"
	+ " was not found in the classpath via '" + pathInJar
	+ "'. Error message: " + e.getMessage());
	} else if (logger.isLoggable(Level.FINE)) {
	logger.log(Level.FINE, "The optional native library ''{0}''" +
	" was not found in the classpath via ''{1}''" +
	". Error message: {2}",
	new Object[]{unmappedName, pathInJar, e.getMessage()});
	}
	}
	return;
	}

or here, for lwjgl and jinput, it does not load them actually just extracts them and sets the path in a system property to be loaded later on by the library itself.

jmonkeyengine/jme3-desktop/src/main/java/com/jme3/system/NativeLibraryLoader.java

Lines 666 to 678 in 375015f

	// XXX: HACK. Vary loading method based on library name.
	// lwjgl and jinput handle loading by themselves.
	if (name.equals("lwjgl") || name.equals("lwjgl3")) {
	System.setProperty("org.lwjgl.librarypath",
	extractionDirectory.getAbsolutePath());
	} else if (name.equals("jinput")) {
	System.setProperty("net.java.games.input.librarypath",
	extractionDirectory.getAbsolutePath());
	} else {
	// all other libraries (openal, bulletjme, custom)
	// will load directly in here.
	System.load(targetFile.getAbsolutePath());
	}

[Ali-RS]

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

Originally posted by @Scrappers-glitch in #1965 (comment)

[Ali-RS]

hardcorded directories

Do you mean the native path in the library jar?

But that is inevitable if we want to support customer library loading. I mean how else we can load a native if we do not know the path to file inside the jar?
The best we can do is to load them from a config file instead which I think not worth it because changing to the native path in a library is rare anyway and some libraries like lwjgl3 have their own loaders anyway. I mean, someone still can load the native library info from a config file in their app and load them via NativeLibraryLoader at runtime, it is totally possible.

[Ali-RS]

Here is a preview of how I workaround those hacks:

Ali-RS@a23467d

Please let me know what do you think about it.

[pavly-gerges]

The best we can do is to load them from a config file instead which I think not worth it because changing to the native path in a library is rare anyway and some libraries like lwjgl3 have their own loaders anyway. I mean, someone still can load the native library info from a config file in their app and load them via NativeLibraryLoader at runtime, it is totally possible.

I always prefer simple stuff, just create an enum NativeBinary or NativeLibrary for example outside of the NativeLibraryLoader, add some constants representing the path and the name of the libraries and that is it, by doing this, you will be able to use those constants to refer to your libraries (as indicated by OOP) without using the actual hardcoded paths in your actual code; because if you keep copying hardcoded stuff, you will need to search for all files/classes that have this hardcoded stuff and update them when it's time for refactor or a binary has been renamed, but with just some constants, you can go over the NativeBinary and change those when anything changes and the code becomes more clean, also if someone for ever reason needs to access those libraries, they shouldn't have to know the hardcoded path, no, they will deal with objects referring to those libraries...

[pavly-gerges]

In addition, there are a lot of formatting errors, for example: the static initializers should be at the top of the class, it's the first thing ever to run in a class; it doesn't even matter for the compiler, or the output bytecode, but it matters to the reader...

EDIT:
The static stuff runs in an ordinal matter and so do static iniitalizers so we should ensure this, that's the way we write them in code is the ordinal way they should run in.

[pavly-gerges]

There are some stream IO code that could be closed cleanly using a try with resources block or by distributing the code into smaller private static functions to better handle the stream resources...

[Ali-RS]

just create an enum NativeBinary or NativeLibrary for example outside of the NativeLibraryLoader, add some constants representing the path and the name of the libraries and that is it

Got that

As always, thanks so much for the feedback, I will study them and try to solve them. I will submit another preview once I figured them out.

[pavly-gerges]

If you still want more illustrations on my idea, you can reference the NativeDynamicLibrary and the LibraryInfo, the NativeDynamicLibary appends a path to the native binary defined by the LibraryInfo, so if i want to load another library from the NativeBinaryLoader, i will just either change the LibraryInfo#LIBRARY value in case i want to change the basename without altering the path, or change the NativeDynamicLibrary values if i want to alter the whole path and ofc you can select better names for these utilities; for example: DynamicLibrary and LibraryBasename.

This is very simple, but effective on long run, if you have to maintain different types of native libraries from a single utility without playing around with hardcoded absolute or even relative paths !

[Ali-RS]

I want to create an enum NativeLibraries and define all NativeLibrary used by JME there.

[pavly-gerges]

I want to create an enum NativeLibraries and define all NativeLibrary used by JME there.

Looks good to me.