Add module-info

[bowbahdoe]

This adds a module info for the reasons described in #760.

For users of Java 6, 7, or 8 this should not make any difference. The module-info.class should be inert under META-INF/versions/9/module-info.class.

The way I implemented it here was with moditect, which directly generates the module-info. There are pros and cons to this, but a small pro is that people should be able to at least run package with JDK 8

[stleary]

@bowbahdoe Thanks for taking the initiative. Will leave this PR up for some days to see if it draws any comments or concerns.

[javadev]

      <plugin>
        <groupId>org.apache.maven.plugins</groupId>
        <artifactId>maven-jar-plugin</artifactId>
        <version>3.3.0</version>
        <configuration>
          <archive>
            <manifestEntries>
              <Automatic-Module-Name>org.json</Automatic-Module-Name>
            </manifestEntries>
          </archive>
        </configuration>
      </plugin>

It's a better configuration since it utilizes the standard Maven plugin.

[bowbahdoe]

Hey @javadev - That does not solve the issue here. Adding an automatic module name (which is already done) just gives a stable name that isn't dependent on the jar name. It does not allow for usage of jlink or jpackage.

If it is a concern using the moditect plugin, there is an alternate implementation where we include the source of the module-info and configure the compiler plugin to do a second compile. (we would add a src/main/java9)

Personally I found this easier, but the other way is an option. It would mean that folks would have to use Java 9+ to compile,
but if there is a desire to just use "standard" plugins we can go that way.

[javadev]

I believe using the module info and Java 9+ compiler is a good idea. We should establish Java 9+ as the minimum required version first.

[bowbahdoe]

@javadev Can you elaborate on what the benefits you see for making Java 9+ the minimum required version?

Any thoughts not already communicated in #741?

[javadev]

John Jaylward has been granted approval for the pull request, and it is now ready for merging.

[johnjaylward]

John Jaylward has been granted approval for the pull request, and it is now ready for merging.

I don't have write access

[stleary]

Is some Gradle work still needed for this PR?

[johnjaylward]

Is some Gradle work still needed for this PR?

No, the pipelines all use maven, and the deployment is maven based too. I believe gradle changes would only be needed if you wanted to support it from a standalone build

[johnjaylward]

Is some Gradle work still needed for this PR?

I just tried building a clean pull of master using gradle on Windows 10, and the java compiler gave me a stack overflow. The maven build on the same system worked fine. I'm guessing we should just remove the gradle build as it seems to not be working and no one has noticed.

.\gradlew build

> Task :compileTestJava FAILED
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
compiler message file broken: key=compiler.misc.msg.bug arguments=11.0.18, {1}, {2}, {3}, {4}, {5}, {6}, {7}
java.lang.StackOverflowError
        at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCLiteral.accept(JCTree.java:2292)
        at jdk.compiler/com.sun.tools.javac.comp.Attr.attribTree(Attr.java:655)
        at jdk.compiler/com.sun.tools.javac.comp.Attr.attribArgs(Attr.java:751)
        at jdk.compiler/com.sun.tools.javac.comp.Attr.visitApply(Attr.java:1997)
        at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCMethodInvocation.accept(JCTree.java:1650)
        at jdk.compiler/com.sun.tools.javac.comp.Attr.attribTree(Attr.java:655)
        at jdk.compiler/com.sun.tools.javac.comp.Attr.visitSelect(Attr.java:3584)
        at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCFieldAccess.accept(JCTree.java:2130)

[stleary]

@johnjaylward I am able to do a clean build using Java 8 on a mac pro. Please let me know if you get any insights into why the build has an error in your environment.

[stleary]

What problem does this code solve?
Update Maven builds to make use of model-info.java to support JLink.
Should only impact personal builds that use Java 9 or greater.

Risks
Low.
Should be transparent to Java builds 8 and below
Only Maven builds are affected, no change to Gradle builds.

Changes to the API?

Will this require a new release?
No

Should the documentation be updated?
No

Does it break the unit tests?
N/A

Was any code refactored in this commit?
No

Review status
APPROVED

Starting 3-day comment window

[johnjaylward]

@johnjaylward I am able to do a clean build using Java 8 on a mac pro. Please let me know if you get any insights into why the build has an error in your environment.

Looks like I initially tested the gradle build against JVM 17. When I explicitly switched to both JVM 1.8 and JVM 11, both of those build fine under gradle.

[bowbahdoe]

Will this require a new release?
No

So is this not going to lead to a maven central release?

[johnjaylward]

Will this require a new release?
No

So is this not going to lead to a maven central release?

"require new release" is usually only "yes" for security issues, or major bug fixes. I'm not sure this counts as major.

When a new release happens, this change will be in maven central, but this merge itself is not forcing a release.

[bowbahdoe]

Gotcha - yep that makes sense. This definitely does not count as major

[stleary]

Reverted during Release 20231013 since it broke deployment to the Maven repo.

[johnjaylward]

Reverted during Release 20231013 since it broke deployment to the Maven repo.

@stleary this doesn't seem like a good reason to revert this. The release should be updated to work.

[stleary]

There is a Wiki page How to make a new JSON-Java release with clear and detailed instructions on how to perform a release. PRs that include changes to pom.xml will need to demonstrate that it does not affect the release instructions, or provide the necessary workaround. I think this is a fair approach that allows for changes but protects the functioning of the project.

[johnjaylward]

@stleary what errors were there? Can you open an issue with reference to this PR and what failed so someone can correct the problem. Supporting java9+ makes this PR required and just revering it without offering any additional information or a fix does seem right.