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

Add Automatic Module Name #1199

Closed
eddumelendez opened this Issue Mar 4, 2018 · 13 comments

Comments

Projects
None yet
4 participants
@eddumelendez
Copy link
Member

eddumelendez commented Mar 4, 2018

Current version is using sun.misc.Unsafe. We should get remove this before to Use Automatic-Module-Name entry

jdeps --jdk-internals target/mybatis-3.4.6-SNAPSHOT.jar                                                                                                                   
mybatis-3.4.6-SNAPSHOT.jar -> jdk.unsupported
   org.apache.ibatis.javassist.util.proxy.DefineClassHelper -> sun.misc.Unsafe                                    JDK internal API (jdk.unsupported)

Warning: JDK internal APIs are unsupported and private to JDK implementation that are
subject to be removed or changed incompatibly and could break your application.
Please modify your code to eliminate dependence on any JDK internal APIs.
For the most recent update on JDK internal API replacements, please check:
https://wiki.openjdk.java.net/display/JDK8/Java+Dependency+Analysis+Tool

JDK Internal API                         Suggested Replacement
----------------                         ---------------------
sun.misc.Unsafe                          See http://openjdk.java.net/jeps/260

@eddumelendez eddumelendez added this to the 3.5.0 milestone Mar 4, 2018

@kazuki43zoo

This comment has been minimized.

Copy link
Member

kazuki43zoo commented Mar 17, 2018

The DefineClassHelper is Javassist's class. We need to wait releasing the next version of Javassist.

See https://github.com/jboss-javassist/javassist/blob/master/src/main/javassist/util/proxy/DefineClassHelper.java

@harawata

This comment has been minimized.

Copy link
Member

harawata commented Oct 24, 2018

Hi @eddumelendez @kazuki43zoo ,
Can we update Javassist to 3.23.1-GA and proceed with adding automatic module name?

@eddumelendez

This comment has been minimized.

Copy link
Member Author

eddumelendez commented Oct 24, 2018

Hi @harawata I just updated the javassist version and there is no more warnings. Now, we can add manually the Automatic-Module-Name or we can add a maven phase in mybatis-parent for all mybatis projects.

WDYT?

@harawata

This comment has been minimized.

Copy link
Member

harawata commented Oct 24, 2018

Thank you for the update, @eddumelendez !

we can add a maven phase in mybatis-parent for all mybatis projects.

I like the idea.
Just to clarify, will there be any 'conflict' when we add module-info.java to some child projects in the future?

@hazendaz

This comment has been minimized.

Copy link
Member

hazendaz commented Oct 25, 2018

Hi, I haven't messed around with modules yet. What is the thinking on what would go in the parent and how does that impact the downstreams?

@harawata

This comment has been minimized.

Copy link
Member

harawata commented Oct 29, 2018

I guess no one is sure about it. :D
In that case, let's add Automatic-Module-Name locally for now.
It shouldn't be too hard to change it later.

By the way, what's the module name?
org.mybatis.core or just org.mybatis ?

@eddumelendez

This comment has been minimized.

Copy link
Member Author

eddumelendez commented Oct 29, 2018

module name use to be the artifactId in this case just mybatis. For artifactIds with dash such as mybatis-spring would be mybatis.spring

@harawata

This comment has been minimized.

Copy link
Member

harawata commented Oct 29, 2018

@eddumelendez ,

Yeah, that's how module name is calculated from the JAR name implicitly.
When declaring Automatic-Module-Name explicitly, however, it should match the module name that will be declared in module-info.java and we should follow the 'reverse DNS' rule, I think.

I'm not sure why Spring chose the filename-based module name, but there must have been a good reason.

@kazuki43zoo

This comment has been minimized.

Copy link
Member

kazuki43zoo commented Oct 29, 2018

I'm not sure why Spring chose the filename-based module name, but there must have been a good reason.

Attaches the issue link.

@kazuki43zoo

This comment has been minimized.

Copy link
Member

kazuki43zoo commented Oct 29, 2018

I've investigated an "Automatic-Module-Name" on some libraries.

GroupId:ArtifactId Automatic-Module-Name
com.zaxxer:HikariCP com.zaxxer.hikari
org.junit.jupiter:junit-jupiter-engine org.junit.jupiter.engine
com.fasterxml.jackson.core:jackson-databind com.fasterxml.jackson.databind
org.mockito:mockito-core org.mockito
@kazuki43zoo

This comment has been minimized.

Copy link
Member

kazuki43zoo commented Oct 29, 2018

we should follow the 'reverse DNS' rule, I think.

👍 I think better that a module name start with org.mybatis.

e.g.)

mybatis -> org.mybatis or org.mybatis.core
mybatis-spring -> org.mybatis.spring
mybatis-cdi -> org.mybatis.cdi
mybatis-scala -> org.mybatis.scala
mybatis-redis -> org.mybatis.redis

WDYT?

@harawata

This comment has been minimized.

Copy link
Member

harawata commented Oct 29, 2018

Thanks @kazuki43zoo for the link.
I still couldn't really understand why they had to use the filename based naming, but it would be safe to say that Spring was a special case. :D

'core' seems unnecessary. So, +1 to 'org.mybatis'.
The others look good to me, too.

@harawata

This comment has been minimized.

Copy link
Member

harawata commented Dec 29, 2018

It's been a while, so I assume that everyone is OK with the reverse DNS naming rule. :)

@hazendaz ,
In the parent's pom.xml, there should be an entry configuring maven-jar-plugin.
For example,

<plugin>
  <groupId>org.apache.maven.plugins</groupId>
  <artifactId>maven-jar-plugin</artifactId>
  <configuration>
    <archive>
      <manifestEntries>
        <Automatic-Module-Name>${module.name}</Automatic-Module-Name>
      </manifestEntries>
    </archive>
  </configuration>
</plugin>

And in each subproject, declare the property.

<properties>
  ...
  <module.name>org.mybatis</module.name>
  ...
</properties>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment