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

Make java9 work again #18496

Merged
merged 5 commits into from May 21, 2016

Conversation

Projects
None yet
5 participants
@rjernst
Copy link
Member

commented May 20, 2016

This change makes ES compile with java9 again, build 118.

  • There are a handful of changes due to failure to determine types during compile.
  • The attachment plugins which use tika needed to have tika upgraded in order to pickup fixes there for java 9.
  • azure discovery and s3 repository indirectly depend on jaxb, which is no longer in the default modules. They now add a jaxb dependency externally, and make JarHell allow for this package.

Note: We shouldn't need to ignore the options xlint class, I will handle this in a follow up to use java 9's new-release` argument (which is a better -target/-source and actually works correctly, instead of the problem we get the warning about here).

@rmuir

View changes

core/src/main/java/org/elasticsearch/bootstrap/JarHell.java Outdated
clazz.startsWith("javax.activation") ||
clazz.startsWith("com.sun.activation.registries")) {
return; // java 9 removed from default modules ...
}

This comment has been minimized.

Copy link
@rmuir

rmuir May 20, 2016

Contributor

this change isn't correct. this is for duplicate classes within the same jar. if that is happening it is indicative of some other problem (e.g. bug in the build)

This comment has been minimized.

Copy link
@uschindler

uschindler May 21, 2016

Contributor

I think this is because we must allow now duplicate classes, because the plugins (azure) that need this now bundle JAXB classes as dependency. If you compile with Java 8, you would see duplicate classes (the one in rt.jar and the dependency one). With Java 9 you would only see one.

I am not sure if the whole thing is right to do. If some modules depends on JAXB, the command line to start ES should add -addmodules javax.xml.bind or similar.

This comment has been minimized.

Copy link
@rmuir

rmuir May 21, 2016

Contributor

No, its not correct. This is the part of the code that checks for duplicate entries within the same zip.

@s1monw

This comment has been minimized.

Copy link
Contributor

commented May 20, 2016

LGTM in general @rjernst I think let move here and get this in. thanks for working on this

@@ -177,7 +177,8 @@ public void testScalingThreadPoolThreadsAreTerminatedAfterKeepAlive() throws Int
}
});
}
assertThat(stats(threadPool, threadPoolName).getThreads(), equalTo(128));
int threads = stats(threadPool, threadPoolName).getThreads();
assertEquals(128, threads);

This comment has been minimized.

Copy link
@jasontedor

jasontedor May 20, 2016

Member

It seems like something in the compiler is broken here. Is there a compiler bug open for this? Or is there a spec change that makes the change here necessary and the compiler is not broken?

This comment has been minimized.

Copy link
@uschindler

uschindler May 21, 2016

Contributor

You are free to open a bug in Oracle's bug tracker! You can also investigate what they already have. But as this is not reproducible in isolation, I don't think you have a chance to fix this. Better to kill assertThat brokenness. I like it that Java 9 complains automatically about that stuff :-)

This comment has been minimized.

Copy link
@rjernst

rjernst May 21, 2016

Author Member

Yeah I tried to reproduce with an isolated test, going as far as trying to replicate the exact class hierarchy/generics/signatures, but I could never get it to fail...

@jasontedor

This comment has been minimized.

Copy link
Member

commented May 20, 2016

I'm with @s1monw, let's just keep moving on this and get to a point quickly where we can get CI turned on for Java 9 now that they are nearing FC and the JDK 9 umbrella JSR draft has been released.

@uschindler

This comment has been minimized.

Copy link
Contributor

commented May 21, 2016

In general: For real cross-compiling you should no longer use javac "source/target" on Java 9 and instead use the new "release" switch. Lucene already switched to this: https://issues.apache.org/jira/browse/LUCENE-7292

This ensures real cross-compilation, because it uses the method signatures and classes of Java 8 for compile.

@rmuir

This comment has been minimized.

Copy link
Contributor

commented May 21, 2016

i'm -1 to the changes because they are not correct. I see others trying to rush this change in but i do not care.

@rjernst rjernst force-pushed the rjernst:java9 branch to 1d40c4b May 21, 2016

@rjernst

This comment has been minimized.

Copy link
Member Author

commented May 21, 2016

@uschindler I plan to switch to -release in a followup, as I noted in the PR description.

Make java9 work again
This change makes ES compile with java9 again, build 118.
* There are a handful of changes due to failure to determine types during compile.
* The attachment plugins which use tika needed to have tika upgraded in order to pickup fixes there for java 9.
* azure discovery and s3 repository indirectly depend on jaxb, which is no longer in the default modules. They now add a jaxb dependency externally, and make JarHell allow for this package.
@rjernst

This comment has been minimized.

Copy link
Member Author

commented May 21, 2016

@rmuir I removed the changes to JarHell. These were unintentional. I originally thought they were necessary, before realizing the failures I was seeing were from the thirdPartyAudit jarhell checks. But after adding all the classes to be ignored there, I never went back and removed the incorrect changes to JarHell.

rjernst added some commits May 21, 2016

@rjernst rjernst force-pushed the rjernst:java9 branch from b26f571 to 1d40c4b May 21, 2016

rjernst added some commits May 21, 2016

@rjernst

This comment has been minimized.

Copy link
Member Author

commented May 21, 2016

I pushed some new commits which fix a log4j issue with java 9.

@rmuir

This comment has been minimized.

Copy link
Contributor

commented May 21, 2016

+1 for the current patch!

@rjernst rjernst merged commit cec4b9a into elastic:master May 21, 2016

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@rjernst rjernst deleted the rjernst:java9 branch May 21, 2016

@uschindler

This comment has been minimized.

Copy link
Contributor

commented May 21, 2016

Hi,
thanks for fixing. I tested compiling painless with Gradle 2.13 using Java 9 build 118: works

FYI: There is a fix for the setAccessible problem around the "nebula something jar" (after hamster scanning for projects). The problem is a bug in Groovy 2.4.4 that was fixed in 2.4.6 (I reported it last year: https://issues.apache.org/jira/browse/GROOVY-7587). Problem is Gradle 2.13 still uses 2.4.4.

To make Gradle 2.13 working with Java 9 do the following:

  • download groovy-all-2.4.6.jar
  • rename it to groovy-all-2.4.4.jar (jaja, you have to do this!)
  • replace the grovy-all-2.4.4.jar in $GRADLE/lib dir
@uschindler

This comment has been minimized.

Copy link
Contributor

commented May 21, 2016

@rjernst @rmuir I have an idea how to remove the useless JAXB dependency: Is far as I know, JAXB is only used for the stupid Base64 decoder/encoder. How about the following: Remove JAXB.jar and jarhell fixes. Place a 3-liner java class named javax.xml.bind.DatatypeConverter which just implements the stupid printBase64Binary and parseBase64Binary using java.util.Base64. Just add a Jarhell exception for this single class with 2 static methods. As its part of the plugin, it is isolated by classloader and may not conflict with other stuff using a real JAXB.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.