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

#483 - Javacpp does not export package required by presets #490

Conversation

reckart
Copy link
Contributor

@reckart reckart commented Jun 2, 2021

  • Add package-info with OSGI annotation so that the presets packages is exported for use by e.g. the openblas preset
  • Add m2e lifecycle bindings for the maven-plugin-plugin to avoid error in Eclipse when importing the project

…is exported for use by e.g. the openblas preset

- Add m2e lifecycle bindings for the maven-plugin-plugin to avoid error in Eclipse when importing the project
@reckart
Copy link
Contributor Author

reckart commented Jun 2, 2021

I made the change according to the pointer provided by @timothyjward

@reckart
Copy link
Contributor Author

reckart commented Jun 2, 2021

Building the package, I can now see this in the MANIFEST.MF:

Export-Package: org.bytedeco.javacpp;uses:="org.bytedeco.javacpp.annot
 ation,org.bytedeco.javacpp.presets";version="1.5.6",org.bytedeco.java
 cpp.annotation;version="1.5.6",org.bytedeco.javacpp.indexer;uses:="or
 g.bytedeco.javacpp";version="1.5.6",org.bytedeco.javacpp.presets;uses
 :="org.bytedeco.javacpp,org.bytedeco.javacpp.annotation";version="1.5
 .6",org.bytedeco.javacpp.tools;uses:="javax.management";version="1.5.
 6"

So it looks like the org.bytedeco.javacpp.presets package is exported now.

@saudet
Copy link
Member

saudet commented Jun 3, 2021

@timothyjward Does this look alright to you?

@saudet saudet merged commit d943c8f into bytedeco:master Jun 3, 2021
@reckart
Copy link
Contributor Author

reckart commented Jun 7, 2021

@agibsonccc if you do a follow-up release to DL4J 1.0.0-M1 soon, it would be great if this change could make it in.

@saudet
Copy link
Member

saudet commented Jun 7, 2021

It's probably not going to be enough. I think we need to add those package-info.java and what not to the presets themselves first.

@reckart
Copy link
Contributor Author

reckart commented Jun 7, 2021

At least in our case, we pack the presets (e.g. openblas.jar) into our own OSGI bundle which then imports the packages from JavaCPP.

We do not add JavaCPP directly into our bundle because it already is a bundle and bundling bundles doesn't really seem to be a good idea.

Do you plan to turn the presets JARs into OSGI bundles as well?

@reckart reckart deleted the bugfix/483-Javacpp-does-not-export-package-required-by-presets branch June 7, 2021 17:19
@agibsonccc
Copy link

@reckart I don't see why not. It would depend on @saudet 's release schedule. I have 1 pending PR that depends on a javacpp upgrade that I can't merge yet, but would be happy to otherwise. A javacpp version bump doesn't usually cost us anything.

@saudet
Copy link
Member

saudet commented Jun 7, 2021

At least in our case, we pack the presets (e.g. openblas.jar) into our own OSGI bundle which then imports the packages from JavaCPP.

We do not add JavaCPP directly into our bundle because it already is a bundle and bundling bundles doesn't really seem to be a good idea.

Do you plan to turn the presets JARs into OSGI bundles as well?

Ideally, yes, contributions are welcome. If you're bundling everything anyway, you should also bundle JavaCPP and ignore it's own bundle though.

@reckart
Copy link
Contributor Author

reckart commented Jun 8, 2021

We had trouble integrating JavaCPP into the bundle in the past. As far as I remember, the Maven bundle plugin somehow merged some of the OSGI metadata from JavaCPP into the main bundle metadata which messed things up. At that point, we decided to handle JavaCPP separately. Also because in perspective, we'd have multiple bundles with native libraries and they should probably all share the same JavaCPP because as far as I understand Java native library loading should be centralized under a single classloader to avoid issues with attempts of loading the same native library multiple times. Not sure if I explain this well - it's a bit if a difficult terrain.

So having everything in one bundle is only a temporary convenience for us that hopefully we'll move away from sooner or later.

@timothyjward
Copy link
Contributor

We had trouble integrating JavaCPP into the bundle in the past. As far as I remember, the Maven bundle plugin somehow merged some of the OSGI metadata from JavaCPP into the main bundle metadata which messed things up. At that point, we decided to handle JavaCPP separately. Also because in perspective, we'd have multiple bundles with native libraries and they should probably all share the same JavaCPP because as far as I understand Java native library loading should be centralized under a single classloader to avoid issues with attempts of loading the same native library multiple times. Not sure if I explain this well - it's a bit if a difficult terrain.

So having everything in one bundle is only a temporary convenience for us that hopefully we'll move away from sooner or later.

This is correct - you should only embed things that are designed to be completely internal to your bundle/module (much like using the maven-shade-plugin). In this case there are multiple different bundles using JavaCPP and (unless DL4J has changed its structure quite a bit) the fact that those modules are using JavaCPP is not an implementation detail.

As a general rule, if a bundle which embeds some packages exports those packages then it probably shouldn't be embedding them.

@agibsonccc
Copy link

@timothyjward not a lot has changed. We're still sitting on your original pull request that adds osgi support and I think the same blockers are still there.

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.

4 participants