gradlex-org / extra-java-module-info

A Gradle 6.8+ plugin to use legacy Java libraries as Java Modules in a modular Java project
Apache License 2.0
100 stars 6 forks source link

wrong result when add provides ... with ... declarations .. #13

Closed Kingson4Wu closed 3 years ago

Kingson4Wu commented 3 years ago

module("groovy-all-2.4.15.jar", 'groovy-all', '2.4.15'){ }

module-info.class result:

open module groovy-all {
    requires java.base;

    provides javax.script.ScriptEngineFactory with org.codehaus.groovy.jsr223.GroovyScriptEngineFactory;
    provides org.codehaus.groovy.plugins.Runners with org.codehaus.groovy.testng.TestNgRunner;
    provides org.codehaus.groovy.runtime.ExtensionModule with moduleName=groovy-all, moduleVersion=2.4.15, extensionClasses=org.codehaus.groovy.jsr223.ScriptExtensions,org.codehaus.groovy.runtime.NioGroovyMethods,org.codehaus.groovy.runtime.SqlGroovyMethods,org.codehaus.groovy.runtime.SwingGroovyMethods,org.codehaus.groovy.runtime.XmlGroovyMethods, staticExtensionClasses=org.codehaus.groovy.jsr223.ScriptStaticExtensions;
    provides org.codehaus.groovy.source.Extensions with groovy;
    provides org.codehaus.groovy.transform.ASTTransformation with groovy.grape.GrabAnnotationTransformation, org.codehaus.groovy.ast.builder.AstBuilderTransformation;
}

provides org.codehaus.groovy.runtime.ExtensionModule with moduleName=groovy-all, moduleVersion=2.4.15, extensionClasses=org.codehaus.groovy.jsr223.ScriptExtensions,org.codehaus.groovy.runtime.NioGroovyMethods,org.codehaus.groovy.runtime.SqlGroovyMethods,org.codehaus.groovy.runtime.SwingGroovyMethods,org.codehaus.groovy.runtime.XmlGroovyMethods, staticExtensionClasses=org.codehaus.groovy.jsr223.ScriptStaticExtensions;

is wrong because the jar exist a file /META-INF/services/org.codehaus.groovy.runtime.ExtensionModule content is

# This is a generated file, do not edit
moduleName=groovy-all
moduleVersion=2.4.15
extensionClasses=org.codehaus.groovy.jsr223.ScriptExtensions,org.codehaus.groovy.runtime.NioGroovyMethods,org.codehaus.groovy.runtime.SqlGroovyMethods,org.codehaus.groovy.runtime.SwingGroovyMethods,org.codehaus.groovy.runtime.XmlGroovyMethods
staticExtensionClasses=org.codehaus.groovy.jsr223.ScriptStaticExtensions
jjohannes commented 3 years ago

@iherasymenko would you mind commenting on this as you added the provides support (#9) and are much more expert on the topic than I am?

iherasymenko commented 3 years ago

@jjohannes this looks like an issue with Groovy (ab)using the META-INF/services namespace. GROOVY-8480 has some insights.

The good news is that the problem was fixed in Groovy 2.5, the bad news is that there's no single jar bundle for 2.5.x which leads to another issue, namely, the split package problem (GROOVY-8666). Groovy website claims, the split package problem will be fixed in Groovy 4 which is yet to be released.

I have the following options in mind on how this particular issue with groovy-all-2.4.15.jar can be solved.

1) Blacklist /META-INF/services/org.codehaus.groovy.runtime.ExtensionModule and not treat it as an SPI implementation descriptor. 2) Introduce a new DSL method ignoreServiceProvider(String) in de.jjohannes.gradle.javamodules.ModuleInfo which will allow skipping automatic migration of some service provider definitions. 3) Use automaticModule('groovy-all-2.4.15.jar', 'groovy.all') if one does not really want to further feed this JAR to jlink or jpackage and the goal is just to have this JAR be placed on the module path.

The second option will also be handy in situations where the automatic migration causes a hard requirement to have a JAR which contains the service interface on the module path or the classpath if the module-info.class defines a provides implementation.

For example, if I were to modularize org.springframework:spring-core, I would have to place io.projectreactor.tools:blockhound on the module path as well. Having the ability to ignore migrating reactor.blockhound.integration.BlockHoundIntegration service implementation descriptor, I would be able to avoid bringing this dependency to my application.

jjohannes commented 3 years ago

Thanks for the detailed analysis and writeup @iherasymenko. I think what you propose as solution 2 would be nice to add, as there are use cases for this:

  1. Introduce a new DSL method ignoreServiceProvider(String) in de.jjohannes.gradle.javamodules.ModuleInfo which will allow skipping automatic migration of some service provider definitions.

Would one of you be willing to contribute a PR for that?

iherasymenko commented 3 years ago

@jjohannes I will prepare a PR this week.

iherasymenko commented 3 years ago

@jjohannes when you have a moment, please review #14.

@Kingson4Wu once this PR is merged and a new version of the plugin is released, you should be able to overcome the issue. I also included a test case which targets your use case with Groovy.

jjohannes commented 3 years ago

@Kingson4Wu version 0.7 now allows you to ignore the problematic service providers as described here: https://github.com/jjohannes/extra-java-module-info#how-do-i-add-provides--with--declarations-to-the-module-infoclass-descriptor

Kingson4Wu commented 3 years ago

@jjohannes @iherasymenko thank you very much.