Add module descriptor while keeping workable Eclipse IDE experience

[A248]

Considering it was noted that the module descriptor broke usage in Eclipse/buildship, I took care to ensure that Caffeine can still compile in Eclipse.

I developed and tested this solution using Eclipse 2021-03 (4.19.0). It relies on the Module Dependencies tab of the project build path menu to fix compilation in Eclipse/buildship.

Hamcrest v1 had to be excluded from transitive dependencies since it lead to a split package clash.

FastUtil had to be downgraded to 8.5.2 because of split packages: vigna/fastutil#245

Resolves #535

[ben-manes]

A big thank you! I had experimented but didn't get nearly as far as you did. I merge this in a bit later when I get a chance to try it out.

In case you didn't see and it was too hidden away in the build, fyi that I had disabled modularity globally in codeQuality.gradle. I suspect you didn't turn it on globally due to other sourceSets, but fyi if that's something that you think should be enabled.

caffeine/gradle/codeQuality.gradle

Line 151 in ba78559

modularity.inferModulePath.set(false)

[ben-manes]

I think it would be okay to move the split packaged Guava tests into the caffeine module. That was laziness, to be honest, since those tests are JUnit4-based and this project uses TestNG. It has been a little while since I've setup the dual suites, but it wasn't hard.

We can do that later and it's not important, since the other modules really won't be used in a modular application (upgraded past Guava, no one uses JCache as horrible spec, Simulator is for research not users).

[A248]

Oh, that explains why I had to add inferModulePath explicitly. I was a little perplexed about that considering the Gradle 7 release notes explain it is enabled by default. While I don't think it would break anything if inferModulePath was enabled globally, it's probably fine to keep it disabled for now and modularize one-by-one. As unfortunate as it is, placing dependencies and sources on the module-path has some tricks and traps due to the way other libraries are not fully tested with it like they are on the class-path.

I'm wondering as to whether I should find a way to determine the module names of the test dependencies automatically. Do you think that would be helpful to reduce the burden of adding more test dependencies?

I originally modularized the Guava module as well but reverted the change because of that. As far as I've seen the caffeine/guava bridge is mostly used by legacy applications anyway. However, split packages can cause other issues so I would agree with resolving them.

[ben-manes]

I think we should remove the global inferModulePath and set it on each project. That way it is more explicit about ordering, as I'd have expected the global setting to be applied later since its after the project's build.gradle is applied.

It seems very quirky in Eclipse given the Gradle docs about how they use classpath for test dependencies, but Eclipse forces to modules. I'd have hoped modularizing test dependencies wouldn't be needed. I'm not sure if there is an automatic way, but running in the IDE is needed for debugging.

[ben-manes]

When I try running a test through the IDE then I get the following error:

WARNING: Unknown module: com.google.common specified to --add-reads
WARNING: Unknown module: com.google.googlejavaformat specified to --add-reads
WARNING: Unknown module: guava.testlib specified to --add-reads
WARNING: Unknown module: it.unimi.dsi.fastutil specified to --add-reads
WARNING: Unknown module: org.apache.commons.lang3 specified to --add-reads
WARNING: Unknown module: org.cache2k.api specified to --add-reads
WARNING: Unknown module: org.hamcrest specified to --add-reads
WARNING: Unknown module: org.jctools.core specified to --add-reads
WARNING: Unknown module: org.mockito specified to --add-reads
WARNING: Unknown module: org.testng specified to --add-reads
[RemoteTestNG] detected TestNG version 7.4.0
java.lang.TypeNotPresentException: Type Encountered problems when parsing the annotation on method com.github.benmanes.caffeine.cache.CacheTest.serialize(). Please check if all classes being referred to, in the annotation are available in the classpath. not present
	at org.testng.internal.annotations.AnnotationHelper.getAnnotationFromMethod(AnnotationHelper.java:382)
	at org.testng.internal.annotations.JDK15AnnotationFinder.findAnnotation(JDK15AnnotationFinder.java:115)
	at org.testng.internal.annotations.JDK15AnnotationFinder.findAnnotation(JDK15AnnotationFinder.java:104)
	at org.testng.internal.TestNGClassFinder.createObjectFactory(TestNGClassFinder.java:226)
	at org.testng.internal.TestNGClassFinder.<init>(TestNGClassFinder.java:65)
	at org.testng.TestRunner.initMethods(TestRunner.java:443)
	at org.testng.TestRunner.init(TestRunner.java:339)
	at org.testng.TestRunner.init(TestRunner.java:292)
	at org.testng.TestRunner.<init>(TestRunner.java:223)
	at org.testng.remote.support.RemoteTestNG6_12$1.newTestRunner(RemoteTestNG6_12.java:33)
	at org.testng.remote.support.RemoteTestNG6_12$DelegatingTestRunnerFactory.newTestRunner(RemoteTestNG6_12.java:66)
	at org.testng.ITestRunnerFactory.newTestRunner(ITestRunnerFactory.java:55)
	at org.testng.SuiteRunner$ProxyTestRunnerFactory.newTestRunner(SuiteRunner.java:659)
	at org.testng.SuiteRunner.init(SuiteRunner.java:173)
	at org.testng.SuiteRunner.<init>(SuiteRunner.java:107)
	at org.testng.TestNG.createSuiteRunner(TestNG.java:1300)
	at org.testng.TestNG.createSuiteRunners(TestNG.java:1276)
	at org.testng.TestNG.runSuitesLocally(TestNG.java:1125)
	at org.testng.TestNG.runSuites(TestNG.java:1063)
	at org.testng.TestNG.run(TestNG.java:1031)
	at org.testng.remote.AbstractRemoteTestNG.run(AbstractRemoteTestNG.java:115)
	at org.testng.remote.RemoteTestNG.initAndRun(RemoteTestNG.java:251)
	at org.testng.remote.RemoteTestNG.main(RemoteTestNG.java:77)
Caused by: java.lang.IllegalAccessError: class com.github.benmanes.caffeine.cache.testing.CacheSpec (in module com.github.benmanes.caffeine) cannot access class com.google.common.util.concurrent.ThreadFactoryBuilder (in unnamed module @0x783a467b) because module com.github.benmanes.caffeine does not read unnamed module @0x783a467b
	at com.github.benmanes.caffeine/com.github.benmanes.caffeine.cache.testing.CacheSpec.<clinit>(CacheSpec.java:619)
	at java.base/java.lang.Class.forName0(Native Method)
	at java.base/java.lang.Class.forName(Class.java:315)
	at jdk.proxy2/com.sun.proxy.jdk.proxy2.$Proxy4.<clinit>(Unknown Source)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:490)
	at java.base/java.lang.reflect.Proxy.newProxyInstance(Proxy.java:1022)
	at java.base/java.lang.reflect.Proxy.newProxyInstance(Proxy.java:1008)
	at java.base/sun.reflect.annotation.AnnotationParser$1.run(AnnotationParser.java:306)
	at java.base/sun.reflect.annotation.AnnotationParser$1.run(AnnotationParser.java:304)
	at java.base/java.security.AccessController.doPrivileged(Native Method)
	at java.base/sun.reflect.annotation.AnnotationParser.annotationForMap(AnnotationParser.java:304)
	at java.base/sun.reflect.annotation.AnnotationParser.parseAnnotation2(AnnotationParser.java:294)
	at java.base/sun.reflect.annotation.AnnotationParser.parseAnnotations2(AnnotationParser.java:121)
	at java.base/sun.reflect.annotation.AnnotationParser.parseAnnotations(AnnotationParser.java:73)
	at java.base/java.lang.reflect.Executable.declaredAnnotations(Executable.java:604)
	at java.base/java.lang.reflect.Executable.declaredAnnotations(Executable.java:602)
	at java.base/java.lang.reflect.Executable.getAnnotation(Executable.java:572)
	at java.base/java.lang.reflect.Method.getAnnotation(Method.java:695)
	at org.testng.internal.annotations.AnnotationHelper.getAnnotationFromMethod(AnnotationHelper.java:377)
	... 22 more

This might be related to observing the warning The type CacheContext is not exported from this module for all test methods, which are parameterized to accept this test utility object.

Do you know how to resolve these issues?

I made some minor style changes, unrelated to these issues. Can you please review and apply them?
eclipse.patch.txt.

[A248]

Oddly, I do not run into that issue, but a different one, still relating to JPMS. When I look further into testing, the problems multiply.

• Elasticsearch generates Provider class org.apache.lucene.search.suggest.document.Completion50PostingsFormat not in module, due to Make it possible to use the high level rest client with modularized (jigsaw) applications elastic/elasticsearch#38299 which is an umbrella issue for Elasticsearch supporting JPMS. This is what happens when I run tests using Eclipse-TestNG 7.3.0.
• More compilation errors occur in src/jmh/java regarding resolution of dependencies. Strangely, these do not show up under Window > Show View > Problems which is why I didn't notice this before. Fixing this requires adding yet more modules to the already onerous def modules = [ 'java.compiler', 'java.logging', .... Moreover, a greater share of these dependencies have unstable filename-based module names.

Solving modularization using this approach is like slaying a hydra. These issues will take time for the ecosystem to resolve. In the meantime, I have another idea to tackle this.

The Eclipse project can be kept on the classpath. Building through Gradle would take into account the module descriptor, but for Eclipse it would be totally ignored. This would have the benefit of avoiding this massive can of worms consisting of problems which should be solved by dependencies like Elasticsearch and helped by build tools like Eclipse-Gradle. I'm not sure if this would have other implications for the release process. Let me know what you think.

I've experimented with this via an explicit exclusion of module-info in the Eclipse Gradle plugin. This configuration would replace all the current module-related groovy code. The following excludes module-info from compilation in Eclipse:

      def main = entries.find { it instanceof SourceFolder && it.path == 'src/main/java' }
      main.excludes.add('module-info.java')

Which results in the appropriate modification of .classpath:

<classpathentry excluding="module-info.java" kind="src" output="bin/main" path="src/main/java">

[ben-manes]

Oh, that is awesome! I was frustrated that turning off Gradle modularization still resulted in Eclipse doing so, because it detected the file. Making it ignore it, but having the build validate it, is a really elegant approach. Let's do that!

[A248]

All looks well. Let me know if this is successful with your setup.

[ben-manes]

This looks wonderful. I saw a Eclipse race where it tried to honor module-info, but a subsequent clean import worked fine. Thanks for figuring this out. 😄

[ben-manes]

Please rebase and minor style changes, and I'll merge!

[A248]

All done. Hopefully dependencies and IDEs will catch up with the module system soon.

[ben-manes]

Thanks! I hope others learn from your trick here and it helps their adoption of the module system.

[ben-manes]

@A248 can I upgrade FastUtil? It seems fine in a local build, as I presume it was to deal with Eclipse which you made a non-issue.

[A248]

Yes, that was for Eclipse. With fastutil 8.5.4 the gradle build succeeded.