Skip to content
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

getResource changed in Loader.findLibrary #276

Closed
wants to merge 5 commits into from
Closed

getResource changed in Loader.findLibrary #276

wants to merge 5 commits into from

Conversation

HGuillemet
Copy link
Contributor

cls.getResource only look for resources in the module containing cls.
Changed to cls.getClassLoader().getResource which looks everywhere.
This change drops the package prefix in the resource searched for, so in the jar of native libs, the libs must be located in /platform/ instead of /org/bytedeco/javacpp/lib/platform/.

The jar of native libs must be an open module for getResource to succeed.
In my latest PR of javacpp-preset it's an automatic module, so open to all.

@saudet
Copy link
Member

saudet commented Dec 5, 2018

Thanks! Is there no way to keep the package name though? I'd rather not make changes just for the sake of making changes...

@HGuillemet
Copy link
Contributor Author

javacpp-side you can very well extract the package from cls and add it as a prefix to the resource.
presets-side the content of the jar of native lib is now generated in its own directory, using the outputDirectory config of the javacpp compiler. With this setting the package prefix is dropped too. So that was the easiest solution.

@HGuillemet
Copy link
Contributor Author

Anyway this is a temporary solution allowing to use javacpp from a modular app. But jlink still won't work. As discussed on gitter i believe the best solution is to turn the jar of libs as a named module that "provides" native libs as a service.

@saudet
Copy link
Member

saudet commented Dec 6, 2018

It looks like we wouldn't need to make them into modules, whatever the path is, as long as it's not a valid package name, for example, containing "-" as with the platform names:

  • If a resource's effective package name is not a valid Java language
    package name (e.g., "META-INF.foo.bar") [4] then the resource can
    be located by code in any module.

http://mail.openjdk.java.net/pipermail/jpms-spec-experts/2016-September/000392.html

Could you test this out?

@HGuillemet
Copy link
Contributor Author

Do you mean adding the jar to the classpath instead of the module path ? Anything in the module path cannot access what is in the classpath, so this won't work if an application is launched with module path.
Any jar in the module path is made a module, either automatic or explicit.
A resource in a module can be encapsulated or not. A not-encapsulated resource can be located and read by all other modules. An encapsulated resource needs an "open" declaration in the module path to be able to be located by other modules.
A resource is encapsulated if its path (effective package) correspond to a java package in the module.
What you are quoting is that a resource which path contains characters that are not valid in a java identifier cannot be encapsulated, so will always be reachable by other modules.

In our case the .so are not encapsulated, because their path can contain "-" and because their path does not correspond to a java package in the jar. But also because the jar of libs is currently an automatic module, which "opens" all its resources by default.

@saudet
Copy link
Member

saudet commented Dec 6, 2018 via email

@saudet
Copy link
Member

saudet commented Dec 6, 2018

The trick to get these kinds of resources included in images created with jlink seems to be with --patch-module: https://stackoverflow.com/questions/48408454/java-9-generating-a-runtime-image-with-jlink-using-3rd-party-jars

@HGuillemet
Copy link
Contributor Author

Sounds about right

Thank you master :)

so the classifier JARs should be fine as they are without adding anything about modules right?

The Jar generated by my PRs ? They are right for running with a modular app, not for jlink since the jar of native libs is still an automatic module.

Have you already tested them with jlink?

Just made a test to be sure:

jlink --module-path /home/hg/javacpp/target/javacpp.jar:/home/hg/javacpp-presets/opencv/target/opencv-linux-x86_64.jar:/home/hg/javacpp-presets/opencv/target/opencv.jar:t.jar --add-modules test --output rt
Error: automatic module cannot be used with jlink: org.bytedeco.javacpp.opencv.linux.x86_64 from file:///home/hg/javacpp-presets/opencv/target/opencv-linux-x86_64.jar

@saudet
Copy link
Member

saudet commented Dec 6, 2018 via email

@HGuillemet
Copy link
Contributor Author

A module-info.java like this one works:

open module org.bytedeco.javacpp.opencv.linux.x86_64 {
  requires transitive org.bytedeco.javacpp.opencv;
} 

Resources are found, provided this module is added to the "requires" of the application.
This will work whatever directory/"package name" of the jar the libs are located in.
So there is two options:

  • still add a java class in the jar, implementing a service having methods like getArch() getSystem() and getLibrary(String name) so that the Loader can request the classes implementing the "opencv native library providing service", identify the one for the right system/arch, and get the library itself, without using getResource. The library may be located anywhere in this case, at the root of the jar for instance.
  • continue with getResource(), in this case the libraries must lie in a directory that uniquely identifies the right library. The current /Linux-x86_64 might do it if we suppose that across all presets, for a same system/arch, the lib names are unique. Else add a prefix in the path identifying the presets.

@saudet
Copy link
Member

saudet commented Dec 6, 2018 via email

@HGuillemet
Copy link
Contributor Author

You still need to replace cls.getResource by cls.getClassLoader().getResource for option 2, and adjusting the path if you think it's necessary. And for option 1 of course.

@saudet
Copy link
Member

saudet commented Dec 7, 2018

I thought cls.getResource() would still work, it doesn't? If not, let's adjust it so it works the same as cls.getResource() with the full path and all, basically what findResources() already does.

@HGuillemet
Copy link
Contributor Author

cls.getResource look for the resource in cls module only.

@saudet
Copy link
Member

saudet commented Dec 7, 2018

Ok, so let's call Loader.findResources() and use the first URL it returns. Looking at the source code of the JDK, that's basically what it does inside sun.misc.URLClassPath.

@saudet
Copy link
Member

saudet commented Dec 7, 2018

Actually, let's fall back on Loader.findResources() only when cls.getResource() returns null. Apparently ClassLoader.getResources() isn't able to load resources that are part of the module of the class. And let's put that logic in a new Loader.findResource() method. Sounds good?

@HGuillemet
Copy link
Contributor Author

HGuillemet commented Dec 7, 2018

Apparently ClassLoader.getResources() isn't able to load resources that are part of the module of the class.

What makes you think that ? ClassLoader.getResource doesn't know about the class that were used to get the class loader from. And the class loader is not related to a particular module.

@saudet
Copy link
Member

saudet commented Dec 7, 2018

Because that's what Alan Bateman, the architect of JPMS, said in one of the messages linked above.

@HGuillemet
Copy link
Contributor Author

Ok, what he says is that cls.getResource will find the resource in cls Module even if the resource is encapsulated, while ClassLoader.getResource will only find the not encapsulated resources, be them in cls module or elsewhere.

@saudet
Copy link
Member

saudet commented Dec 7, 2018

So, anyway would you be OK with a new method like Loader.findResource()?

@HGuillemet
Copy link
Contributor Author

Yes, returning the first matching resource is enough, we don't need an array of url like returned by findResources.
But calling cls.getResource first seems useless to me. It will only be useful if the libs are in the javacpp module (so running a modular application with an uberjar transformed somehow into a named module) and encapsulated (which will never happen if you keep the '-' in the path).
Concerning the possible use of such findResource for anything else than loading library... I don't know.

@saudet
Copy link
Member

saudet commented Dec 7, 2018

For backward compatibility we need it anyway.

@HGuillemet
Copy link
Contributor Author

You can keep it to be sure, the overhead is probably negligible, but if you keep the same path, there is no reason ClassLoader.getResource doesn't find what cls.getResource does.

@saudet
Copy link
Member

saudet commented Dec 8, 2018 via email

@HGuillemet
Copy link
Contributor Author

What I'm tying to say is that if you replace:

url = cls.getResource('/Linux-x86_64/libopencv_core.so');

by something like:

url = cls.getClassLoader().getResource(cls.getPackageName().replace('.','/')+'/Linux-x86_64/libopencv_core.so');

I believe that the behavior will be identical in a not-modular context, so no backward compatibility issue.

@saudet
Copy link
Member

saudet commented Dec 8, 2018

It's fine for that case, yes, but it's not the only case.

saudet added a commit that referenced this pull request Dec 10, 2018
…` to work around issues with JPMS ([pull #276](#276))

 * Enhance `Loader.findResources()` with `Class.getResource()` and search among parent packages
 * Take shortest common package name among all user classes for the default output path of `Builder`
@saudet
Copy link
Member

saudet commented Dec 10, 2018

I've included that addition among other things in the latest commit. With these changes, the output directory for all the presets gets evaluated to /org/bytedeco/javacpp/ whether we set the "global" value or not, and the Loader can now look up resources in parent packages as well, so even if one sets the output directory manually to, for example, /linux-x86_64/, it will be able to find them there too. But the default now works fine, so there shouldn't be a need to set it anymore. Let me know if you encounter any issues with these changes though! Thanks

@saudet saudet closed this Dec 10, 2018
@HGuillemet
Copy link
Contributor Author

Works with my modular opencv branch.

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.

None yet

2 participants