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

[Proposal] Include libdisni.so into assembly jar #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[Proposal] Include libdisni.so into assembly jar #17

wants to merge 1 commit into from

Conversation

petro-rudenko
Copy link
Contributor

@petro-rudenko petro-rudenko commented Jan 12, 2018

This will include libdisni into assembly jar, thus not require to mess with LD_LIBRARY_PATH, etc, allowing to have different disni versions for different components. Making PR for discussion if everything's OK will submit to gerrit.

This approach used by many popular libraries with jvm/jni. E.g. xgboost

cc: @yuvaldeg

@yuvaldeg
Copy link
Collaborator

Hi Peter,
Actually, this was already discussed in the past. We have concluded that the cleanest way is to use the approach used in https://github.com/fusesource/leveldbjni, which is to pack multiple versions of libdisni.so along with the main jar file and invoke the correct one at runtime. This way when deployed through Maven Central, one can avoid compiling and installing libdisni.so altogether, without any concern about the specifics of the target machine.

@patrickstuedi
Copy link
Member

I remember we have discussed the leveldbjni approach, not sure we came to a conclusion though. I think the approach presented by this pull request is not bad as it automatically builds libdisni when building the jar, but does not include binaries into a jar.

@petro-rudenko
Copy link
Contributor Author

It includes libraries into jar: https://github.com/zrlio/disni/pull/17/files#diff-6c01060f17a19807e7ef9d8221b3e43aR51
In assembly jar you'll see /lib folder with binaries. Need to think maybe to do cross compilation for different platforms + something like this:
https://github.com/adgaudio/vowpal_wabbit/blob/0f3c11c20d3f68bb9674ae5945c2f4b07818a4a9/java/src/main/java/vw/jni/NativeUtils.java#L165

@patrickstuedi
Copy link
Member

Adding a shared library to the assembly jar and load it from there may work for small applications, any larger system anyway would need a more systematic approach to managing the CLASSPATH and LD_LIBRARY_PATH. I'm not so convinced adding to the jar is a good idea.

@petro-rudenko
Copy link
Contributor Author

In assembly jar there's only libdisni binary, all other libraries (spdk, dpdk, etc.) should be managed by LD_LIBRARY_PATH. The issue if use disni as dependency library need to have 1:1 correspondence to libdisi. E.g. 1 library may depend from disni-1.3 another one from disni-1.4-SNAPSHOT (with ODP MR prefetch). In this case need to set separate LD_LIBRARY_PATH so each corresponds to its own libdisni. If libdisni would be in assembly jar, then just to add maven dependency, and no compilation or setting separate LD_LIBRARY_PATH needed

@patrickstuedi
Copy link
Member

Understand there is a dependency here between disni and libdisni, but that can be solved by having both libraries export versions and have disni check if the loaded libdisni has the right version. Adding it to the assembly which then copies is to some local folder which at runtime may or may not exist on the given platform, combined with other potential runtime problems like the library being incompatible with the platform, or the library already being in the LD_LIBRARY of the system (e.g., Crail) making it unclear which version gets loaded, etc.., all this gives me a gut feeling that using this approach in a real large system creates all sorts of problem. It may at best be a solution for small hello world applications.

@petro-rudenko
Copy link
Contributor Author

No need to extract to some folder. It was just an example. Totally possible to read from jar itself. Here's for example netty (not hello world app):
https://github.com/netty/netty/blob/4.1/transport-native-epoll/pom.xml#L171

Apache Ignite:
https://github.com/apache/ignite/blob/3271d75fca41db9ae186047c08f422f6b6f66498/modules/core/src/main/java/org/apache/ignite/internal/util/ipc/shmem/IpcSharedMemoryNativeLoader.java#L237

@PepperJo
Copy link
Contributor

I also think that generally the jar is not a good place for binaries besides .class files. However I understand that from a usability perspective this is nice if it works like advertised (every platform etc.). We should put some effort into creating package builds for the big distros, e.g. debs and rpms and see if we can get this into their repositories. I believe the chance is higher now since it is used by an Apache project.

@asqasq
Copy link
Member

asqasq commented Jan 18, 2018

I would prefer not to include binaries in jars. As far as I remember, we did not conclude to include libdisni in the jar.

A much cleaner solution is to create packages like deb or rpm (as Jonas said). This will lead to less problems than trying to include binaries for different architectures and platforms.
Given that we have to deal with LD_LIBRARY_PATH anyways, if we use other libraries (like dpdk), I think it is fine to use LD_LIBRARY_PATH for libdisni as well. Correct versioning and version dependencies we have to deal with in any case.

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.

5 participants