Skip to content

Experimental JDK9 module-info support#124

Merged
jstuyts merged 3 commits intoargparse4j:masterfrom
concision:feat/jdk9-support
Oct 1, 2020
Merged

Experimental JDK9 module-info support#124
jstuyts merged 3 commits intoargparse4j:masterfrom
concision:feat/jdk9-support

Conversation

@concision
Copy link
Copy Markdown
Contributor

@concision concision commented Sep 28, 2020

This pull request resolves issue #123 "JDK 9+ Compatability with Project Jigsaw & module-info.class" by adding a Java 9 compilation phase in all modules and upgrading Travis CI to openjdk9.

Maven module changes:

  • argparse4j-root:

    • removed default values for properties jdk.6.rt.jar.path and jdk.7.rt.jar.path, as JDK9+ no longer has a rt.jar file; values must now be explicitly passed as Maven CLI properties (e.g. -Djdk.6.rt.jar.path=...) or passed through the environment variables JAVA6_HOME and JAVA7_HOME
    • added Maven property to allow skipping JDK 6/7 path validation with -DskipJdkPathValidation=true
    • enforce a JDK9+ JAVA_HOME, otherwise the build fails
    • bumped org.apache.maven.plugins:maven-compiler-plugin from 3.6.2 to 3.8.1 for better Java 9 support
    • bumped com.github.siom79.japicmp:japicmp-maven-plugin from 0.10.0 to 0.14.3 to resolve a class level error (i.e. older plugin did not support Java 9 bytecode)

  • argparse4j, argparse4j-java7, argparse4j-hadoop (grouped for brevity):

    • added a JDK9 module compilation phase jdk9-module-compile that compiles all classes to Java 9 (including module-info.java); this is used to generate and validate the module-info class.
    • moved the default-compile compilation phase to jdk6-compile that compiles all classes to Java 6 (or Java 7, in the case of argparse4j-java7), overwriting the Java 9 classes (excluding module-info.class)
    • set the default-testCompile test compilation phase to Java 6 (or Java 7, in the case of argparse4j-java7)
    • added a module-info.java that exposes all packages publicly, with the following module names:
      • argparse4j module name: net.sourceforge.argparse4j
      • argparse4j-java7 module name: net.sourceforge.argparse4j.ext.java7
      • argparse4j-hadoop module name: net.sourceforge.argparse4j.ext.hadoop

  • argparse4j-hadoop:

    • excluded jdk.tools:jdk.tools from org.apache.hadoop:hadoop-common due to it causing a dependency resolution error

Caveats:

  • the argparse4j-hadoop module requires the Maven dependency org.apache.hadoop:hadoop-common, which does not have a Java 9 module-info.class
  • building the project now requires JDK 6, 7, and 9 installations

Travis CI Changes:

  • removed Maven install command in the install stage, as this was effectively building the project twice (and not just downloading plugins/dependencies)
  • moved/updated all JDK switching commands to install stage
  • removed explicit -Djdk.6.rt.jar.path=... and -Djdk.7.rt.jar.path=... arguments during the script stage, as they are now inferred from JAVA6_HOME and JAVA7_HOME

@jstuyts I think these changes are ready for feedback. Projects with Java 9 module support that have a requires argparse4j will need to migrate to requires net.sourceforge.argparse4j. I have granted permission for project maintainers to update my source branch feat/jdk9-support. I imagine the semver version needs to be updated to reflect these changes.

As an aside, this took significantly longer than I expected due to various strange Maven interactions.

Thanks in advance.

@concision concision mentioned this pull request Sep 28, 2020
Closed
@concision
Copy link
Copy Markdown
Contributor Author

concision commented Sep 28, 2020

Since this pull request makes the change that JDK9+ is required for compiling the project, I suspect that the functionality from #95 "Ensure compatibility with Java 1.6 and 1.7 when running Maven using 1.8" can be replaced using JDK9+'s new -release flag, which later versions of org.apache.maven.plugins:maven-compiler-plugin support. The ramifications of this potential change would simplify the Maven configuration for compiling against Java 6/7 boot classpaths and remove the dependency on JDK 6 /7 (i.e. only one JDK9+ is required).

@jstuyts
Copy link
Copy Markdown
Collaborator

jstuyts commented Sep 29, 2020

Thanks for the tip about the --release flag. The JDK6 and 7 compilation also ensure that no classes from later versions are used, but the flag seems to cover that too. Let's not make the change as part of this PR, but do it in a separate issue.

@concision
Copy link
Copy Markdown
Contributor Author

concision commented Sep 29, 2020

I am willing to author another PR to make use of the --release flag after this one is reviewed/merged. I think it significantly simplifies the build configuration and is more developer-friendly (only one required JDK; no JAVA6_HOME environment variable). I did some preliminary testing with the release compiler property and it seems to behave in the same way as the JDK 6/7 bootclasspath setups - ensuring no later classes or methods are used.

@concision
Copy link
Copy Markdown
Contributor Author

concision commented Sep 29, 2020

After some talking with a developer on a another pull request of mine to integrate module support (see kichik/pecoff4j#124), I think it might be worthwhile to build a multi-release JAR (here is a brief overview of multi-releases). While not absolutely necessary for module support, it may have better compatibility with Java tooling. Let me know what your thoughts are.

@jstuyts
Copy link
Copy Markdown
Collaborator

jstuyts commented Sep 29, 2020

A multi-release JAR is not necessary in this case as only 1 additional class file that won't break older JDKs is needed. But I am not against an MRJAR if this makes the Maven POM a lot cleaner. I suggested to change the build in a separate issue earlier, but as this PR changes the build significantly anyway I don't mind if you change it to an MRJAR in this PR if it simplifies things a lot.

And about the version number: The changes you have made are backwards compatible, so only a revision increase would be sufficient. The head is currently at 0.9.0-SNAPSHOT which is a higher minor version than the last release. These changes could also be done in 0.8.2.

@concision
Copy link
Copy Markdown
Contributor Author

concision commented Sep 29, 2020

I do not think a MRJAR would simplify the Maven POM (quite the opposite) - I will leave it out.

To be clear - should I be changing the version number (to 0.8.2) in this PR or is that left to the project maintainers in another commit? I imagine if we would also want to make the the JDK6/7 --release changes prior to any release/version change.

Is there anything that needs to be changed in this pull request before potentially merging?

@jstuyts
Copy link
Copy Markdown
Collaborator

jstuyts commented Sep 29, 2020

Leave the version number as is. I will change it what I think is appropriate before a release.

The new compilation using --release can certainly be included in the next release as it does not affect the API and functionality.

I already reviewed your changes, and build it locally. It is looking great, but I want to have 1 more careful look before I merge. I expect that I can merge within the next couple of days.

@jstuyts jstuyts marked this pull request as ready for review October 1, 2020 15:24
@jstuyts jstuyts merged commit 783f8ac into argparse4j:master Oct 1, 2020
@concision concision mentioned this pull request Oct 2, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@concision @jstuyts