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 build system enhancements #2866

Merged
merged 66 commits into from
Feb 18, 2020
Merged

Java build system enhancements #2866

merged 66 commits into from
Feb 18, 2020

Conversation

yuzawa-san
Copy link
Contributor

@yuzawa-san yuzawa-san commented Jan 17, 2020

Description: I cleaned up the CMake files to delegate building and testing to a Gradle build system. Gradle is manages java build/testing, dependencies, and packaging better than CMake. Additionally, all of the Java build output files are in a single output directory. I added a README which explains what JAR files and build output is generated. I relaxed the requirements in JNI loader in OnnxRuntime.java to allow for flexible loading of the library (i.e. from the os libpath or if the developer wants to load the library from some other way). If the developer wants to do that, then they would not use the onnxruntime-lib.jar. I attempted to build out an azure pipeline for the java build (based on the python build). I do not know how to test that. I would assume the maintainers would have to add it. I added spotless plugin to ensure formatting compliance. I selected a stock formatting profile (google's) and reformatted the project in a consistent manner. All changes to java files are formatting except OnnxRuntime.java. Not in this PR: I am playing around with Gradle's maven publishing mechanism.

Motivation and Context

  • Why is this change required? What problem does it solve?

Per my original issue below, I wanted to improve the Java packaging and maintainability. I feel these changes will eventually make way for proper maven distribution.

  • If it fixes an open issue, please link to the issue here.

This is relevant towards #2675

@yuzawa-san yuzawa-san requested a review from a team as a code owner January 17, 2020 21:49
@msftclas
Copy link

msftclas commented Jan 17, 2020

CLA assistant check
All CLA requirements met.

@snnn
Copy link
Member

snnn commented Jan 17, 2020

I'd prefer to remove all the Java related things out of cmake. We don't have to use cmake driving the java compilation. In order to make it part of the CI, we can add one extra step in win-ci-pipeline.yml. And we may directly use maven/gradle to do so. All of our Windows build machines already have gradle installed at %GRADLE_HOME%. You may invoke it directly.

@yuzawa-san
Copy link
Contributor Author

Yeah I moved all of the Java stuff out of the CMake except the piece that compiles the JNI shared object, which needs to stay. I chose the Gradle wrapper for ease of developers, since they won't have to install it and we can lock the version for compatibility purposes. The normal CI process should work just fine since the --build_java flag is in there already, so I guess we'll see shortly once this runs. The pipeline I added was a new one for packaging and it creates an azure artifacts onnxruntime4j and onnxruntime4j_gpu. My vision is that these resulting artifacts can be published to maven for versioned releases and for nightly snapshot releases, just like the python stuff is pushed to PyPi: https://dev.azure.com/onnxruntime/onnxruntime/_release?_a=releases&view=mine&definitionId=1 Regardless how we want to build with respect to CI pipelines, the output is cleaner and all in build/$OS/$CONFIG/java/build.

Copy link
Member

@snnn snnn left a comment

Choose a reason for hiding this comment

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

What's the gradle-wrapper.jar ?

@yuzawa-san
Copy link
Contributor Author

The gradle wrapper jar needs to be there since it is a part of the wrapper mechanism https://docs.gradle.org/current/userguide/gradle_wrapper.html#sec:adding_wrapper see the note in that section

@snnn
Copy link
Member

snnn commented Jan 17, 2020

The gradle wrapper jar needs to be there since it is a part of the wrapper mechanism https://docs.gradle.org/current/userguide/gradle_wrapper.html#sec:adding_wrapper see the note in that section

It's auto-generated right? Then why not generated it in the build?

@snnn
Copy link
Member

snnn commented Jan 17, 2020

Can we avoid using Gradle Wrapper? I'd prefer to not download something from a third-party website during the build, because it adds extra risk.

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 8 pipeline(s).

}
System.load(tempFile.getAbsolutePath());
Copy link
Member

Choose a reason for hiding this comment

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

So, if onnxruntime.dll has any dependencies that are not in the java.library.path, then this call will fail, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not know precisely since I do not work in windows environments, but I believe the load call will load the file and whatever dependency configuration is set in the library file. Linux SO files can have relative, absolute, and rpath dependency locations. DLL's should probably have similar capabilities.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should check the system path for dependent libraries as it's basically a wrapper around the platform's dynamic library loader.

@snnn
Copy link
Member

snnn commented Feb 18, 2020

I'm fixing it.

@snnn
Copy link
Member

snnn commented Feb 18, 2020

/azp run Linux CPU CI Pipeline

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@snnn
Copy link
Member

snnn commented Feb 18, 2020

@Craigacp Do you think this PR is ready to merge?

@Craigacp
Copy link
Contributor

@snnn Assuming all the CI pipelines pass, yes.

@snnn
Copy link
Member

snnn commented Feb 18, 2020

/azp run Linux CPU CI Pipeline,Linux GPU CI Pipeline,Linux GPU TensorRT CI Pipeline,MacOS CI Pipeline,Win CPU x86 CI Pipeline,Windows CPU CI Pipeline,Windows GPU CI Pipeline,Windows GPU TensorRT CI Pipeline

@snnn
Copy link
Member

snnn commented Feb 18, 2020

/azp run Win CPU x64 NoContribops CI Pipeline,MacOS NoContribops CI Pipeline,Linux CPU x64 NoContribops CI Pipeline

@azure-pipelines
Copy link

Azure Pipelines successfully started running 8 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@yuzawa-san
Copy link
Contributor Author

Ok cool the linux build works. The windows ci pipelines timed out because they took a long time for unknown reasons. Something is off given that most of the runs for that job take around 25 minutes. Any ideas? Sadly I do not have access to a windows environment, so any help would be appreciated.

Does the gradle daemon keep running or something weird which prevents that stage from ending? I think my last CI build hung indefinitely after those pybind jobs were built. I can see the Java builds occurring in the logs.

@snnn
Copy link
Member

snnn commented Feb 18, 2020

Something is wrong here. Windows build hangs forever.

@snnn
Copy link
Member

snnn commented Feb 18, 2020

Let me try to disable gradle daemon.

@snnn
Copy link
Member

snnn commented Feb 18, 2020

/azp run Linux CPU CI Pipeline,Linux GPU CI Pipeline,Linux GPU TensorRT CI Pipeline,MacOS CI Pipeline,Win CPU x86 CI Pipeline,Windows GPU CI Pipeline,Windows GPU TensorRT CI Pipeline

@snnn
Copy link
Member

snnn commented Feb 18, 2020

/azp run Win CPU x64 NoContribops CI Pipeline,MacOS NoContribops CI Pipeline,Linux CPU x64 NoContribops CI Pipeline

@snnn
Copy link
Member

snnn commented Feb 18, 2020

This time it should be ok.

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 7 pipeline(s).

Copy link
Member

@snnn snnn left a comment

Choose a reason for hiding this comment

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

Thank you so much!

@snnn snnn merged commit 411b3aa into microsoft:master Feb 18, 2020
@yuzawa-san yuzawa-san deleted the java-gradle branch February 19, 2020 01:22
@Craigacp Craigacp mentioned this pull request Mar 1, 2023
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