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] Setting Java module name to com.microsoft.onnxruntime.extensions #730

Merged
merged 1 commit into from
May 28, 2024

Conversation

Craigacp
Copy link
Contributor

The Java module system (introduced in Java 9) requires that all modules on the module path have unique names, and the current name for ORT-extensions collides with ORT as both use com.microsoft.onnxruntime. This PR changes the ORT-extensions module name to com.microsoft.onnxruntime.extensions to remove the collision.

If they are colliding then work needs to be done in the user's build system to pull ORT-extensions off from the module path and put it on the classpath which can be irritating to do particularly if it's consumed by a third party dependency which might not be doing a module aware build.

@@ -113,7 +113,7 @@ if (cmakeBuildDir != null) {
// Overwrite jar location
task allJar(type: Jar) {
manifest {
attributes('Automatic-Module-Name': project.group,
attributes('Automatic-Module-Name': "com.microsoft.onnxruntime.extensions",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this change or should project.group be set to com.microsoft.onnxruntime.extensions? Not sure what is preferred.

Does build-android.gradle need updating? It doesn't seem to set Automatic-Module-Name. That Android package is used in example apps without any issues AFAIK.

https://github.com/microsoft/onnxruntime-inference-examples/blob/1c2199208a9084ec0e4ceb188cb5192608d75938/mobile/examples/super_resolution/android/app/build.gradle#L49
https://github.com/microsoft/onnxruntime-inference-examples/blob/1c2199208a9084ec0e4ceb188cb5192608d75938/mobile/examples/super_resolution/android/app/src/main/java/ai/onnxruntime/example/superresolution/MainActivity.kt#L4

Copy link
Contributor Author

@Craigacp Craigacp May 28, 2024

Choose a reason for hiding this comment

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

project.group sets the group name for Maven Central, and that can be com.microsoft.onnxruntime. Maven has both a group and artifact name, so it's fine for it to be group com.microsoft.onnxruntime and artifact onnxruntime-extensions. The namespacing will keep things together on Maven - https://central.sonatype.com/namespace/com.microsoft.onnxruntime. Maven Central's name doesn't matter for the Java platform module check.

I don't believe Android cares about Java modules, but anyway ORT-extensions targets a version of Android which only supports Java 8 and modules came in Java 9. Adding the automatic module name wouldn't cause problems if added to the build-android.gradle as it'll ignore things in the manifest it doesn't care about, but it probably wouldn't do anything either.

The problem in Java occurs if the user has modularized their application and then tries to run it with both the ORT and ORT-extensions jars as dependencies. As ORT-extensions (for non-Android Java) isn't on Maven Central yet there aren't too many people doing that, but it'll likely cause problems when it gets there. For example the stable diffusion example I wrote (https://github.com/oracle/sd4j) won't compile properly when migrated to use ORT-extensions from Maven Central without this change.

@skottmckay
Copy link
Contributor

Something is up with the connection to the CI. I manually ran onnxruntime-extensions.CI here and that passed.

Once it passed, the button to check in the PR became enabled, although it still claims to be waiting on the status for the CI.

@skottmckay skottmckay merged commit fcf28fe into microsoft:main May 28, 2024
2 checks passed
@Craigacp Craigacp deleted the java-module-fix branch May 28, 2024 13:19
@Craigacp
Copy link
Contributor Author

Is it possible to get this in to the 0.11.0 release or has that already happened?

@wenbingl
Copy link
Member

Is it possible to get this in to the 0.11.0 release or has that already happened?

sorry, the release team are already in publishing. Let me see if we can do a next release or a patch release shortly.

@Craigacp
Copy link
Contributor Author

Ok, thanks for the update.

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.

3 participants