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

Separate the Maven Plugin from JavaCPP for OSGi #328

Merged
merged 1 commit into from
Aug 21, 2019

Conversation

timothyjward
Copy link
Contributor

This change separates the JavaCPP Maven Plugin into its own module. This reduces the dependency fanout of the main library, and avoids the risk that anyone might try to call the Mojos without the necessary dependencies on the classpath.

This change also forms the basis of being able to create a working JavaCPP OSGi Bundle

This change separates the JavaCPP Maven Plugin into its own module. This reduces the dependency fanout of the main library, and avoids the risk that anyone might try to call the Mojos without the necessary dependencies on the classpath

Signed-off-by: Tim Ward <[email protected]>
@saudet saudet changed the title Separate the Maven Plugin from JavaCPP Separate the Maven Plugin from JavaCPP for OSGi Aug 14, 2019
@saudet
Copy link
Member

saudet commented Aug 14, 2019

FWIW, others appear to be using JavaCPP just fine with OSGi, see #194.
Why do you say this is absolutely necessary?

@saudet
Copy link
Member

saudet commented Aug 15, 2019

Excluding a single class seems incredibly easy to do:
https://osgi.org/specification/osgi.core/7.0.0/framework.module.html#i3106983
Have you given this a try?

@timothyjward
Copy link
Contributor Author

FWIW, others appear to be using JavaCPP just fine with OSGi, see #194.

It's not clear from that bug how the JavaCPP library is being used, but my money would be on it being embedded inside one big module with the code that's using it. That isn't really being a module, nor is it easy to reuse. It should be possible to install JavaCPP as a bundle and have it work properly. This is what users (quite rightly) expect.

Excluding a single class seems incredibly easy to do:
https://osgi.org/specification/osgi.core/7.0.0/framework.module.html#i3106983
Have you given this a try?

I am aware of this part of the OSGi specification, but:

  1. Carrying around "dead code" is a bad idea, it can introduce all sorts of bugs and security holes
  2. There is still the risk of further coupling appearing over time which prevents further classes from being loaded
  3. The bundle still has dependencies on org.apache.maven.xyz packages. These must either be supplied at runtime, or they must be manually excluded in an error-prone build file that JavaCPP developers have to maintain manually
  4. In 10 years I have never seen anyone use export exclusions in the wild. It's unbelievably confusing for users who can safely compile, but get NoClassDefFoundError at runtime

Why do you say this is absolutely necessary?

Because history teaches us that taking shortcuts comes back to bite us in the end. The fact that the Maven plugin wasn't made separate in the first place has already resulted in #113, failing to fix the fundamental issue will lead to more pain in future (as it did for me trying to use JavaCPP in an OSGi project!). Making the plugin separate is a small change, offers better separation of concerns, shrinks the size of the runtime library, protects against misuse...

JavaCPP is a key part of ND4J and DL4J, which as Eclipse projects are going to need to run well in an OSGi framework. I can guarantee that these issues are going to impact JavaCPP users more often in the future.

@saudet
Copy link
Member

saudet commented Aug 15, 2019

None of your concerns are technical blockers, they're just concerns, about how you feel and how maybe others feel, but not everyone feels the same. (BTW, optional dependencies are not transitive. We don't need to exclude them or anything. Simply ignore them and everything will be fine). If/when more people start requesting this + incentives, we'll see. For now, you're the only person that has ever even asked about this, so I'm not going to increase the number of modules or break backward compatibility for a single user here. This is not "a small change". Please stop thinking that you're the only user in the world. If you want your own version of JavaCPP, you're free to maintain a fork, since the license allows it and everything.

BTW, it sounds like you're telling me that all OSGi users (which I assume are all enterprises) feel the same about these issues as you do, but where are all those users? It seems like you guys don't really care about native libraries like ND4J and DL4J, but it's going to be required to compete with other platforms in the future. What are you going to do about that?

@timothyjward
Copy link
Contributor Author

This is not "a small change". Please stop thinking that you're the only user in the world.

I really don't think that, and I went out of my way to make sure that the change would not break things.

  • The runtime library retains the same GAV, packaging and classifier before and after the change
  • Other than the Mojo types, which you've told me that nobody uses at runtime (because if they did it would blow up), no types are moved or renamed.

Can you point me to how the impact on existing users at runtime is non-zero?

The only difference that anyone will see is that if they choose to upgrade the version of the maven plugin that they use then they must update an artifact id as well as a version. For that they get to use a maven plugin with a packaging of maven-plugin, easily locatable in maven central, and that plays nicely with all the maven tools expecting a <plugin> to be packaged as such.

It seems like you guys don't really care about native libraries like ND4J and DL4J, but it's going to be required to compete with other platforms in the future. What are you going to do about that?

I clearly do care, as the reason I ended up looking at JavaCPP was because I wanted to make use of DL4J alongside other Eclipse projects. In fact I care so much that rather than just raising issues and expecting people to do work for me I spent my own time trying to improve things through discussions and Pull Requests. Unfortunately for me this seems to have resulted in a fair bit of hostility.

@saudet
Copy link
Member

saudet commented Aug 15, 2019

Can you point me to how the impact on existing users at runtime is non-zero?

Well, there's 3 modules instead of 1 now. This adds complexity. People will be confused about whether they should depend on javacpp or javacpp-maven-plugin. This is basic UX stuff.

I clearly do care, as the reason I ended up looking at JavaCPP was because I wanted to make use of DL4J alongside other Eclipse projects. In fact I care so much that rather than just raising issues and expecting people to do work for me I spent my own time trying to improve things through discussions and Pull Requests. Unfortunately for me this seems to have resulted in a fair bit of hostility.

It's great that you're trying to help, but if you're the only one who cares in your domain, it's not going to change things, unfortunately. You seem rather adamant in changing everything here, but what about changing your own colleagues? Why does no one seemingly care? Be just as adamant with your friends than with everyone else, and then I'll be able to take you more seriously.

@timothyjward
Copy link
Contributor Author

Well, there's 3 modules instead of 1 now. This adds complexity. People will be confused about whether they should depend on javacpp or javacpp-maven-plugin. This is basic UX stuff.

That argument is a bit thin - I'm sure that you don't believe that all libraries should be delivered as a single module, and do you really think that a user would depend on the javacpp-maven-plugin thinking that they were getting something other than a Maven plugin? What about adding integration tests for JavaCPP? Testing would be a good idea to prove things work at runtime (in OSGi they currently don't) but mixing all that test code and test execution logic into an already complicated module...

There are almost no library projects out there with a single module build. Almost all of them have multiple modules producing runtime code, and most have additional modules which run tests. I think it's incorrect to state that the users of these libraries are intrinsically confused as a result.

You seem rather adamant in changing everything here, but what about changing your own colleagues?

As I've pointed out, I worked really hard in this PR to avoid changing things as much as possible. My goal was to take JavaCPP, which does a great job simplifying the use of native code in Java, and to make it work just as simply in OSGi. As with most things this is a two step process:

  1. Get the library packaged with the proper metadata. This is calculated based on the runtime dependencies of the library class files (hence the maven mojos cause a lot of yuck)
  2. Run the library in OSGi and fix points (if any) where the class loader assumptions in the library don't match

I have worked hard to avoid doing anything that would cause breakages in the API because I know that there are other users out there.

I am certain that there are ways to work around JavaCPP's structure as it currently is by yet further customising the build, and using some of the more unusual features of OSGi, but there is always a trade off. The build will be more complex (and so harder to maintain and for others to contribute to), tools can give less help (so errors creep into the packaging unseen).

I have been trying to approach this process by doing the right thing, not just hacking together an approach which works for me. DL4J and ND4J are Eclipse projects and as such it is reasonable to expect them to work within the Eclipse ecosystem. I know that they are comparatively new Eclipse projects which is why, at the moment, not much has happened in this area but that will change. As a core part of the API for both DL4J and ND4J that will also require more from JavaCPP if it is to be successful.

then I'll be able to take you more seriously.

I've not often known an Open Source project be so willing to turn away contributions from people with relevant experience, but ok. What do you propose for making JavaCPP deploy as part of an Eclipse RCP application, or deploying ML behaviours to a concierge framework running on an embedded gateway?

@saudet
Copy link
Member

saudet commented Aug 16, 2019

That argument is a bit thin - I'm sure that you don't believe that all libraries should be delivered as a single module, and do you really think that a user would depend on the javacpp-maven-plugin thinking that they were getting something other than a Maven plugin? What about adding integration tests for JavaCPP? Testing would be a good idea to prove things work at runtime (in OSGi they currently don't) but mixing all that test code and test execution logic into an already complicated module...

The "integration tests" are basically the presets, check them out:
https://github.com/bytedeco/javacpp-presets

There are almost no library projects out there with a single module build. Almost all of them have multiple modules producing runtime code, and most have additional modules which run tests. I think it's incorrect to state that the users of these libraries are intrinsically confused as a result.

It's matter of degrees, there's even a name for this phenomenon:
https://en.wikipedia.org/wiki/Hick%27s_law
In any case, it is true that there is a convention for Maven plugins, but for now there is nothing preventing JavaCPP from working easily with OSGi without breaking backward compatibility. I would be fine with rolling this "modularization" in with other breaking changes in the future, but right now is not the time.

  1. Get the library packaged with the proper metadata. This is calculated based on the runtime dependencies of the library class files (hence the maven mojos cause a lot of yuck)

Could you elaborate on the "yuck" factor? As far as I understand we just need to add an "exclude" directive and that's it, we're done.

  1. Run the library in OSGi and fix points (if any) where the class loader assumptions in the library don't match

This is unrelated to the changes in this PR. Let's have another thread for that.

I've not often known an Open Source project be so willing to turn away contributions from people with relevant experience, but ok. What do you propose for making JavaCPP deploy as part of an Eclipse RCP application, or deploying ML behaviours to a concierge framework running on an embedded gateway?

Then you need to get out more often! I see LWJGL is having just as hard of a time with OSGi: LWJGL/lwjgl3#277

@timothyjward
Copy link
Contributor Author

Could you elaborate on the "yuck" factor? As far as I understand we just need to add an "exclude" directive and that's it, we're done.

OSGi works by modules declaring "exported" packages and "imported" packages. The exclude directive only handles hiding classes on the exporting side (and is a very unusual thing to use). The remaining maintenanceproblem will be on the import side.

The metadata needed by OSGi is very hard to write correctly if you do it by hand, and then almost impossible to keep in sync with the code afterwards. Thankfully there exist tools which can generate this metadata in the build. They do this generation based on some instructions (e.g. "export this package") and by scanning the byte code of the class files for references to classes from other packages. These references are used to calculate the necessary imports.

"Hiding" the Mojos in the exported package doesn't remove them from the jar file, nor does it ensure that they aren't used in other code paths through the bundle, therefore the dependencies still exist, and get added as imported packages. The imports for these packages can be suppressed using configuration in the build, but you're then in the position of saying "I know that the dependencies are there, but I promise that they aren't needed". This is a comparatively weak statement and as coupling tends to accrete over time projects that do this tend to break over time in ways that could have been detected by the tools.

@timothyjward
Copy link
Contributor Author

I would be fine with rolling this "modularization" in with other breaking changes in the future, but right now is not the time.

What would your opinion be of a PR which added a reactor and some integration tests, but neither of them were deployed (i.e. the existing library exactly as is, just pushed down one folder level)? There are significant advantages to having the integration tests run as part of every top level build, and this would avoid any additional artifacts in Maven central.

If you aren't happy with adding tests in this way then where would be the right place for a test module showing a failure in OSGi?

@saudet
Copy link
Member

saudet commented Aug 16, 2019

"Hiding" the Mojos in the exported package doesn't remove them from the jar file, nor does it ensure that they aren't used in other code paths through the bundle, therefore the dependencies still exist, and get added as imported packages. The imports for these packages can be suppressed using configuration in the build, but you're then in the position of saying "I know that the dependencies are there, but I promise that they aren't needed". This is a comparatively weak statement and as coupling tends to accrete over time projects that do this tend to break over time in ways that could have been detected by the tools.

This means we're going to have the same problem with the module-info.class from JPMS, no? I don't see how adding a couple more classes like that would make it so much worse.

What would your opinion be of a PR which added a reactor and some integration tests, but neither of them were deployed (i.e. the existing library exactly as is, just pushed down one folder level)? There are significant advantages to having the integration tests run as part of every top level build, and this would avoid any additional artifacts in Maven central.

If you aren't happy with adding tests in this way then where would be the right place for a test module showing a failure in OSGi?

Are you asking about how to add an integration test? We should be able to do it in the standard fashion with Maven Surefire or Failsafe, right?

@timothyjward
Copy link
Contributor Author

This means we're going to have the same problem with the module-info.class from JPMS, no? I don't see how adding a couple more classes like that would make it so much worse.

Having the Mojos in the JavaCPP library will certainly mean that Maven JPMS modules (if/when they become available) show up as dependencies in jdeps. The intent is that the Java Compiler also validates and enforces the module-info.class, so it is likely that JavaCPP will fail to compile if it tries to create a JPMS module using the standard Java Tools. There may or may not be a way to override or work around this. From an OSGi perspective module-info.class isn't a source of dependencies because it's forbidden from containing executable code, and so can't cause problems at runtime.

Are you asking about how to add an integration test? We should be able to do it in the standard fashion with Maven Surefire or Failsafe, right?

The test I envisage does the following things:

  1. Contains source for a Java type using JavaCPP annotations and a native method
  2. Contains source for the C/C++ providing the implementation of the native method
  3. Contains source for a JUnit test which uses the native method
  4. Uses the JavaCPP maven plugin (from the current build) to generate/compile the necessary JNI code (this helps to make the test multi-platform)
  5. Uses the bnd-maven-plugin to wrap all the classes/native code up in an OSGi bundle
  6. Uses the bnd-testing-maven-plugin to deploy the test bundle, JavaCPP (again, from the current build) and JUnit, then run the test

This is pretty simple to set up as a separate module in the main build, and it will play nicely with IDEs. It is also the way that OSGi bundles are typically integration tested.

I'm sure that it's also possible to run the same test as a sub-build using the maven-invoker-plugin with the sources stored in src/main/it. This approach will make it much harder to develop and maintain the tests as IDEs don't typically understand how to load maven modules that are embedded in other modules.

Yet another approach would be to use the Surefire Plugin and the src/main/it folder to include an OSGi test using PAX-Exam, this test could programatically drive the JavaCPP Builder to assemble the compiled JNI code, and then use TinyBundles to build that into an OSGi bundle before running the test. This approach has the advantage that it will also work reasonably well in an IDE (once src/main/it is added as a source folder), however the test will contain a lot of relatively complex code used to set up the system. This testing model will also not test the JavaCPP maven plugin.

@saudet saudet changed the base branch from master to osgi August 17, 2019 06:39
@saudet
Copy link
Member

saudet commented Aug 17, 2019

Well, if you really need to break backward compatibility, let's maintain a separate experimental branch for this. I've created one named "osgi". Put whatever you need there, and we'll get Travis CI running tests and publishing snapshots. We'll see if there is interest for that in the community that way. Sounds good?

@saudet
Copy link
Member

saudet commented Aug 19, 2019

You know, if the class names don't change, it will be possible for users to override the version of JavaCPP to say 1.6.0-SNAPSHOT and try out stuff that way. I think that's more than enough for now, especially since I'm sure that when users actually start trying this out, they'll have their own ideas about how to change everything...

This was referenced Aug 19, 2019
@timothyjward
Copy link
Contributor Author

Based on the discussions in this thread I've created two more PRs implementing the two options that seem most workable.

PR #331 takes this PR and adds to it. The resulting JavaCPP has OSGi metadata, and there is an additional Maven Module which provides OSGi tests.

PR #332 ignores the commit in this PR and just adds OSGi metadata to the JavaCPP library as is. The PR also adds an integration test (identical to the integration test in the other PR) to the src/it/ folder.

Obviously it only makes sense to merge one of these two PRs. I leave that decision to the project leads.

@saudet
Copy link
Member

saudet commented Aug 20, 2019

@timothyjward Thanks! So which one should we merge to the osgi branch? Put another way, you're probably going to be the only one looking at this for a while, so let's go with the one you feel like maintaining.

@timothyjward
Copy link
Contributor Author

So #331 would be my preferred option because I think it's easier to develop on, and it properly separates the concerns of the Maven modules (one for the library, one for the maven plugin, one as an integration test project).
On the other hand I want there to be a reasonable chance of these changed getting merged back into master someday, so if there are significant objections to the maven module separation then I'm ok with continuing to work on the all-in-one version

@saudet saudet merged commit 276ee95 into bytedeco:osgi Aug 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants