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

Add module-info #761

Merged
merged 2 commits into from
Sep 3, 2023
Merged

Add module-info #761

merged 2 commits into from
Sep 3, 2023

Conversation

bowbahdoe
Copy link
Contributor

@bowbahdoe bowbahdoe commented Aug 16, 2023

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

@bowbahdoe bowbahdoe mentioned this pull request Aug 16, 2023
@stleary
Copy link
Owner

stleary commented Aug 17, 2023

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

@javadev
Copy link
Contributor

javadev commented Aug 20, 2023

      <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
Copy link
Contributor Author

bowbahdoe commented Aug 20, 2023

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
Copy link
Contributor

javadev commented Aug 21, 2023

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
Copy link
Contributor Author

bowbahdoe commented Aug 23, 2023

@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
Copy link
Contributor

javadev commented Aug 24, 2023

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

@johnjaylward
Copy link
Contributor

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

I don't have write access

@stleary
Copy link
Owner

stleary commented Aug 24, 2023

Is some Gradle work still needed for this PR?

@johnjaylward
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Owner

stleary commented Aug 25, 2023

@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
Copy link
Owner

stleary commented Aug 25, 2023

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
Copy link
Contributor

@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
Copy link
Contributor Author

Will this require a new release?
No

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

@johnjaylward
Copy link
Contributor

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
Copy link
Contributor Author

bowbahdoe commented Aug 25, 2023

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

@stleary stleary merged commit a309931 into stleary:master Sep 3, 2023
5 checks passed
stleary pushed a commit that referenced this pull request Oct 13, 2023
stleary added a commit that referenced this pull request Oct 13, 2023
@stleary
Copy link
Owner

stleary commented Oct 13, 2023

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

@johnjaylward
Copy link
Contributor

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
Copy link
Owner

stleary commented Oct 14, 2023

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
Copy link
Contributor

@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.

johnjaylward pushed a commit to johnjaylward/JSON-java that referenced this pull request Oct 16, 2023
This reverts commit b180dbe.
johnjaylward pushed a commit to johnjaylward/JSON-java that referenced this pull request Oct 16, 2023
This reverts commit b180dbe.
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

Successfully merging this pull request may close these issues.

4 participants