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

Java bindings using JNA #83

Merged
merged 17 commits into from
Oct 28, 2017
Merged

Conversation

octylFractal
Copy link
Contributor

@octylFractal octylFractal commented May 5, 2017

Java bindings using JNA (Java Native Access) to bind libsweep. Builds using Gradle, can be installed to local Maven repositories with ./gradlew install.

Things to consider:

  • Naming. I didn't name much of this well as I am unfamiliar with C++. I only know enough to write bindings to it :)
  • Adding deployment to Maven Central, as most Java developers would it expect it there.
  • Travis integration (not difficult)
  • Tests

@daniel-j-h
Copy link
Collaborator

Thanks for this!

How do native dependencies work with Maven Central? Do users have to have libsweep.so installed locally already? If we can bundle it, we need to build it for several platforms and and against old libc / libstdc++ for compatibility, no?

Is there the need to bundle the gradle jar in this repository? If so, why? (Same for gradle wrap scripts)

Also I think this PR needs to be updated to master - @dcyoung did some changes to the API wrt. how the motor stabilization needs to be handled etc.

@zugaldia maybe you can have a look at the Java bindings itself.
I think you can talk the best about them.

@octylFractal
Copy link
Contributor Author

How do native dependencies work with Maven Central? Do users have to have libsweep.so installed locally already? If we can bundle it, we need to build it for several platforms and and against old libc / libstdc++ for compatibility, no?

LWJGL is a similar project, it basically stores the natives in JAR files and unpacks them at runtime. This requires a lot of custom library loading, so to keep this project light I would require users to install libsweep.so themselves.

Is there the need to bundle the gradle jar in this repository? If so, why? (Same for gradle wrap scripts)

There is no need to bundle the Gradle wrapper, it just is a standard for Gradle projects to bind the version of Gradle to the project. It also helps people who want to build quickly, as they can simply run the gradlew script instead of installing Gradle entirely.

Also I think this PR needs to be updated to master

I will get to that shortly.

@jabrena
Copy link

jabrena commented May 5, 2017

As @kenzierocks says, include the Gradle wrapper is a good Java/Gradle practice.
https://docs.gradle.org/current/userguide/gradle_wrapper.html

@zugaldia
Copy link

@kenzierocks Excited to see this port happening!

I'm trying to test this branch on a brand new Android app but, unfortunately, the library is currently being built with Java 8 (targetCompatibility = sourceCompatibility = '1.8') which makes it a no-go (eventually I'd like to use this with Android Things). Any reason not to go for targetCompatibility = sourceCompatibility = '1.7' instead?

so to keep this project light I would require users to install libsweep.so themselves.

My preference is for the project to bundle the necessary .so files so that everything works out of the box (what's the weight of including the .so files?). This is for example what I've seen with other projects like OpenCV. Otherwise, I think that the process of adding the native library should be documented as part of this PR.

Which brings me to my last comment: I think that we should include a README.md file with step by step instructions on how to build a basic sample Java/Android app with these bindings.

@jabrena
Copy link

jabrena commented Jun 22, 2017

How is going the Java Support?

@octylFractal
Copy link
Contributor Author

I've switched it back to 7, I didn't need 8 for anything but the method references, which I have reworked in a clever way.

I'm going to start testing out including the SO files, as well as building them with the Gradle build.

@jabrena
Copy link

jabrena commented Jun 23, 2017

Hi @kenzierocks,
I am going to acquire a Unit if I found Stock on an online shop.
When I have the unit, I will inform to help in the testing process.

@octylFractal
Copy link
Contributor Author

So, I don't know much about C library compatibility, but JNA differentiates between Linux (Ubuntu and Arch seem to fall under here), Solaris, FreeBSD, Android, and others. I don't know if (1) We need a build of the library for each of these platforms and (2) if we can build them all in one go. If we only need to make one, I can do that if you give me instructions.

However, the latest commit contains a working prototype for 64-bit Linux. It bundles the .so into a natives JAR, which would then be uploaded to Maven along with the JAR itself. This system works with JNA out of the box, which I had not realized, so it shouldn't be too much of a hit to include it.

@octylFractal octylFractal force-pushed the feature/java-jna branch 2 times, most recently from ff36476 to c73b462 Compare June 23, 2017 08:58
@daniel-j-h
Copy link
Collaborator

See #54 for compatibility

Building these platform dependant libraries is a bit more involved, though. The common way to do it for Linux is to spawn up a Docker container with an old libc, build the library there (for compatibility), and distribute it.

@octylFractal
Copy link
Contributor Author

I've added an arm64 Dockerfile that should build with CentOS 5. However, I assume we would need multiple images to build for each arch that is supported.

The Dockerfile uses the pypa manylinux1 arm64 image as its base, so it should also be easy to roll in the Python build later on.

I'm also working on uploading all of this to Maven Central.

@jabrena
Copy link

jabrena commented Jun 23, 2017

Hi,

Do C compilation support for ARM boards like RaspberryPi?

@octylFractal
Copy link
Contributor Author

I'm not sure I understand what you're asking, C compilers do support ARM but usually only on an ARM computer. I need to make an ARM Dockerfile for that, unless I could get cross-compiling to work.

@jabrena
Copy link

jabrena commented Jun 24, 2017

Hi @kenzierocks, nothing. If C is able to compile on Raspberry Pi or EV3 bricks. I hope to have soon the LIDAR to test.

@cansik
Copy link

cansik commented Jul 18, 2017

It looks like your example is not correct:

device.startScanning();
while (!device.isMotorReady()) {
    Thread.sleep(10);
}

If you start scanning, you are not allowed to check if the motor's are ready. The library seems to work on OSX. Building was a bit more difficult, because you just create a native jar with the linux files. It would be nice if I could add my changes to your PR, but you're repository is not public available.

@octylFractal
Copy link
Contributor Author

It looks like the example might be out of date, as I copied it from one of the other languages. I'll take a look at it.

As mentioned above I only wrote build code to work on Linux. It should have worked on macOS as well but perhaps I missed something.

I'm not sure what you mean by the repo being unavailable, it's not set to private or anything.

@octylFractal
Copy link
Contributor Author

@cansik Could you inform me as to why OSX is not able to build the project as-is? It looks like Travis CI is fine.

@daniel-j-h Is there anything else blocking this, anything you'd like to see improved?

Copy link
Collaborator

@daniel-j-h daniel-j-h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have much experience with Java and the ecosystem, the overall bindings look nicely done from my side. But I would love if someone could review this with a bit more detailed Java knowledge (maybe @zugaldia?).


# EPEL support
yum -y install wget curl
# curl -sLO https://dl.fedoraproject.org/pub/epel/5/x86_64/epel-release-5-4.noarch.rpm
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for bundling the binary rpm file in the repository?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is copied from the Python docker file, so I assume they had their own reasons for doing it (perhaps bandwidth related?). We can probably restore the download.

@daniel-j-h
Copy link
Collaborator

Ping - I really would love to see this merged. We need some eyes here re. code review and best practices. There are also some open questions in the changeset.

@zugaldia
Copy link

I'm not superfamiliar with JNA but what I see in the current PR looks like a good first iteration to me. A few folks have pointed out extra items that probably could be ticketed out for follow up work so that we don't block this binding from landing.

@cansik
Copy link

cansik commented Sep 25, 2017

I currently use that fork for the processing port. It is working well and developed like it should be. Would be nice to see it as the official library.

@octylFractal
Copy link
Contributor Author

Is there anything else blocking? I switched the epel-release file to download.

@daniel-j-h
Copy link
Collaborator

👍 merging in - @kenzierocks you will take care of publishing this in the Java ecosystem / as a maven package I assume?

@daniel-j-h daniel-j-h merged commit 242e656 into scanse:master Oct 28, 2017
@daniel-j-h
Copy link
Collaborator

This is in v.1.3.0 https://github.com/scanse/sweep-sdk/releases 🎉

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