improvement: don't override bloop java home if version greater or equ…

…al to metals java version
scalameta · Apr 22, 2024 · acdfcb2 · acdfcb2
1 parent 407ae62
commit acdfcb2
Show file tree

Hide file tree

Showing 3 changed files with 104 additions and 18 deletions.
diff --git a/metals/src/main/scala/scala/meta/internal/metals/BloopServers.scala b/metals/src/main/scala/scala/meta/internal/metals/BloopServers.scala
@@ -143,9 +143,10 @@ final class BloopServers(
   }
 
   private def writeJVMPropertiesToBloopGlobalJsonFile(
-      maybeBloopJvmProperties: List[String]
+      maybeBloopJvmProperties: List[String],
+      newJavaHome: Option[String],
   ): Try[Unit] = Try {
-    if (metalsJavaHome.isDefined || maybeBloopJvmProperties.nonEmpty) {
+    if (newJavaHome.isDefined || maybeBloopJvmProperties.nonEmpty) {
       val javaOptionsField =
         if (maybeBloopJvmProperties.nonEmpty)
           Some(
@@ -156,7 +157,7 @@ final class BloopServers(
         else None
       val fields: List[(String, ujson.Value)] =
         List(
-          metalsJavaHome.map(v => "javaHome" -> ujson.Str(v.trim())),
+          newJavaHome.map(v => "javaHome" -> ujson.Str(v.trim())),
           javaOptionsField,
         ).flatten
       val obj = ujson.Obj.from(fields)
@@ -179,8 +180,13 @@ final class BloopServers(
       .flatMap {
         case messageActionItem
             if messageActionItem == Messages.BloopJvmPropertiesChange.reconnect =>
+          val javaHome = getJavaHomeForBloopJson.flatMap {
+            case AlreadyWritten(javaHome) => Some(javaHome)
+            case _ => metalsJavaHome
+          }
           writeJVMPropertiesToBloopGlobalJsonFile(
-            maybeBloopJvmProperties
+            maybeBloopJvmProperties,
+            javaHome,
           ) match {
             case Failure(exception) => Future.failed(exception)
             case Success(_) =>
@@ -276,31 +282,53 @@ final class BloopServers(
       userConfiguration: UserConfiguration
   ) = {
     // we should set up Java before running Bloop in order to not restart it
-    bloopJsonPath match {
-      case Some(bloopPath) if !bloopPath.exists =>
+    getJavaHomeForBloopJson match {
+      case Some(WriteMetalsJavaHome) =>
         metalsJavaHome.foreach { newHome =>
-          scribe.info(s"Setting up current java home $newHome in $bloopPath")
+          bloopJsonPath.foreach { bloopPath =>
+            scribe.info(s"Setting up current java home $newHome in $bloopPath")
+          }
         }
         // we want to use the same java version as Metals, so it's ok to use java.home
         writeJVMPropertiesToBloopGlobalJsonFile(
-          userConfiguration.bloopJvmProperties.getOrElse(Nil)
+          userConfiguration.bloopJvmProperties.getOrElse(Nil),
+          metalsJavaHome,
+        )
+      case Some(OverrideWithMetalsJavaHome(javaHome, oldJavaHome, opts)) =>
+        scribe.info(
+          s"Replacing bloop java home $oldJavaHome with java home at $javaHome."
         )
+        writeJVMPropertiesToBloopGlobalJsonFile(opts, Some(javaHome))
+      case _ =>
+    }
+  }
+
+  private def getJavaHomeForBloopJson: Option[BloopJavaHome] =
+    bloopJsonPath match {
+      case Some(bloopPath) if !bloopPath.exists =>
+        Some(WriteMetalsJavaHome)
       case Some(bloopPath) if bloopPath.exists =>
         maybeLoadBloopGlobalJsonFile(bloopPath) match {
           case (Some(javaHome), opts) =>
-            metalsJavaHome.foreach { newHome =>
-              if (newHome != javaHome) {
-                scribe.info(
-                  s"Replacing bloop java home $javaHome with java home at $newHome."
-                )
-                writeJVMPropertiesToBloopGlobalJsonFile(opts)
-              }
+            metalsJavaHome match {
+              case Some(metalsJava) =>
+                (
+                  JdkVersion.maybeJdkVersionFromJavaHome(javaHome),
+                  JdkVersion.maybeJdkVersionFromJavaHome(metalsJava),
+                ) match {
+                  case (Some(writtenVersion), Some(metalsJavaVersion))
+                      if writtenVersion.major >= metalsJavaVersion.major =>
+                    Some(AlreadyWritten(javaHome))
+                  case _ if javaHome != metalsJava =>
+                    Some(OverrideWithMetalsJavaHome(metalsJava, javaHome, opts))
+                  case _ => Some(AlreadyWritten(javaHome))
+                }
+              case None => Some(AlreadyWritten(javaHome))
             }
-          case _ =>
+          case _ => None
         }
-      case _ =>
+      case _ => None
     }
-  }
 
   private def connectToLauncher(
       bloopVersion: String,
@@ -393,4 +421,13 @@ object BloopServers {
       )
     }
   }
+
+  sealed trait BloopJavaHome
+  case class AlreadyWritten(javaHome: String) extends BloopJavaHome
+  case class OverrideWithMetalsJavaHome(
+      javaHome: String,
+      oldJavaHome: String,
+      opts: List[String],
+  ) extends BloopJavaHome
+  object WriteMetalsJavaHome extends BloopJavaHome
 }
diff --git a/metals/src/main/scala/scala/meta/internal/metals/JavaInteractiveSemanticdb.scala b/metals/src/main/scala/scala/meta/internal/metals/JavaInteractiveSemanticdb.scala
@@ -198,6 +198,11 @@ case class JdkVersion(
 
 object JdkVersion {
 
+  def maybeJdkVersionFromJavaHome(
+      maybeJavaHome: String
+  )(implicit ec: ExecutionContext): Option[JdkVersion] =
+    maybeJdkVersionFromJavaHome(Try(AbsolutePath(maybeJavaHome)).toOption)
+
   def maybeJdkVersionFromJavaHome(
       maybeJavaHome: Option[AbsolutePath]
   )(implicit ec: ExecutionContext): Option[JdkVersion] = {

diff --git a/tests/unit/src/test/scala/tests/BloopJavaHomeLspSuite.scala b/tests/unit/src/test/scala/tests/BloopJavaHomeLspSuite.scala
@@ -4,6 +4,8 @@ import scala.meta.internal.metals.BloopServers
 import scala.meta.internal.metals.MetalsEnrichments._
 import scala.meta.io.AbsolutePath
 
+import coursierapi.JvmManager
+
 class BloopJavaHomeLspSuite extends BaseLspSuite("java-home") {
 
   val jsonFilePath: Option[AbsolutePath] =
@@ -12,6 +14,7 @@ class BloopJavaHomeLspSuite extends BaseLspSuite("java-home") {
   val contents: Option[String] = jsonFile.map(_.readText)
 
   val javaHome: String = sys.props.get("java.home").getOrElse("")
+  val pathToJava21: String = JvmManager.create().get("21").toString()
 
   def stashBloopJson(): Unit = {
     jsonFile.foreach(_.delete())
@@ -79,6 +82,47 @@ class BloopJavaHomeLspSuite extends BaseLspSuite("java-home") {
     } yield ()
   }
 
+  test("correct-home") {
+    cleanWorkspace()
+    val bloopJsonContent =
+      ujson.Obj
+        .apply("javaHome" -> pathToJava21)
+        .toString()
+
+    jsonFilePath.foreach(_.writeText(bloopJsonContent))
+    for {
+      _ <- initialize(
+        """/metals.json
+          |{
+          |  "a": {}
+          |}
+          |/a/src/main/scala/a/A.scala
+          |package a
+          |object A { val x : String = 1 }
+          |""".stripMargin
+      )
+      _ <- server.didOpen("a/src/main/scala/a/A.scala")
+      _ = assertNoDiff(
+        client.workspaceDiagnostics,
+        """|a/src/main/scala/a/A.scala:2:29: error: type mismatch;
+           | found   : Int(1)
+           | required: String
+           |object A { val x : String = 1 }
+           |                            ^
+           |""".stripMargin,
+      )
+      bloopJsonText = jsonFilePath.map(_.readText).getOrElse("")
+      _ = assert(
+        server.client.messageRequests.isEmpty(),
+        "No message should show up",
+      )
+      _ = assertNoDiff(
+        bloopJsonText,
+        bloopJsonContent,
+      )
+    } yield ()
+  }
+
   test("no-home") {
     cleanWorkspace()
     for {