Enable loader to be able to return an executable path by name

[junlarsen]

This patch implements Loader.loadExecutable() which will load an executable from a preset class by name.

To target the llvm-dis executable from the LLVM presets, we can use

String llvmDis = Loader.load(org.bytedeco.llvm.program.llvm.class, "llvm-dis");
new ProcessBuilder( ... );

We originally discussed having Loader.load(cls, executable) but it's not an easy job to introduce a new argument into Loader.load without breaking backwards compatibility and I think loadExecutable communicates intent better than yet another overload for Loader.load.

The functionality of Loader.load is still the same and none of the old method signatures have changed. It will still return the first executable in the list, while Loader.loadExecutable will return a specific executable if it exists. This is pretty much a MVP so let me know if there's anything you'd like to change.

[saudet]

We originally discussed having Loader.load(cls, executable) but it's not an easy job to introduce a new argument into Loader.load without breaking backwards compatibility and I think loadExecutable communicates intent better than yet another overload for Loader.load.

Why is it not an easy job? I think something like this would work:

diff --git a/src/main/java/org/bytedeco/javacpp/Loader.java b/src/main/java/org/bytedeco/javacpp/Loader.java
index 57a9d882..781ccd3a 100644
--- a/src/main/java/org/bytedeco/javacpp/Loader.java
+++ b/src/main/java/org/bytedeco/javacpp/Loader.java
@@ -1147,6 +1147,12 @@ public class Loader {
     public static String load(Class cls) {
         return load(cls, loadProperties(), Loader.pathsFirst);
     }
+    public static String load(Class cls, String executable) {
+        return load(cls, loadProperties(), Loader.pathsFirst, executable);
+    }
+    public static String load(Class cls, Properties properties, boolean pathsFirst) {
+        return load(cls, properties, pathsFirst, null);
+    }
     /**
      * Loads native libraries associated with the given {@link Class} and initializes it.
      *
@@ -1160,7 +1166,7 @@ public class Loader {
      * @see #findLibrary(Class, ClassProperties, String, boolean)
      * @see #loadLibrary(URL[], String, String...)
      */
-    public static String load(Class cls, Properties properties, boolean pathsFirst) {
+    public static String load(Class cls, Properties properties, boolean pathsFirst, String executable) {
         Class classToLoad = cls;
 
         if (!isLoadLibraries() || cls == null) {
@@ -1276,8 +1282,8 @@ public class Loader {
                         }
                     }
                 }
-                for (String executable : executables) {
-                    String[] split = executable.split("#");
+                for (String e : executables) {
+                    String[] split = e.split("#");
                     filename = prefix + split[0] + suffix;
                     String filename2 = split.length > 1 ? prefix + split[1] + suffix : null;
                     for (int i = extensions.length - 1; i >= -1; i--) {
@@ -1307,7 +1313,11 @@ public class Loader {
             }
         }
         if (executables.size() > 0) {
-            return executablePaths.size() > 0 ? executablePaths.get(0) : null;
+            int i = 0;
+            if (executable != null && (i = executables.indexOf(executable)) < 0) {
+                return null;
+            }
+            return executablePaths.size() > 0 ? executablePaths.get(i) : null;
         }
 
         int librarySuffix = -1;

I'm not sure I'd want to refactor this just to add a Loader.loadExecutable() in there. Of course, we should think of something cleaner for JavaCV 2.0 or something, but just for a patch kind of thing like that, hum...

[junlarsen]

Sure, we can merge both actions into Loader.load() and it should work. My initial concern was breaking function signatures but that should be avoidable. We'll just add an overload which calls the main implementation with an extra null parameter.

    /** Returns {@code load(cls, properties, pathsFirst, null)}. */
    public static String load(Class cls, Properties properties, boolean pathsFirst) {
        return load(cls, properties, pathsFirst, null);
    }

There is the possibility that an executablePath isn't found for each executable (which means the list of executablePaths is not updated). This would shift the index for the executablePaths off by one, breaking the mapping between executable name index and executable path index. For this reason I used a Map instead of a List to keep track of the relationship between executable name and path. What do you think?

The Loader.load() implementation may return the library path instead of an executable. Would there not be an edge case where a class has no executables and an unexpected library path is returned?

[saudet]

Sure, we can merge both actions into Loader.load() and it should work. My initial concern was breaking function signatures but that should be avoidable. We'll just add an overload which calls the main implementation with an extra null parameter.

    /** Returns {@code load(cls, properties, pathsFirst, null)}. */
    public static String load(Class cls, Properties properties, boolean pathsFirst) {
        return load(cls, properties, pathsFirst, null);
    }

There is the possibility that an executablePath isn't found for each executable (which means the list of executablePaths is not updated). This would shift the index for the executablePaths off by one, breaking the mapping between executable name index and executable path index. For this reason I used a Map instead of a List to keep track of the relationship between executable name and path. What do you think?

Sounds good! However, we should use a LinkedHashMap so that we still have the equivalent of a getFirst().

The Loader.load() implementation may return the library path instead of an executable. Would there not be an edge case where a class has no executables and an unexpected library path is returned?

In that case the argument is simply ignored, right? We could throw an IllegalArgumentException or something though.

[junlarsen]

Sounds good! However, we should use a LinkedHashMap so that we still have the equivalent of a getFirst().

Should work now.

In that case the argument is simply ignored, right? We could throw an IllegalArgumentException or something though.

IllegalArgumentException is now thrown when the size of executables for the class is 0 and executable is not null.

[saudet]

Thanks! I've refined a few last things. Let me know if that looks good.

[junlarsen]

Sure, LGTM