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-2435: Add automatic-module-names to the manifests #1304

Merged
merged 1 commit into from
Apr 4, 2020

Conversation

bbucko
Copy link
Contributor

@bbucko bbucko commented Jul 21, 2019

This change adds an Automatic-Module-Name entry to the manifest which specifies name of the module used by JPMS. It's a potentially breaking change because, by default, module's name is based on the jar's filename.

@datastax-bot
Copy link

Hi @bbucko, thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes.

Sincerely,
DataStax Bot.

@datastax-bot
Copy link

Thank you @bbucko for signing the Contribution License Agreement.

Cheers,
DataStax Bot.

@adutra
Copy link
Contributor

adutra commented Jul 25, 2019

Thank you for your contribution. We started investigating JPMS support in JAVA-1827 but stopped out of lack of interest from our user base. You are actually the first one asking for JPMS support :-)

I agree that a quick win would be to add automatic module names.

That said, if we go down that route we must ensure that:

  1. All modules in all projects are properly assigned with automatic module names;
  2. That we use a consistent way of producing automatic module names across the board. I'm not sure that your approach is the best because it piggybacks on the Maven Bundle plugin, which is actually meant to handle OSGi stuff. I think a better approach would be to use the Maven Jar plugin for that, or use a dedicated plugin such as the Moditect plugin.

@bbucko
Copy link
Contributor Author

bbucko commented Jul 26, 2019

Yeah, I've already noticed that Maven Bundle plugin does other things (e.g. copies classes from dependencies to your classpath.. 🤷‍♂️) but adding Automatic-Module-Name entry in MANIFEST.MF should be the first step. It won't give us the full power of JPMS (e.g. no native images) but it's still better than nothing. I'll add module names to remaining projects and I'll check if Maven Jar plugin can be configured together with Bundler but I don't want to mess with the OSGI configuration as I'm not familiar with it.

@bbucko
Copy link
Contributor Author

bbucko commented Jul 26, 2019

I removed the Automatic-Module-Name from shaded jar as shading breaks the encapsulation either way. All the entries are now generated by maven-jar-plugin and they are kept separate from the OSGi configuration - some characters are allowed in OSGi but not in JPMS.

@olim7t olim7t added this to the 4.2.0 milestone Aug 9, 2019
@adutra adutra modified the milestones: 4.2.0, 4.3.0 Sep 23, 2019
@olim7t olim7t modified the milestones: 4.3.0, 4.4.0 Nov 15, 2019
@adutra adutra modified the milestones: 4.4.0, 4.5.0 Jan 8, 2020
@adutra adutra removed this from the 4.5.0 milestone Feb 24, 2020
Copy link
Contributor

@olim7t olim7t left a comment

Choose a reason for hiding this comment

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

First, apologies for leaving this unaddressed for so long.
4.x has changed a lot, but this PR still rebases pretty easily, there are a couple of conflicts but they are trivial (I can take care of that if needed).

I ran into an issue with gremlin-shaded (a new transitive dependency that was added for graph support in 4.5):

Error occurred during initialization of boot layer
java.lang.module.FindException: Unable to derive module descriptor for /path/to/gremlin-shaded-3.4.5.jar
Caused by: java.lang.module.InvalidModuleDescriptorException: Provider class com.fasterxml.jackson.core.JsonFactory not in module

This happens because they've shaded their own dependencies, but kept around service descriptors that still reference the unshaded classes. I'll open a ticket to ask them to fix this, in the meantime I think the only workaround is to exclude the Tinkerpop dependencies as explained here; sadly, that means Graph users won't be able to use JPMS.

I removed the Automatic-Module-Name from shaded jar as shading breaks the encapsulation either way

I think we should keep it. The rationale for the shaded JAR is to avoid version clashes if the client application requires a different Netty version. Modules will never be able to solve that, requires directives don't allow a version, that is an explicit non-goal of JPMS.

PS- we'll also need to upgrade native-protocol to 1.4.10-SNAPSHOT in order to use the change from your other pull request. That version is now defined in bom/pom.xml.

@@ -71,6 +71,7 @@
<artifactId>maven-bundle-plugin</artifactId>
<configuration>
<instructions>
<Automatic-Module-Name>com.datastax.oss.driver.querybuilder</Automatic-Module-Name>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to go to the jar-plugin configuration here as well.

@olim7t
Copy link
Contributor

olim7t commented Mar 5, 2020

TINKERPOP-2347 for the issue with gremlin-shaded.

@olim7t olim7t changed the title Add Automatic-Module-Name to the manifest JAVA-2435: Add automatic-module-names to the manifests Apr 2, 2020
@olim7t olim7t added this to the 4.6.0 milestone Apr 2, 2020
Copy link
Contributor

@olim7t olim7t left a comment

Choose a reason for hiding this comment

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

I've rebased on the latest 4.x and addressed my comments.

I'll leave this open for a bit more and merge tomorrow.

@@ -303,6 +303,7 @@
</goals>
<configuration>
<instructions>
<Automatic-Module-Name>com.datastax.oss.driver.core</Automatic-Module-Name>
Copy link
Contributor

Choose a reason for hiding this comment

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

For the shaded core, we have to do this from the bundle plugin, because we generate a separate manifest file and include it manually in the final archive.

@olim7t
Copy link
Contributor

olim7t commented Apr 4, 2020

I'm not sure why Travis is failing, it's related to the plumber doclet on JDK 11, but this PR doesn't change anything about that. I'll merge and see if the problems persist on 4.x.

@olim7t olim7t merged commit 0be0f64 into apache:4.x Apr 4, 2020
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