Use the Java 9 API to parse module descriptors

[plamentotev]

Hi,

What do you think about the idea to make Plexus Java multi-release JAR so on Java 9+ it uses the APIs provided by Java SE. For example now if you compile module A on Java 10 and use Maven to compile B that depends on A, it will fail because ASM is not compatible with the new class format introduced with Java 10.

[rfscholte]

MRJars are kind of terrible to maintain and test. It should be possible to implement it while keeping source+target on 7 and just put it under src/main/java. At runtime you can look for a certain class to decide if ASM should be used or the Java9 API.

[rfscholte]

Let's pick this up. This should make it possible to use EA versions of Java without having to wait for ASM. We've already been hit by Java 10 and 11...

[rfscholte]

@plamentotev what about this? My IDE doesn't like it, but Maven does. Just need to ensure to use at least Java9 when releasing.

[plamentotev]

@rfscholte I have to admit that is just now that I realized what you meant with "MRJars are kind of terrible to maintain and test". I tried importing the project both in Eclipse and IntelliJ IDEA. IDEA does not recognize the Java 9 version of the class but otherwise seems to work fine with the rest of the project. Eclipse didn't recognized the project as Java project at all - I guess this is your IDE.

As for the testing - unfortunately both IDEA and the Surefire plugin are testing only the classes inside src/main/java so the Java 9 version of BinaryModuleInfoParser is not tested even when the build is run on JDK 9. As a matter of fact there are some issues with the implementation(unfortunately I didn't have the time to look at them today). For example looks like it does not work well with jmod files. If the BinaryModuleInfoParserTest is executed in the integration-test phase by Surefire Failsafe, I expect that the proper class will be tested. But I wonder if its worth the trouble. To me your suggestion to use Java 7 only is the better option until the tooling has a better support for MRJars. What do you think?

[rfscholte]

Good find! Added e219cfd to ensure that the Java 9 code is tested.

So it seems that a MultiRelease jar can only be tested as jar, not as exploded classes in the output directory, You must test this during the integration-test phase. This is quite inconvenient when working with IDEs.

Eclipse does recognize it as a Java project on my system, though

[plamentotev]

Sorry, my bad - I meant to say "If the BinaryModuleInfoParserTest is executed in the integration-test phase by Failsafe...", not "...by Surefire...". Failsafe uses the Jar file created during the package phase so java picks up the correct class. The following configuration tests the proper class:

    <plugin>
      <groupId>org.apache.maven.plugins</groupId>
      <artifactId>maven-failsafe-plugin</artifactId>
      <version>2.21.0</version>
      <executions>
        <execution>
          <goals>
            <goal>integration-test</goal>
            <goal>verify</goal>
          </goals>
        </execution>
      </executions>
      <configuration>
        <includes>
            <include>**/*Test.java</include>
            <include>**/IT*.java</include>
            <include>**/*IT.java</include>
            <include>**/*ITCase.java</include>
        </includes>      
      </configuration>
    </plugin>

There are some test failures though.

[rfscholte]

I wanted to reuse the configuration of surefire during integration-test and even noticed differences between the 2 runs. But the result was indeed misleading, failsafe is the preferred solution. Now it is looking much better.
For me the benefits of turning this into a multirelease jar are more compared to the IDE struggles and the reflection approach we had to do if we don't want do depend on ASM.
Main thing for me: no need to catch up with every new Java release.

[plamentotev]

@rfscholte I agree with you. I was concerned about the tests but now looks good to me.

[plamentotev]

Also I wonder if we can use the enforcer plugin to ensure that at least JDK 9 is used during a release(to avoid releasing without the Java 9 classes by mistake).

[rfscholte]

That idea crossed my mind as well. There are only a few that will release this library and they all know about it, so we should be good. But just in case: let's add a release-profile, just to be sure.

[rfscholte]

@plamentotev do you think it is ready to merge?

[plamentotev]

Yes, looks great.

…

On Wed, May 23, 2018, 20:25 Robert Scholte ***@***.***> wrote: @plamentotev <https://github.com/plamentotev> do you think it is ready to merge? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMLobXD2P9rON9LYdYOZ_6JM32XCkpCQks5t1ZuSgaJpZM4RxKZS> .