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

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

Merged
merged 5 commits into from
Aug 7, 2019
Merged

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

merged 5 commits into from
Aug 7, 2019

Conversation

bender316
Copy link
Contributor

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 http://java9.wtf/class-loading/

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 http://java9.wtf/class-loading/

fixes #206
@nedtwigg
Copy link
Member

nedtwigg commented Aug 1, 2019

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
Copy link
Member

jbduncan commented Aug 1, 2019

@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
Copy link
Contributor Author

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.

@jbduncan
Copy link
Member

jbduncan commented Aug 3, 2019

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

@bender316 I personally don't have anything else to add, and if anything I'm probably unqualified to review this PR because even after reading http://java9.wtf/class-loading/, 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?

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

@bender316
Copy link
Contributor Author

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
Copy link
Contributor Author

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is Double.parseDouble("1.8.0")?

Copy link
Contributor Author

@bender316 bender316 Aug 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, apologies @mohamedanees6, I didn't notice java.specification.version vs java.version. That does look better to me, especially since java.version seems to have an unreliable format: https://stackoverflow.com/a/5103166/1153071

Your call @bender316 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@bender316
Copy link
Contributor Author

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
Copy link

mohamedanees6 commented Aug 6, 2019

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 nedtwigg merged commit ff5ae55 into diffplug:master Aug 7, 2019
@nedtwigg
Copy link
Member

nedtwigg commented Aug 7, 2019

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

@bender316
Copy link
Contributor Author

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
Copy link
Member

Released in x.24.1

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.

java.lang.NoClassDefFoundError: javax/script/ScriptException with freshmark on Java9
4 participants