Skip to content

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

Merged
saudet merged 4 commits into
bytedeco:masterfrom
junlarsen:load-executable-by-name
Mar 16, 2021
Merged

Enable loader to be able to return an executable path by name#466
saudet merged 4 commits into
bytedeco:masterfrom
junlarsen:load-executable-by-name

Conversation

@junlarsen
Copy link
Copy Markdown
Member

@junlarsen junlarsen commented Mar 11, 2021

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.

Comment thread src/main/java/org/bytedeco/javacpp/Loader.java
Comment thread src/main/java/org/bytedeco/javacpp/Loader.java Outdated
Comment thread src/main/java/org/bytedeco/javacpp/Loader.java Outdated
@saudet
Copy link
Copy Markdown
Member

saudet commented Mar 12, 2021

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
Copy link
Copy Markdown
Member Author

junlarsen commented Mar 12, 2021

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
Copy link
Copy Markdown
Member

saudet commented Mar 12, 2021

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
Copy link
Copy Markdown
Member Author

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
Copy link
Copy Markdown
Member

saudet commented Mar 15, 2021

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

@junlarsen
Copy link
Copy Markdown
Member Author

Sure, LGTM

@saudet saudet merged commit ddcbde5 into bytedeco:master Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants