feat(FeatureClassLoader): support class loading of Java 9+ #426

bender316 · 2019-08-01T06:11:07Z

With Java 9 class loading was changed in that way that the bootstrap class loader does not see all classes anymore.
In Java 9+ the platform classloader needs to be used.
A good explanation can be found at fixes #206

It would be great if this fix can be released ASAP as it stalls our migration to Java 11. I'm honestly quite surprised that not more people are affected by this.

With Java 9 class loading was changed in that way that the bootstrap class loader does not see all classes anymore. In Java 9+ the platform classloader needs to be used. A good explanation can be found at fixes #206

nedtwigg · 2019-08-01T17:12:00Z

it stalls our migration to Java 11

If you comment out freshmark, does all of the rest work? Is it only freshmark which is blocked?

I haven't ventured past Java 8 myself yet, so I'm going to rely on the expertise of the rest of the Spotless community to code review this. Looks like spotbugs is upset, run spotbugsMain on your box to see what it's upset about.

jbduncan · 2019-08-01T17:25:54Z

@bender316 I'm also a bit surprised that it's taken this long for Java 9+ related issues to appear with Spotless, because I've been using Spotless for Gradle on a Java 11 project without problems for a while now. 🤔

I'll see if I can find the time to review this PR this weekend. :)

bender316 · 2019-08-02T06:05:49Z

If you comment out freshmark, does all of the rest work? Is it only freshmark which is blocked?

Yes. It's not spotless in general which is affected but single formatters. Since we're only using java, groovy and freshmark formatters I can't tell for other formatters.

freshmark breaks because it uses ScriptEngine and javax.script package does not get picked up by the bootstrap classloader anymore.

lib/src/main/java/com/diffplug/spotless/FeatureClassLoader.java

jbduncan · 2019-08-03T19:04:15Z

I think @nedtwigg's review of this PR is great!

But regardless, from my point of view, if you're able to apply comment, then this PR receives my 👍. :)

lib/src/main/java/com/diffplug/spotless/FeatureClassLoader.java

bender316 · 2019-08-05T06:04:19Z

I'm a bit confused as to how we can get references to all the classes in the Java standard library even though they were put into different modules as of Java 9. Is it because we (Spotless) or Gradle uses the classpath rather than the module path?

@jbduncan It's because the FeatureClassLoader extends URLClassLoader. If URLClassLoader has no set parent it uses the bootstrap classloader. Prior to Java 9 the bootstrap classloader had access to all classes. Starting with Java 9 it has access to a reduces number of classes. Hence we need to use the platform classloader which can access all classes.

bender316 · 2019-08-05T12:25:27Z

Ok, I added a simple JavaVersion class which parses the version string and provides a getter for the major version. In FeatureClassLoader there is now a check if the version is >= 9 and then calls the getPlatformClassLoader method

mohamedanees6 · 2019-08-06T05:19:16Z

lib/src/main/java/com/diffplug/spotless/JavaVersion.java

+
+	static {
+		JAVA_VERSION = System.getProperty("java.version");
+		final String[] versionStrings = JAVA_VERSION.split("\\.");


Can we write like,

double version = Double.parseDouble(System.getProperty("java.specification.version")); if(version > 1.8) { /Do something } else { /Do something }

Just curious, why? and why not? It eliminates the need of this Utility class.

What is Double.parseDouble("1.8.0")?

Seems like java.specification.version just returns 1.x or 11 (in cases of new versioning) and not the full version. I didn't know about this property. So IMO we could use this and get rid of the JavaVersion class.

Ah, apologies https://stackoverflow.com/a/5103166/1153071

Your call @bender316 :)

I will switch to the proposal above and remove the JavaVersion class.

nedtwigg

Great, thanks very much! This looks great, two very small nits and I'm ready to merge.

lib/src/main/java/com/diffplug/spotless/FeatureClassLoader.java

lib/src/main/java/com/diffplug/spotless/JavaVersion.java

bender316 · 2019-08-06T07:53:18Z

Hopefully the last push 😁

I testet it with
openjdk-1.8.0.171 (OpenJDK)
jdk-9.0.4+11 (AdoptOpenJDK)
jdk-11.0.4+11 (AdoptOpenJDK)

So at least it supports older versions and the latests LTS ones.

mohamedanees6 · 2019-08-06T08:46:54Z

Hopefully the last push 😁

I testet it with
openjdk-1.8.0.171 (OpenJDK)
jdk-9.0.4+11 (AdoptOpenJDK)
jdk-11.0.4+11 (AdoptOpenJDK)

So at least it supports older versions and the latests LTS ones.

👍 :) Looks more clean now!!

nedtwigg · 2019-08-07T07:27:10Z

Thanks very much for this fix, @bender316. I'll release by Monday at the latest.

bender316 · 2019-08-07T08:20:59Z

Thanks very much for this fix, @bender316. I'll release by Monday at the latest.

Sounds good. Thank you all for this PR review.

nedtwigg · 2019-08-12T22:51:09Z

Released in x.24.1

bender316 added 2 commits July 31, 2019 16:07

feat(FeatureClassLoader): support class loading of Java 9+

9fce763

With Java 9 class loading was changed in that way that the bootstrap class loader does not see all classes anymore. In Java 9+ the platform classloader needs to be used. A good explanation can be found at fixes #206

added changes and link to PR to CHANGES.md

ac3a496

fixed spotbugs errors

e28d8c3

nedtwigg requested changes Aug 2, 2019

View reviewed changes

lib/src/main/java/com/diffplug/spotless/FeatureClassLoader.java Show resolved Hide resolved

lib/src/main/java/com/diffplug/spotless/FeatureClassLoader.java Show resolved Hide resolved

mohamedanees6 reviewed Aug 4, 2019

View reviewed changes

lib/src/main/java/com/diffplug/spotless/FeatureClassLoader.java Outdated Show resolved Hide resolved

lib/src/main/java/com/diffplug/spotless/FeatureClassLoader.java Outdated Show resolved Hide resolved

implemented Java version check

f4efc4a

lib/src/main/java/com/diffplug/spotless/FeatureClassLoader.java Show resolved Hide resolved

lib/src/main/java/com/diffplug/spotless/JavaVersion.java Outdated Show resolved Hide resolved

use java.specification.version as base for version parsing

e0af947

nedtwigg merged commit ff5ae55 into diffplug:master Aug 7, 2019

nedtwigg mentioned this pull request Aug 9, 2019

Build only: TestProvisioner has become very flaky #429

Closed

nedtwigg mentioned this pull request Aug 20, 2019

removeUnusedImports does not work on Java 9 #83

Closed

feat(FeatureClassLoader): support class loading of Java 9+ #426

feat(FeatureClassLoader): support class loading of Java 9+ #426

Uh oh!

Conversation

bender316 commented Aug 1, 2019

Uh oh!

nedtwigg commented Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbduncan commented Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bender316 commented Aug 2, 2019

Uh oh!

Uh oh!

jbduncan commented Aug 3, 2019

Uh oh!

Uh oh!

bender316 commented Aug 5, 2019

Uh oh!

bender316 commented Aug 5, 2019

Uh oh!

mohamedanees6 Aug 6, 2019

Choose a reason for hiding this comment

Uh oh!

nedtwigg Aug 6, 2019

Choose a reason for hiding this comment

Uh oh!

bender316 Aug 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nedtwigg Aug 6, 2019

Choose a reason for hiding this comment

Uh oh!

bender316 Aug 6, 2019

Choose a reason for hiding this comment

Uh oh!

nedtwigg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bender316 commented Aug 6, 2019

Uh oh!

mohamedanees6 commented Aug 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nedtwigg commented Aug 7, 2019

Uh oh!

bender316 commented Aug 7, 2019

Uh oh!

nedtwigg commented Aug 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nedtwigg commented Aug 1, 2019 •

edited

Loading

jbduncan commented Aug 1, 2019 •

edited

Loading

bender316 Aug 6, 2019 •

edited

Loading

mohamedanees6 commented Aug 6, 2019 •

edited

Loading