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

Build Multi-Release Jar #345

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

A248
Copy link

@A248 A248 commented Apr 2, 2021

Builds a multi-release JAR which enables using features from newer JDK versions while still supporting Java 6. For example, any of the following can be done:

  • Implementing Add support for Thread.onSpinWait() while spinning #284 in a clean fashion without any need for reflection or static initializers
  • Switching to VarHandles instead of Unsafe, or if Unsafe is considered better for performance reasons, adding some sort of fallback to VarHandles in case Unsafe is not available
  • Adding a full module-info which adds support for jlink.

Module-Info

This PR also adds module-info to the JDK 11 source set - mainly as an example and first step in what can be accomplished with a MR JAR. This is designed first and foremost to create a multi-release jar so that JCTools can leverage features from new JDK versions.

Compatibility - OSGI and Others

I made sure to maintain the same OSGI information in the manifest. The felix plugin originally thought that META-INF/versions/11 was a package, but that has been fixed. There is still a minor warning because bnd does not think there should be classes inside META-INF/versions/11. It all still works beside that and OSGI supports MR JARs just fine. Also relevant is bndtools/bnd#2227

Besides OSGI, there were a couple webservers which had incompatibilities with module-info.class being placed inside the root of the jar. To address that, most projects add module-info inside META-INF/versions rather than the jar root (which this PR also does). Plenty of popular libraries have done something like this by now so I do not envision an issue for JCTools either.

* Uses separate runs of the maven-compiler-plugin to compile
  the main source sets and the JDK 11 module-info
* Adding the Export-Package instruction to felix is required to
  prevent bnd from thinking META-INF/versions/11 is a package.
@nitsanw
Copy link
Contributor

nitsanw commented Apr 14, 2021

Hi, I'm playing with this PR and verified the jar produced will load in JDK 6 which works. I'm not clear on how we are supposed to add the multi-version classes though. I tried to add a ThreadUtil class under the new source directory, but it didn't work. Can you explain how you see this working? also note the CI failure, please look into it.
Thanks!

@A248
Copy link
Author

A248 commented Apr 14, 2021

The way MRJARs are made, you need to include the class in both source sets. Be careful that they have the same binary interface. So for example, with ThreadUtil:

// src/main/java
public final class ThreadUtil {

  private ThreadUtil() {}

  public static void onSpinWait() {}
}

// src/main/java11
public final class ThreadUtil {

  private ThreadUtil() {}

  public static void onSpinWait() {
    Thread.onSpinWait();
  }
}

@A248
Copy link
Author

A248 commented Apr 14, 2021

The CI seems to be failing because it still uses JDK 8, whereas this PR increases the build-time requirement to JDK 11.

@nitsanw
Copy link
Contributor

nitsanw commented Apr 14, 2021

I added ThreadUtil in a package org.jctools.util the build fails:

package org.jctools.util;

@InternalAPI
public class ThreadUtil {
    public static void onSpinWait() {
        Thread.onSpinWait();
    }
}

Errors:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (compile-java11) on project jctools-core: Compilation failure: Compilation failure: 
[ERROR] /Users/yak/git/JCTools/jctools-core/src/main/java11/org/jctools/util/ThreadUtil.java:[3,2] cannot find symbol
[ERROR]   symbol: class InternalAPI
[ERROR] /Users/yak/git/JCTools/jctools-core/src/main/java11/module-info.java:[2,24] package is empty or does not exist: org.jctools.counters
[ERROR] /Users/yak/git/JCTools/jctools-core/src/main/java11/module-info.java:[3,24] package is empty or does not exist: org.jctools.maps
[ERROR] /Users/yak/git/JCTools/jctools-core/src/main/java11/module-info.java:[4,24] package is empty or does not exist: org.jctools.queues

@nitsanw
Copy link
Contributor

nitsanw commented Apr 14, 2021

This patch needs to update https://github.com/JCTools/JCTools/blob/master/.travis.yml to use JDK 11 then.

@A248
Copy link
Author

A248 commented Apr 15, 2021

That is interesting. The same build is successful in my environment. I've committed ThreadUtil just like you've shown, and both versions are compiled correctly to the right places.

I'm using the following versions on my end and running mvn clean package -DskipTests:

Zulu11.45+27-CA (build 11.0.10+9-LTS)
Apache Maven 3.8.1
Mac OS 10.14.6 (unfortunately)

I've updated the .travis.yml as well. However, it seems that LGTM is still using JDK 8 to build and test the commits.

@nitsanw
Copy link
Contributor

nitsanw commented Apr 19, 2021

I can confirm the code builds with mvn clean install but fails to build with just mvn install

@nitsanw
Copy link
Contributor

nitsanw commented Apr 19, 2021

On reflection, I think the right approach here would be a multi-module setup (which is also IDE friendly) with a build step packing the MR jar from all the module. This is outlined here: https://maven.apache.org/plugins/maven-compiler-plugin/multirelease.html
While doing this it would be good to split the tests into a different module, again to have a better OOTB experience with IDEs. WDYT?

@A248
Copy link
Author

A248 commented Apr 22, 2021

I've been investigating the issue with the current setup and mvn package - it is related to a nasty bug at the intersection of the maven-compiler-plugin, maven-bundle-plugin, module descriptors and MR jars. The MRJAR feature in the maven-compiler-plugin is quite unstable overall. So I agree with you in using the multi-module approach.

The downside I see to the Maven multi-module setup and the reason I was reluctant to use it is that it does not allow easily compiling the JPMS module descriptor. To workaround this I will see about using --patch-module, likely using the exec-maven-plugin. Also, it will be the mrjar module that will have to be deployed.

Regarding the tests, are you suggesting the existing unit tests in jctools-core/src/test/java be moved to their own module? I must be mistaken. As far I have seen those are almost always kept in the same module and IDEs support them fine.

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.

None yet

2 participants