-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
New Java build #1193
New Java build #1193
Conversation
@deaktator have you had a chance to look at this? Automake is definitely somewhat painful. It's not used for the master branch build because the relevant configure scripts create artificial differences amongst platforms leading to nasty diffs when merging. It's used for the releases because automake is somewhat more forgiving of system layout differences than a Makefile. Having both is a fair amount of trouble and I've contemplated dropping automake because of that. Do you have what needs to be done worked out for the master branch Makefile? Moving over to CMake has certainly been discussed, and collapsing our build from 3 systems to 1 (there is visual studio also) would be great. Primarily, this is a question of doing. |
Moving over to CMake will probably make the Java build better, but it also looks like it'll make creating yum and debian packages easier too. I wonder if this PR should be accepted and then a new PR to move over to CMake can be opened. This would be a HUGE move for VW though and would require buy in to help from the authors of the original configure system as they'll understand the build process best. |
I chatted with CNTK folks and they also consider moving to CMake…
|
I chatted with Jon Morra about this and CMake seems to have native support for JNI projects which would be nice. This seems like a huge move but it would be nice to get this integrated and to make it so a controlling entity can push out the source packages to various package managers. If we could create source RPMs, etc and specifically brew packages, we could develop on Macs and push to prod on Linux and have the libraries be there via chef, etc. That would make dev and deployment seemingly easier than it is now.
Those are my two cents but I have no experience with CMake so I don't know how difficult all of this is.
|
java/Makefile
Outdated
VWLIBS := -L../vowpalwabbit -l vw | ||
STDLIBS = $(BOOST_LIBRARY) $(LIBS) | ||
JAVA_INCLUDE = -I $(JAVA_HOME)/include | ||
|
||
ifeq ($(UNAME), Linux) | ||
JAVA_INCLUDE += -I $(JAVA_HOME)/include/linux | ||
SHARED_LIBRARY = libvw_jni.so |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you might want to abstract out the library name vw_jni
as well, since System.loadLibrary("vw_jni");
will compose the shared library name based on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, done
java/Makefile
Outdated
@@ -48,7 +52,10 @@ target/vw_jni.lib: $(jni_OBJS) ../vowpalwabbit/main.o ../vowpalwabbit/libvw.a .. | |||
|
|||
-include $(jni_SRCS:.cc=.o) | |||
|
|||
install: target/$(SHARED_LIBRARY) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missed a replacement here: $(LOCAL_LIBRARY)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
[EDIT I just realized that I shouldn't be calling the makefile directly but instead through Everything seems to compile fine. Just two things: the |
@cpdomina thanks so much for this review, this is very thorough. What I think you've stumbled upon is the fact that I don't know how to use autoconf. Could you suggest changes to make the entire Java build work with autoconf? |
@cpdomina On mac, Hi Jon, thanks for improving the Java build. When do you think we can push this change forward? We have several JNI extensions we wanted to contribute and the changes are relying on similar fixes I made for the Java build. It'd be nice if we can merge your changes to avoid future conflicts. |
In order to really put this to bed we have to get the autoconf stuff working within Java and then integrate the Java release process. I can do the second part, but I don't know how to do the first part. If someone wants to branch from this branch and make that work, that'd be amazing. |
I've been wondering of late if the autoconf stuff is worth it. Relatively few people (not including myself) seem to really understand autoconf and we can't use autoconf effectively on the master branch. Maybe we should drop that and just go with Makefiles that we decorate to work on multiple platforms? That would be relatively easy to do, and decreasing the number of build systems by 1 is fairly desirable. |
I think simplifying the build system is a great idea. I think we should consider going to CMake instead of just Makefiles. But whatever decision is made, this PR should be merged in AFTER it is made. |
CMake makes great sense. The core issue there is that no one has had the time/expertise to really dig into it for the last several years. This may change in the next few months as various things are in the works. In the meantime, the only reason why I hesitate to pull the plug on automake is that I don't have a good feel for how much pain that will cause. Maybe we just need to try and find out? |
I think so. I think that it's important that whomever undertakes this project be a core contributor (potentially Microsoft employee) as it will be a very involved move. |
It sounds like this may be the solution to my problem, which is the 8.0.0 maven artifact does not run on Ubuntu 16.04 LTS.
|
This is correct. You need to build it yourself until this issue is
resolved.
…On Tue, Jun 6, 2017 at 12:08 PM Pat Ferrel ***@***.***> wrote:
It sounds like this may be the solution to my problem, which is the 8.0.0
maven artifact does not run on Ubuntu 16.04 LTS.
1. do we think this will solve the "run on Ubuntu 16 problem"?
2. do I need to build the maven artifact? I assume so until this PR is
accepted.
3. I will need to upgrade the vw JNI API from 8.0.0 since the VW class
is no longer instantiated, is this > 8.0.0 JNI API documented anywhere
outside of code?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1193 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAHKG5vY_HiXySDYCZVLhQPWDN_Rq3kzks5sBaOfgaJpZM4L7Nun>
.
|
How do we bump the importance of this? Ubuntu 14.04 is not the typical install these days and a good deal of users on Macs and Linux will be using Java. So I imagine we are not the only people in a bad spot. That spot is choosing to use 8.0 with a deprecated JNI API on olds OSes or maintain our own build for new OSes. We will test this PR on macOS and Ubuntu 16.04. Is there anything else we can do to move this forward? BTW still looking for the 8.3 JNI API docs, I guess javadocs for now? |
As I've stated before, we need to figure out whom is going to spearhead
this rewrite of the installation (if that's what John wants to do).
Yes, the javadoc is the best place to look for now.
…On Wed, Jun 7, 2017 at 9:15 AM Pat Ferrel ***@***.***> wrote:
How do we bump the importance of this? Ubuntu 14.04 is not the typical
install these days and a good deal of users on Macs and Linux will be using
Java. So I imagine we are not the only people in a bad spot. That spot is
choosing to use 8.0 with a deprecated JNI API on olds OSes or maintain our
own build for new OSes.
We will test this PR on macOS and Ubuntu 16.04. Is there anything else we
can do to move this forward?
BTW still looking for the 8.3 JNI API docs, I guess javadocs for now?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1193 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAHKG3x8Qaao9g7AU40jwnEHI1k7CN7kks5sBsy1gaJpZM4L7Nun>
.
|
"this rewrite" is where I get confused. It seems like above you are saying this PR is not done in the "right" way and that what is "right" is still being discussed (having to do with various C build tools) as well as finding an MS person to push this. Anyway, I'm only bumping to show people are interested in the outcome and why. BTW are we wasting time testing this branch? |
I think comments like this are exactly what this branch needs. Let's see
how John wants to handle this.
BTW the rewrite I'm referring to is either moving to CMake, staying on
autoconf and getting rid of all Makefiles or going with just Makefiles and
getting rid of all autoconf.
…On Wed, Jun 7, 2017 at 9:56 AM Pat Ferrel ***@***.***> wrote:
"this rewrite" is where I get confused. It seems like above you are saying
this PR is not done in the "right" way and that what is "right" is still
being discussed (having to do with various C build tools) as well as
finding an MS person to push this.
Anyway, I'm only bumping to show people are interested in the outcome and
why.
BTW are we wasting time testing this branch?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1193 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAHKGzoxgAENK35jlC50dsoa2j205tSUks5sBtZRgaJpZM4L7Nun>
.
|
@JohnLangford I'd like to merge this in now. It removes a lot of confusion regarding the Java build. I can then do a new release to Maven central if you're ok with it. |
+1 I'm using Java and, since we have decided to only use binary releases, am stuck back on 8.0.0. and therefor Ubuntu 14.04. We had to downgrade from 16.04 to get things working. This would be a great help going forward. |
@JohnLangford I just added a change to CSOAA to work with Java predictions. Please check out the changes in csoaa.cc to make sure they make sense to you. I also added a test to show how one would use this. If this is accepted then PR #1244 can probably be squashed in favor of this. |
Ok. It looks good except something is cranky about the travis test. |
@JohnLangford I fixed the failing python tests, but the build is still failing cause of the command
It looks like it's failing on master too so I don't think this branch is causing this problem. If you're ok with it please merge it in, if you want me to investigate this more let me know. |
Ok, merged. Thanks! And thank you for your patience. |
This is an attempt to make the Java build more robust. What I tried to accomplish here is the following.
.jar
. This is easily done by removingjava/src/main/bin/*
.vw_jni
is loadable through a call to System.loadLibrary("vw_jni"). This assumes that the compiled shared library is available on thejava.library.path
.Makefile
in Java to build the shared library in a system dependent manner. I then place the shared library in some location that MIGHT work for the OS's I'm aware of. This is key because if this isn't correct then the Java layer will fail.I have not figured out how to bring the Java environment into the build system that @JohnLangford currently uses. Namely it looks like the
Makefile
s are not included when doing a release, but are instead generated byconfigure
. I'm not familiar with how this works. Someone who is familiar with this should help me figure this out before this PR is accepted.It would be incredibly useful for a variety of people @deaktator, @JohnLangford, and any one else out there that uses the Java interface to test this PR on a variety of OSs to make sure it works.
After this is done we should then consider a unified release system as well for yum, apt-get, brew, and whatever Windows uses.