Fix java 17 compilation #77
Conversation
keynmol
force-pushed
the
fix-java-17-compilation
branch
from
728f372 to
b81a9dc
Compare
| Option( | ||
| javacOptions.in(p) ++= { | ||
| if (Versions.isJavaAtLeast(17, home = javaHome.in(p).value)) | ||
| javacModuleOptions | ||
| else Nil | ||
| } | ||
| ), |
There was a problem hiding this comment.
On JDK 17+ pass the required parameters to JVM that launches javac.
| javaHome.in(p) := { | ||
| javaHome.in(p).value orElse { | ||
| // We can safely use java.home property | ||
| // on JDK 17+ as it won't be pointing to JRE which | ||
| // doesn't contain a compiler. | ||
| if (Versions.isJavaAtLeast(17)) { | ||
| // On JDK 17+ we need to explicitly fork the compiler | ||
| // so that we can set the necessary JVM options to access | ||
| // jdk.compiler module | ||
| Some(new File(System.getProperty("java.home"))) | ||
| } else { | ||
| // If JDK is below 17, we don't actually need to | ||
| // fork the compiler, so we can keep javaHome empty | ||
| None | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
It's important that we set javaHome explicitly - otherwise SBT will instantiate a local (in-process) Java compiler, which will refuse to add these settings: https://sourcegraph.com/github.com/sbt/zinc/-/blob/internal/zinc-compile-core/src/main/scala/sbt/internal/inc/javac/LocalJava.scala?L302
We only need to do that on JDK 17+ which is great because on on Java 8 the java.home property actually points to the JRE according to the spec (and as such doesn't contain javac)
| // On JDK 17+ we need to explicitly fork the compiler | ||
| // so that we can set the necessary JVM options to access | ||
| // jdk.compiler module | ||
| Some(new File(System.getProperty("java.home"))) |
There was a problem hiding this comment.
I'm not 100% sure, but forking javac may cause some builds to fail. Not a blocking comment, just good to keep an eye for it. If we can't turn on forking then we could try to automatically add the exports to SBT_OPTS
There was a problem hiding this comment.
Can you expand on what might be causing issues?
Perhaps I could add an extra test for this just to be sure
There was a problem hiding this comment.
I can't remember any concrete examples. Fine to try this out and just keep an eye out for regressions.
| else Paths.get("bin", "java") | ||
|
|
||
| scala.sys.process | ||
| .Process(Seq(cmd.toString(), "-version"), cwd = javaHome) |
There was a problem hiding this comment.
This is always going to be quite brittle. Ideally, we use something like SystemJavaVersion from scip-java. Not a blocking comment, just an FYI if you don't want too maintain this parser forever https://sourcegraph.com/github.com/sourcegraph/scip-java/-/blob/scip-java/src/main/scala/com/sourcegraph/scip_java/buildtools/SystemJavaVersion.scala?L16
There was a problem hiding this comment.
I just checked and the entire PrintJavaVersion class is only 504 bytes - so I think in some other PR I'll just base64 that and include as resources:
yv66vgAAAD0AIQoAAgADBwAEDAAFAAYBABBqYXZhL2xhbmcvT2JqZWN0AQAGPGluaXQ+AQADKClWCQAIAAkHAAoMAAsADAEAEGphdmEvbGFuZy9TeXN0ZW0BAANvdXQBABVMamF2YS9pby9QcmludFN0cmVhbTsIAA4BAAxqYXZhLnZlcnNpb24KAAgAEAwAEQASAQALZ2V0UHJvcGVydHkBACYoTGphdmEvbGFuZy9TdHJpbmc7KUxqYXZhL2xhbmcvU3RyaW5nOwoAFAAVBwAWDAAXABgBABNqYXZhL2lvL1ByaW50U3RyZWFtAQAFcHJpbnQBABUoTGphdmEvbGFuZy9TdHJpbmc7KVYHABoBABBQcmludEphdmFWZXJzaW9uAQAEQ29kZQEAD0xpbmVOdW1iZXJUYWJsZQEABG1haW4BABYoW0xqYXZhL2xhbmcvU3RyaW5nOylWAQAKU291cmNlRmlsZQEAFVByaW50SmF2YVZlcnNpb24uamF2YQAhABkAAgAAAAAAAgABAAUABgABABsAAAAdAAEAAQAAAAUqtwABsQAAAAEAHAAAAAYAAQAAAAEACQAdAB4AAQAbAAAAKAACAAEAAAAMsgAHEg24AA+2ABOxAAAAAQAcAAAACgACAAAAAwALAAQAAQAfAAAAAgAg
So that I can shell out to java command and just run this class. Thanks for the hint.
This is currently working on top of #76
Test plan