janino-compiler / janino

Janino is a super-small, super-fast Java™ compiler.
http://janino-compiler.github.io/janino
Other
1.21k stars 205 forks source link

Add Automatic-Module-Name to MANIFEST.MF #183

Closed ceki closed 1 year ago

ceki commented 1 year ago

Given that Janino artifacts are not modularized, it would help if you could set Automatic-Module-Name property in MANIFEST.MF.

I would suggest the following names:

artifact suggested Automatic-Module-Name value
org.codehaus.janino:commons-compiler org.codehaus.janino.commons.compiler
org.codehaus.janino:janino org.codehaus.janino.janino

These names can be added to your pom.xml as follows:

        <groupId>org.apache.maven.plugins</groupId>
        <artifactId>maven-jar-plugin</artifactId>
        <executions>
          <execution>
            <id>default-jar</id>
            <phase>package</phase>
            <goals>
              <goal>jar</goal>
            </goals>
            <configuration>
              <archive combine.children="append">
                <manifestEntries>
                  <Automatic-Module-Name>org.codehaus.janino.janino</Automatic-Module-Name>
                </manifestEntries>
              </archive>
            </configuration>
          </execution>
        </executions>
      </plugin>

This issue is a blocker for the logback project.

SLF4J 1.7.x series added Automatic-Module-Name entries a while back. See slf4j PR 184

aunkrig commented 1 year ago

Hey Ceki,

do don't understand a lot about modules... could that entry in the manifest have negative consequences for other users?

CU Arno

ceki commented 1 year ago

Hi @aunkrig,

I doubt adding Auto-Module-Name entries will have a negative effect. When Auto-Module-Name entries were added to SLF4J 1.7.x series several years ago, no one complained.

In the mean time, I have patched 3.1.9-SNAPSHOT to see if Auto-Module-Name entries help. Instead of 1) in the build I can pas the more specific 2) as an argument to the compiler. 1) --add-reads ch.qos.logback.core=ALL-UNNAMED (using 3.1.8) 2) --add-reads ch.qos.logback.core=org.codehaus.janino.commons.compiler,org.codehaus.janino.janino (using my patched 3.1.9-SNAPHOT)

In 1) logback is opened to all unnamed modules, whereas in 2), it is open specifically to janino and the commons-compiler modules which is much more specific and therefore preferable.

ceki commented 1 year ago

Patch

diff --git a/commons-compiler/pom.xml b/commons-compiler/pom.xml
index 2b4436b6..61b3ad99 100644
--- a/commons-compiler/pom.xml
+++ b/commons-compiler/pom.xml
@@ -17,6 +17,18 @@

   <build>
     <plugins>
+      <plugin>
+        <artifactId>maven-jar-plugin</artifactId>
+        <version>3.0.2</version>
+        <configuration>
+          <archive>
+             <manifestEntries combine.children="append">
+                  <Automatic-Module-Name>org.codehaus.janino.commons.compiler</Automatic-Module-Name>
+             </manifestEntries>
+          </archive>
+        </configuration>
+      </plugin>
+
       <plugin>
         <groupId>org.apache.felix</groupId>
         <artifactId>maven-bundle-plugin</artifactId>
diff --git a/janino/pom.xml b/janino/pom.xml
index 0a66697f..cb74c5e7 100644
--- a/janino/pom.xml
+++ b/janino/pom.xml
@@ -46,6 +46,18 @@

   <build>
     <plugins>
+      <plugin>
+        <artifactId>maven-jar-plugin</artifactId>
+        <version>3.0.2</version>
+        <configuration>
+          <archive>
+             <manifestEntries combine.children="append">
+                  <Automatic-Module-Name>org.codehaus.janino.janino</Automatic-Module-Name>
+             </manifestEntries>
+          </archive>
+        </configuration>
+      </plugin>
+
       <plugin>
         <groupId>org.apache.maven.plugins</groupId>
         <artifactId>maven-surefire-plugin</artifactId>
aunkrig commented 1 year ago

Hey Ceki,

I looked into that and have the following questians:

  1. commons-compiler-jdk should also have an automatic-module-name, right?
  2. Is it really useful to give every maven module (janino, commons-compilerand maybe commons-compiler-jdk) an individual (Java) module name, as you propose? I.e. why would one open the maven modules individually for, say, log4j? Or is it just good style to have distinct (Java) module names for each .jar file?

CU Arno

ceki commented 1 year ago

Hi Arno,

1) yes. commons-compiler-jdk should probably also have an automatic-module-name 2) Please note that non-jigsaw/JPMS artifacts are already automatically assigned a module name. For example, janino.jar is already assigned the module name "janino" and commons-compiler.jar the name "commons-compiler". By setting the name via Auto-Module-Name, you are establishing today a contract for the names your modules will have in the future, when and if you decide to jigsaw/JPMS modularize them. Otherwise, users depending on janino will have to change their module-info declarations from "requires janino" (janino is the current module name) to "requires "some.new.name.for.janino".

Setting a Auto-Module-Name in your artifacts is a very small step in the direction of jigsaw/JPMS modularization.

aunkrig commented 1 year ago

Ok, I added the manifest entry to all four modules:

$ zzgrep --include '***!META-INF/MANIFEST.MF' Automatic */target/*.jar
commons-compiler\target\commons-compiler-3.1.10-SNAPSHOT.jar!META-INF/MANIFEST.MF:Automatic-Module-Name: org.codehaus.commons.compiler
commons-compiler-jdk\target\commons-compiler-jdk-3.1.10-SNAPSHOT.jar!META-INF/MANIFEST.MF:Automatic-Module-Name: org.codehaus.commons.compiler.jdk
commons-compiler-tests\target\commons-compiler-tests-3.1.10-SNAPSHOT.jar!META-INF/MANIFEST.MF:Automatic-Module-Name: org.codehaus.commons.compiler.tests
janino\target\janino-3.1.10-SNAPSHOT.jar!META-INF/MANIFEST.MF:Automatic-Module-Name: org.codehaus.janino
$

Is that totally correct? Please check carefully.

ceki commented 1 year ago

@aunkrig It seems to me that the names should be prefixed by "org.codehaus.janino" and not just "org.codehaus".

aunkrig commented 1 year ago

I read that the module name should be equal to the "main package" of the module -- that would be without an additional ".janino." infix, right?

ceki commented 1 year ago

Yes. You are right. I somehow missed that the packages contained "org.codehaus.janino:commons-compiler" artifact did not have janino in their middle.

In JMPS/Jigsaw modularized applications, it is not possible for two artifacts (jar files) to duplicate a given package. This rule is enforced by the JVM at both compile and runtime. It is also true that the JVM will not allow two artifacts to have the same module name.

To cut a long story short, as the likelihood of collision under the org.codehaus stem is very low, both "org.codehaus.janino" and "org.codehaus" would work just fine. If I had to choose, I'd have a slight preference for "org.codehaus.janino".