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

Consider adding a module descriptor #296

Open
rfscholte opened this issue Feb 21, 2025 · 7 comments
Open

Consider adding a module descriptor #296

rfscholte opened this issue Feb 21, 2025 · 7 comments

Comments

@rfscholte
Copy link

rfscholte commented Feb 21, 2025

Currently state of this library is like this:

mvn org.apache.maven.plugins:maven-dependency-plugin:3.8.1:list -DincludeScope=runtime
[INFO] Scanning for projects...
[INFO]
[INFO] ---------------------< org.spdx:java-spdx-library >---------------------
[INFO] Building java-spdx-library 2.0.0-RC3-SNAPSHOT
[INFO]   from pom.xml
[INFO] --------------------------------[ jar ]---------------------------------
[INFO]
[INFO] --- maven-dependency-plugin:3.8.1:list (default-cli) @ java-spdx-library ---
[INFO] Can't extract module name from spdx-java-model-2_X-1.0.0-RC2.jar: spdx.java.model.2.X: Invalid module name: '2' is not a Java identifier
[INFO] Can't extract module name from spdx-java-model-3_0-1.0.0-RC2.jar: spdx.java.model.3.0: Invalid module name: '3' is not a Java identifier
[INFO]
[INFO] The following files have been resolved:
[INFO]    org.slf4j:slf4j-api:jar:2.0.7:compile -- module org.slf4j
[INFO]    org.apache.commons:commons-lang3:jar:3.5:compile -- module commons.lang3 (auto)
[INFO]    org.jsoup:jsoup:jar:1.15.3:compile -- module org.jsoup [auto]
[INFO]    com.google.code.gson:gson:jar:2.8.9:compile -- module com.google.gson
[INFO]    com.google.code.findbugs:jsr305:jar:3.0.2:compile -- module jsr305 (auto)
[INFO]    org.spdx:spdx-java-model-2_X:jar:1.0.0-RC2:compile
[INFO]    org.spdx:spdx-java-core:jar:1.0.0-RC2:compile -- module spdx.java.core (auto)
[INFO]    org.spdx:spdx-java-model-3_0:jar:1.0.0-RC2:compile

If a line ends with (auto), the module name is based on the filename
If a line ends with [auto], the module name is the Automatic-Module-Name entry of the META-INF/MANIFEST.MF
All others are explicit modules (containing a module-info.class)

Once it is structured as a valid module (e.g. no split packages with other modules), you could add the Automatic-Module-Name entry in the META-INF/MANIFEST.MF for this project.
Once all modules are explicit (no more automatic modules), you can also add the module-info.java to this project.

@goneall
Copy link
Member

goneall commented Feb 21, 2025

@rfscholte - can you provide a bit more specifics on a recommended approach?

We would like to continue to support Java 8 which doesn't support modules, at the same time we would like to support proper module naming as introduced with this pull request: #192

Is the solution to add a module.info file to the dependencies (e.g. spdx-java-core, spdx-java-model-2_X, and spdx-java-model-3_0)?

I haven't used modules in the past, so any advice is much appreciated.

@bact
Copy link
Collaborator

bact commented Feb 22, 2025

(based on @rfscholte and @goneall comments and some research: )

In this case, Maven is trying to determine the name of the unnamed module based on the JAR filename (spdx-java-model-2_X-1.0.0-RC2.jar).

spdx-java-model-2_X is being converted to spdx.java.model.2.X

The issue here is, each part of the module name must be a valid Java identifier name -- but 2 is not.

One of the way to fix this particular module naming issue is to change the JAR name to spdx-java-model-v2_X-RC2.jar, so the extracted module name will be spdx.java.model.v2.X -- every single parts are now a valid Java identifier.

--

But ideally, and by convention, the module name should be in a reverse DNS fashion.

#192 agrees that this should be org.spdx.library for Spdx-Java-Library.

Based on the current directory structure/package name, the module name for spdx-java-model-v2_X is likely to be: org.spdx.library.model.v2.

To achieve that, we have to explicitly name the module.

Automatic-Module-Name entry in pom.xml is one of the ways.

Another way is to put the module name inside module-info.java (to be compiled and resulting module-info.class) .

The content of module-info.java may look like something along this line:

module org.spdx.library.model.v2 {
    exports org.spdx.library.model.v2;
    exports org.spdx.library.referencetype;
    exports org.spdx.storage.compatv2;

    requires transitive org.spdx.core;
    requires transitive org.spdx.storage;
}

If we can do this with all org.spdx.* libraries, I believe it will fix the module naming issues.

--

However, the code base with module-info.java will not be well compiled in Java 8.

I'm not sure if we can use Maven build profiles to include/exclude module-info.java based on a JDK version (include if JDK is [9,), exclude if JDK is (,8]).

@rfscholte please correct me / add anything. I never use modules as well. Thank you.

@rfscholte
Copy link
Author

That's indeed quite a complete and correct analysis.

A view things to add to that: Maven just passes the files to Java, asking for it's module name.
https://github.com/openjdk/jdk/blob/jdk-21%2B35/src/java.base/share/classes/jdk/internal/module/ModulePath.java#L509 will transform the filename to a modulename based on a couple of rules.
In the end https://github.com/openjdk/jdk/blob/jdk-21%2B35/src/java.base/share/classes/jdk/internal/module/Checks.java#L44 gives the exception (and that message is used by the maven-dependency-plugin)

Profiles are useful for different Java runtimes for Maven. Back in the days of Java 9 that approach would make sense because some code could not be compiled with Java 9 yet. But currently most projects are running Maven with at least Java 11 without any uses, even to create Java 8 compatible code.

What I would do is the following:

      <plugin>
      <groupId>org.apache.maven.plugins</groupId>
      <artifactId>maven-compiler-plugin</artifactId>
      <version>3.13.0</version>
      <executions>
        <execution>
          <id>java8-base</id>
          <configuration>
            <release>8</release>
            <excludes>
              <exclude>module-info.java</exclude>
            </excludes>
          </configuration>
        </execution>
        <execution>
          <id>java11</id>
          <configuration>
            <release>11</release>
            <includes>
              <include>module-info.java</include>
            </includes>
          </configuration>
        </execution>
      </executions>
      </plugin>

Looking at model-2_X:

mvn org.apache.maven.plugins:maven-dependency-plugin:3.8.1:list -DincludeScope=runtime
[INFO] Scanning for projects...
[INFO]
[INFO] --------------------< org.spdx:spdx-java-model-2_X >--------------------
[INFO] Building spdx-java-model-2_X 1.0.0-RC3-SNAPSHOT
[INFO]   from pom.xml
[INFO] --------------------------------[ jar ]---------------------------------
[INFO]
[INFO] --- maven-dependency-plugin:3.8.1:list (default-cli) @ spdx-java-model-2_X ---
[INFO]
[INFO] The following files have been resolved:
[INFO]    org.spdx:spdx-java-core:jar:1.0.0-RC2:compile -- module spdx.java.core (auto)
[INFO]    com.google.code.findbugs:jsr305:jar:3.0.2:compile -- module jsr305 (auto)
[INFO]    org.slf4j:slf4j-api:jar:2.0.3:compile -- module org.slf4j
[INFO]    org.apache.commons:commons-lang3:jar:3.5:compile -- module commons.lang3 (auto)
[INFO]    org.jsoup:jsoup:jar:1.15.3:compile -- module org.jsoup [auto]
[INFO]    com.google.code.gson:gson:jar:2.8.9:compile -- module com.google.gson

This tells me you should not add the module-info.java yet, because I'm pretty sure jsr350 will not be the modulename.
Up until everything has an explicit modulename, stick to providing an Automatic-Module-Name.

@goneall
Copy link
Member

goneall commented Feb 23, 2025

Thanks @rfscholte @bact - I'll work on a PR based on the suggestions

@goneall
Copy link
Member

goneall commented Feb 25, 2025

From reading this article, it looks like adding the Automatic-Module-Name entry in pom.xml will add it to the Manifest and not impact the ability to use the library in Java 8 whereas adding the manifest file will require the above mentioned changes to the POM file to have separate executions for Java 8 and Java 9+. Since supporting multiple Java JDKs is more involved (e.g., I'd like to have the CI test the different JDK compilations), I'm thinking I'll just add the Automatic-Module-Name entry in the following libraries:

I'd like to leave this issue open to either implement the module descriptor once we have to drop support for Java 8 or we implement a more sophisticated CI.

@bact @rfscholte - Thanks again for the advice and pointers. If you disagree with the approach, I'll need a bit more help in constructing the PR which includes all the necessary tests and changes.

@goneall
Copy link
Member

goneall commented Feb 25, 2025

@bact @rfscholte - Please review the following PR's:

This now generates the following dependencies:

[INFO] The following files have been resolved:
[INFO]    org.slf4j:slf4j-api:jar:2.0.7:compile -- module org.slf4j
[INFO]    org.apache.commons:commons-lang3:jar:3.5:compile -- module commons.lang3 (auto)
[INFO]    org.jsoup:jsoup:jar:1.15.3:compile -- module org.jsoup [auto]
[INFO]    com.google.code.gson:gson:jar:2.8.9:compile -- module com.google.gson
[INFO]    com.google.code.findbugs:jsr305:jar:3.0.2:compile -- module jsr305 (auto)
[INFO]    org.spdx:spdx-java-model-2_X:jar:1.0.0-RC3-SNAPSHOT:compile -- module org.spdx.model.v2 [auto]
[INFO]    org.spdx:spdx-java-core:jar:1.0.0-RC3-SNAPSHOT:compile -- module org.spdx.core [auto]
[INFO]    org.spdx:spdx-java-model-3_0:jar:1.0.0-RC3-SNAPSHOT:compile -- module org.spdx.model.v3 [auto]

@bact
Copy link
Collaborator

bact commented Feb 26, 2025

Please review the module name addition in org.spdx.v3jsonldstore

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

No branches or pull requests

3 participants