bugfix: Use platform Java from BSP instead of metals one

~~I am hesistant to add a test case, since that would require downloading separate JDK for the sake of one test~~ Decided to add test as the logic became a bit more complex. Fixes #5780
scalameta · Oct 24, 2023 · 104ff2a · 104ff2a
1 parent 3bc135f
commit 104ff2a
Show file tree

Hide file tree

Showing 7 changed files with 67 additions and 21 deletions.
diff --git a/metals/src/main/scala/scala/meta/internal/metals/JavaBinary.scala b/metals/src/main/scala/scala/meta/internal/metals/JavaBinary.scala
@@ -40,14 +40,25 @@ object JavaBinary {
       }
   }
 
-  def allPossibleJavaBinaries(
-      javaHome: Option[String]
-  ): List[AbsolutePath] = {
-    JdkSources
-      .defaultJavaHome(javaHome)
+  private def fromAbsolutePath(javaPath: Iterable[AbsolutePath]) = {
+    javaPath
       .flatMap(home => List(home.resolve("bin"), home.resolve("bin/jre")))
       .flatMap(bin => List("java", "java.exe").map(bin.resolve))
       .withFilter(_.exists)
       .flatMap(binary => List(binary.dealias, binary))
   }
+
+  def javaBinaryFromPath(
+      javaHome: Option[String]
+  ): Option[AbsolutePath] =
+    fromAbsolutePath(javaHome.map(_.toAbsolutePath)).headOption
+
+  def allPossibleJavaBinaries(
+      javaHome: Option[String]
+  ): Iterable[AbsolutePath] = {
+    fromAbsolutePath(
+      JdkSources
+        .defaultJavaHome(javaHome)
+    )
+  }
 }
diff --git a/metals/src/main/scala/scala/meta/internal/metals/codelenses/RunTestCodeLens.scala b/metals/src/main/scala/scala/meta/internal/metals/codelenses/RunTestCodeLens.scala
@@ -31,6 +31,7 @@ import ch.epfl.scala.bsp4j.BuildTargetIdentifier
 import ch.epfl.scala.{bsp4j => b}
 import com.google.gson.JsonElement
 import org.eclipse.{lsp4j => l}
+import scala.meta.internal.metals.JavaBinary
 
 /**
  * Class to generate the Run and Test code lenses to trigger debugging.
@@ -293,13 +294,19 @@ final class RunTestCodeLens(
       main: b.ScalaMainClass,
       buildServerCanDebug: Boolean,
   ): List[l.Command] = {
+    val javaBinary = buildTargets
+      .scalaTarget(target)
+      .flatMap(scalaTarget =>
+        JavaBinary.javaBinaryFromPath(scalaTarget.jvmHome)
+      )
+      .orElse(userConfig().usedJavaBinary)
     val (data, shellCommandAdded) = buildTargetClasses.jvmRunEnvironment
       .get(target)
-      .zip(userConfig().usedJavaBinary) match {
+      .zip(javaBinary) match {
       case None =>
         (main.toJson, false)
-      case Some((env, javaHome)) =>
-        (ExtendedScalaMainClass(main, env, javaHome, workspace).toJson, true)
+      case Some((env, javaBinary)) =>
+        (ExtendedScalaMainClass(main, env, javaBinary, workspace).toJson, true)
     }
     val params = {
       val dataKind = b.DebugSessionParamsDataKind.SCALA_MAIN_CLASS

diff --git a/metals/src/main/scala/scala/meta/internal/metals/debug/DebugProvider.scala b/metals/src/main/scala/scala/meta/internal/metals/debug/DebugProvider.scala
@@ -65,6 +65,7 @@ import com.google.common.net.InetAddresses
 import com.google.gson.JsonElement
 import org.eclipse.lsp4j.MessageParams
 import org.eclipse.lsp4j.MessageType
+import scala.meta.internal.metals.JavaBinary
 
 /**
  * @param supportsTestSelection test selection hasn't been defined in BSP spec yet.
@@ -427,9 +428,15 @@ class DebugProvider(
           if params.getDataKind == b.DebugSessionParamsDataKind.SCALA_MAIN_CLASS =>
         json.as[b.ScalaMainClass] match {
           case Success(main) if params.getTargets().size > 0 =>
+            val javaBinary = buildTargets
+              .scalaTarget(params.getTargets().get(0))
+              .flatMap(scalaTarget =>
+                JavaBinary.javaBinaryFromPath(scalaTarget.jvmHome)
+              )
+              .orElse(userConfig().usedJavaBinary)
             val updatedData = buildTargetClasses.jvmRunEnvironment
               .get(params.getTargets().get(0))
-              .zip(userConfig().usedJavaBinary) match {
+              .zip(javaBinary) match {
               case None =>
                 main.toJson
               case Some((env, javaHome)) =>

diff --git a/metals/src/main/scala/scala/meta/internal/metals/debug/ExtendedScalaMainClass.scala b/metals/src/main/scala/scala/meta/internal/metals/debug/ExtendedScalaMainClass.scala
@@ -47,7 +47,7 @@ case class ExtendedScalaMainClass private (
 object ExtendedScalaMainClass {
 
   private def createCommand(
-      javaHome: AbsolutePath,
+      javaBinary: AbsolutePath,
       classpath: List[String],
       jvmOptions: List[String],
       arguments: List[String],
@@ -61,16 +61,16 @@ object ExtendedScalaMainClass {
     )
     val argumentsString = arguments.mkString(" ")
     // We need to add "" to account for whitespace and also escaped \ before "
-    val escapedJavaHome = javaHome.toNIO.getRoot().toString +
-      javaHome.toNIO
+    val escapedJavaHome = javaBinary.toNIO.getRoot().toString +
+      javaBinary.toNIO
         .iterator()
         .asScala
         .map(p => s""""$p"""")
         .mkString(File.separator)
-    val safeJavaHome =
+    val safeJavaBinary =
       if (Properties.isWin) escapedJavaHome.replace("""\"""", """\\"""")
       else escapedJavaHome
-    s"$safeJavaHome $jvmOptsString -classpath \"$classpathString\" $mainClass $argumentsString"
+    s"$safeJavaBinary $jvmOptsString -classpath \"$classpathString\" $mainClass $argumentsString"
   }
 
   /**
@@ -126,7 +126,7 @@ object ExtendedScalaMainClass {
   def apply(
       main: ScalaMainClass,
       env: b.JvmEnvironmentItem,
-      javaHome: AbsolutePath,
+      javaBinary: AbsolutePath,
       workspace: AbsolutePath,
   ): ExtendedScalaMainClass = {
     val jvmOpts = (main.getJvmOptions().asScala ++ env
@@ -152,7 +152,7 @@ object ExtendedScalaMainClass {
       jvmOpts.asJava,
       (jvmEnvVariables ++ mainEnvVariables).asJava,
       createCommand(
-        javaHome,
+        javaBinary,
         env.getClasspath().asScala.map(_.toAbsolutePath.toString).toList,
         jvmOpts,
         main.getArguments().asScala.toList,

diff --git a/tests/unit/src/main/scala/tests/BaseCodeLensLspSuite.scala b/tests/unit/src/main/scala/tests/BaseCodeLensLspSuite.scala
@@ -16,7 +16,10 @@ abstract class BaseCodeLensLspSuite(
     initializer: BuildServerInitializer = QuickBuildInitializer,
 ) extends BaseLspSuite(name, initializer) {
 
-  protected def runFromCommand(cmd: Command): Option[String] = {
+  protected def runFromCommand(
+      cmd: Command,
+      javaHome: Option[String],
+  ): Option[String] = {
     cmd.getArguments().asScala.toList match {
       case (params: DebugSessionParams) :: _ =>
         params.getData() match {
@@ -43,6 +46,12 @@ abstract class BaseCodeLensLspSuite(
                   .replace("ProgramFiles", "Program Files")
                   .replace("runshellcommand", "run shell command")
               )
+            javaHome.foreach { home =>
+              assert(
+                cmd(0).startsWith(home),
+                s"${cmd(0)} didn't start with java home:\n $home ",
+              )
+            }
             ShellRunner
               .runSync(cmd.toList, workspace, redirectErrorOutput = false)
               .map(_.trim())
@@ -60,14 +69,17 @@ abstract class BaseCodeLensLspSuite(
     }
   }
 
-  protected def testRunShellCommand(name: String): Unit =
+  protected def testRunShellCommand(
+      name: String,
+      javaHome: Option[String] = None,
+  ): Unit =
     test(name) {
       cleanWorkspace()
       for {
         _ <- initialize(
           s"""|/metals.json
               |{
-              |  "a": {}
+              |  "a": { ${javaHome.map(jh => s"\"platformJavaHome\": \"$jh\"").getOrElse("")} }
               |}
               |/a/src/main/scala/a/Main.scala
               |package foo
@@ -83,7 +95,7 @@ abstract class BaseCodeLensLspSuite(
         lenses <- server.codeLenses("a/src/main/scala/a/Main.scala")
         _ = assert(lenses.size > 0, "No lenses were generated!")
         command = lenses.head.getCommand()
-        _ = assertEquals(runFromCommand(command), Some("Hello java!"))
+        _ = assertEquals(runFromCommand(command, javaHome), Some("Hello java!"))
       } yield ()
     }
 

diff --git a/tests/unit/src/main/scala/tests/QuickBuild.scala b/tests/unit/src/main/scala/tests/QuickBuild.scala
@@ -71,6 +71,7 @@ case class QuickBuild(
     additionalSources: Array[String],
     sbtVersion: String,
     sbtAutoImports: Array[String],
+    platformJavaHome: String,
 ) {
   def withId(id: String): QuickBuild =
     QuickBuild(
@@ -84,6 +85,7 @@ case class QuickBuild(
       orEmpty(additionalSources),
       sbtVersion,
       orEmpty(sbtAutoImports),
+      platformJavaHome,
     )
   private def orEmpty(array: Array[String]): Array[String] =
     if (array == null) new Array(0) else array
@@ -203,7 +205,9 @@ case class QuickBuild(
           artifacts,
         )
       }
-    val javaHome = Option(Properties.jdkHome).map(Paths.get(_))
+    val javaHome = Option(platformJavaHome)
+      .map(Paths.get(_))
+      .orElse(Option(Properties.jdkHome).map(Paths.get(_)))
     val tags = if (isTest) Tag.Test :: Nil else Nil
 
     val scalaCompiler =

diff --git a/tests/unit/src/test/scala/tests/CodeLensLspSuite.scala b/tests/unit/src/test/scala/tests/CodeLensLspSuite.scala
@@ -1,6 +1,7 @@
 package tests
 
 import scala.meta.internal.metals.UserConfiguration
+import coursierapi.JvmManager
 
 class CodeLensLspSuite extends BaseCodeLensLspSuite("codeLenses") {
   override protected val changeSpacesToDash = false
@@ -305,6 +306,10 @@ class CodeLensLspSuite extends BaseCodeLensLspSuite("codeLenses") {
         |""".stripMargin
   )
 
+  testRunShellCommand(
+    "run-shell-command-old-java",
+    Some(JvmManager.create().get("8").toString()),
+  )
   testRunShellCommand("run-shell-command")
   testRunShellCommand("run shell command")