-
Notifications
You must be signed in to change notification settings - Fork 3
Fix java 17 compilation #77
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
Changes from all commits
b81a9dc
b3709ba
41b2f32
4617850
ce2cdd7
8cd2e7b
ee0b3e3
86ec7c4
4417d62
729040c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,12 +31,20 @@ object SourcegraphEnable { | |
| ) | ||
|
|
||
| val semanticdbJavacVersion = Versions.semanticdbJavacVersion() | ||
|
|
||
| val settings = for { | ||
| (p, semanticdbVersion, overriddenScalaVersion) <- collectProjects( | ||
| extracted | ||
| ) | ||
| enableSemanticdbPlugin = | ||
| List( | ||
| Option( | ||
| javacOptions.in(p) ++= { | ||
| if (Versions.isJavaAtLeast(17, home = javaHome.in(p).value)) | ||
| javacModuleOptions | ||
| else Nil | ||
| } | ||
| ), | ||
| Option( | ||
| allDependencies.in(p) += | ||
| "com.sourcegraph" % "semanticdb-javac" % semanticdbJavacVersion | ||
|
|
@@ -51,6 +59,11 @@ object SourcegraphEnable { | |
| Option(SemanticdbPlugin.semanticdbEnabled.in(p) := true), | ||
| semanticdbVersion.map(ver => | ||
| SemanticdbPlugin.semanticdbVersion.in(p) := ver | ||
| ), | ||
| Option( | ||
| javaHome.in(p) := { | ||
| javaHome.in(p).value orElse calculateJavaHome | ||
| } | ||
| ) | ||
| ).flatten | ||
| settings <- | ||
|
|
@@ -101,6 +114,14 @@ object SourcegraphEnable { | |
| ) | ||
| ) | ||
| }.toSeq | ||
| ), | ||
| Option( | ||
| javacOptions.in(p) ++= { | ||
| if (Versions.isJavaAtLeast(17)) javacModuleOptions else Nil | ||
| } | ||
| ), | ||
| Option( | ||
| javaHome.in(p) := javaHome.in(p).value orElse calculateJavaHome | ||
| ) | ||
| ).flatten | ||
| settings <- | ||
|
|
@@ -143,4 +164,34 @@ object SourcegraphEnable { | |
| semanticdbVersion = Versions | ||
| .semanticdbVersion(overriddenScalaVersion.getOrElse(projectScalaVersion)) | ||
| } yield (p, semanticdbVersion, overriddenScalaVersion) | ||
|
|
||
| def javacModuleOptions: List[String] = | ||
| List( | ||
| "-J--add-exports", | ||
| "-Jjdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED", | ||
| "-J--add-exports", | ||
| "-Jjdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", | ||
| "-J--add-exports", | ||
| "-Jjdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED", | ||
| "-J--add-exports", | ||
| "-Jjdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED", | ||
| "-J--add-exports", | ||
| "-Jjdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED" | ||
| ) | ||
|
|
||
| private def calculateJavaHome = { | ||
| // 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"))) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you expand on what might be causing issues? Perhaps I could add an extra test for this just to be sure
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't remember any concrete examples. Fine to try this out and just keep an eye out for regressions. |
||
| } else { | ||
| // If JDK is below 17, we don't actually need to | ||
| // fork the compiler, so we can keep javaHome empty | ||
| None | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ import java.nio.file.Paths | |
| import java.util.Properties | ||
| import scala.collection.JavaConverters._ | ||
| import scala.sys.process._ | ||
| import java.io.File | ||
|
|
||
| object Versions { | ||
| def scalametaVersion = "4.4.26" | ||
|
|
@@ -65,6 +66,67 @@ object Versions { | |
| .updated(semanticdbJavacKey, semanticdbJavacVersions.last) | ||
| } | ||
|
|
||
| private val jvmVersionCache = collection.mutable.Map.empty[Option[File], Int] | ||
|
|
||
| def isJavaAtLeast(n: Int, home: Option[File] = None): Boolean = { | ||
|
|
||
| val significant = jvmVersionCache.getOrElseUpdate( | ||
| home, { | ||
| val raw = | ||
| home match { | ||
| case None => | ||
| System.getProperty("java.version") | ||
| case Some(javaHome) => | ||
| val sb = new StringBuilder | ||
| val proc = { | ||
| val cmd = | ||
| if (scala.util.Properties.isWin) | ||
| Paths.get("bin", "java") | ||
| else Paths.get("bin", "java") | ||
|
|
||
| scala.sys.process | ||
| .Process(Seq(cmd.toString(), "-version"), cwd = javaHome) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is always going to be quite brittle. Ideally, we use something like
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: So that I can shell out to
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That sounds good. |
||
| .!!(ProcessLogger(sb.append(_))) | ||
|
|
||
| sb.result().trim | ||
| } | ||
|
|
||
| val rgx = "version \"(.*?)\"".r | ||
|
|
||
| rgx.findFirstMatchIn( | ||
| proc.linesIterator.take(1).mkString("") | ||
| ) match { | ||
| case None => | ||
| sys.error( | ||
| s"Cannot process [java -version] output (in $javaHome): [$proc]" | ||
| ) | ||
| case Some(value) => | ||
| value.group(1) | ||
| } | ||
| } | ||
|
|
||
| val prop = raw.takeWhile(c => c.isDigit || c == '.') | ||
|
|
||
| val segments = prop.split("\\.").toList | ||
|
|
||
| segments match { | ||
| // Java 1.6 - 1.8 | ||
| case "1" :: lessThan8 :: _ :: Nil => lessThan8.toInt | ||
| // Java 17.0.1, .. | ||
| case modern :: _ :: _ :: Nil => modern.toInt | ||
| // Java 12 | ||
| case modern :: Nil => modern.toInt | ||
| case other => | ||
| sys.error( | ||
| s"Cannot process java.home property, unknown format: [$raw]" | ||
| ) | ||
| } | ||
| } | ||
| ) | ||
|
|
||
| significant >= n | ||
| } | ||
|
|
||
| private def proc(cmd: String*): List[String] = { | ||
| println(cmd.updated(0, "coursier").mkString("$ ", " ", "")) | ||
| cmd.!!.linesIterator.toList | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| sbt.version=1.5.2 | ||
| sbt.version=1.9.3 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,2 @@ | ||
| > sourcegraphEnable | ||
| > show semanticdbEnabled | ||
| > show Compile/semanticdbEnabled | ||
| > sourcegraphCompile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On JDK 17+ pass the required parameters to JVM that launches javac.