Add buildTargets/jvmEnvironment endpoint #1214

lukaszwawrzyk · 2020-03-25T16:34:30Z

Depends on build-server-protocol/build-server-protocol#113
Just implements the new endpoint, to currently be exactly the same as jvmTestEnvironment.
Fills in new optional parameters javaHome and javaVersion, currently based on the jvm that bloop is running on. In future we may allow to customize it through Config.Project

olafurpg · 2020-03-27T14:31:24Z

frontend/src/main/scala/bloop/bsp/BloopBspServices.scala

@@ -863,6 +864,13 @@ final class BloopBspServices(
  private def toBuildTargetId(project: Project): bsp.BuildTargetIdentifier =
    bsp.BuildTargetIdentifier(project.bspUri)

+  private def toJvmBuildTarget(project: Project): bsp.JvmBuildTarget = {
+    val javaHome = sys.props.get("java.home").map(s => bsp.Uri(Paths.get(s).toUri))


nit: sys.props allocates a new immutable map containing all system properties on every method call so I personally use Option(System.getProperty(..)) instead

olafurpg

LGTM 👍 We can merge this once the BSP PR is released

olafurpg · 2020-03-30T14:21:25Z

frontend/src/main/scala/bloop/bsp/BloopBspServices.scala

@@ -863,6 +864,13 @@ final class BloopBspServices(
  private def toBuildTargetId(project: Project): bsp.BuildTargetIdentifier =
    bsp.BuildTargetIdentifier(project.bspUri)

+  private def toJvmBuildTarget(project: Project): bsp.JvmBuildTarget = {
+    val javaHome = Option(System.getProperty("java.home")).map(s => bsp.Uri(Paths.get(s).toUri))


I just learned from an offline discussion with @jvican that Bloop supports configuring custom Java home per-project. See .platform here

bloop/frontend/src/main/scala/bloop/data/Project.scala

Line 52 in 906110d

platform: Platform,

We should use JdkConfig.javaHome instead of the java.home system property.

olafurpg · 2020-03-30T14:31:02Z

build.sbt

@@ -27,6 +27,7 @@ val benchmarkBridge = project
 lazy val bloopShared = (project in file("shared"))
  .settings(
    name := "bloop-shared",
+    resolvers += Resolver.sonatypeRepo("snapshots"),


We should cut a new stable BSP release so we don't use SNAPSHOTs

jvican

Hey @lukaszwawrzyk thanks for your work, overall LGTM 👍 I've just left some comments and suggestions.

jvican · 2020-03-31T09:45:23Z

frontend/src/main/scala/bloop/bsp/BloopBspServices.scala

@@ -863,6 +864,20 @@ final class BloopBspServices(
  private def toBuildTargetId(project: Project): bsp.BuildTargetIdentifier =
    bsp.BuildTargetIdentifier(project.bspUri)

+  private def toJvmBuildTarget(project: Project): bsp.JvmBuildTarget = {
+    Option(project.platform)


We have some of this logic already in a method called project.jdkConfig. What do you think about using that instead of this? We can also replace the values of java.home and the java version and use those that are available in JavaRuntime.

yeah, I guess it is enough to use project.jdkConfig, anyway it sets the correct jvm default config if not provided, and if it returns none, JvmBuildTarget can be empty as it means native or js.

jvican · 2020-03-31T09:45:37Z

frontend/src/main/scala/bloop/bsp/BloopBspServices.scala

+        val javaHome = Option(System.getProperty("java.home")).map(s => bsp.Uri(Paths.get(s).toUri))
+        val javaVersion = Option(System.getProperty("java.vm.specification.version"))
+        bsp.JvmBuildTarget(javaHome, javaVersion)
+      }


Isn't this method returning a jvm build target always, regardless of the platform?

Why return a JvmBuildTarget here for projects that are not JVM?

jvican · 2020-03-31T09:45:52Z

frontend/src/main/scala/bloop/bsp/BloopBspServices.scala

+      }
+      .getOrElse {
+        val javaHome = Option(System.getProperty("java.home")).map(s => bsp.Uri(Paths.get(s).toUri))
+        val javaVersion = Option(System.getProperty("java.vm.specification.version"))


Are you really use you want java.vm.specification.version here instead of java.version?

jvican

What happened to the use of java.specification.vm.version?

jvican · 2020-03-31T10:33:33Z

frontend/src/main/scala/bloop/bsp/BloopBspServices.scala

+        val javaHome = Option(System.getProperty("java.home")).map(s => bsp.Uri(Paths.get(s).toUri))
+        val javaVersion = Option(System.getProperty("java.vm.specification.version"))
+        bsp.JvmBuildTarget(javaHome, javaVersion)
+      }


Why return a JvmBuildTarget here for projects that are not JVM?

jvican · 2020-03-31T10:34:17Z

frontend/src/main/scala/bloop/bsp/BloopBspServices.scala

@@ -863,6 +864,17 @@ final class BloopBspServices(
  private def toBuildTargetId(project: Project): bsp.BuildTargetIdentifier =
    bsp.BuildTargetIdentifier(project.bspUri)

+  private def toJvmBuildTarget(project: Project): bsp.JvmBuildTarget = {
+    project.jdkConfig


Nit: could we make all of this as concise as possible by using pattern matching here ?

lukaszwawrzyk · 2020-03-31T10:41:49Z

What happened to the use of java.specification.vm.version?

So I addressed the comment about not filling in JvmBuildTarget in case when target is not jvm.
If project.jdkConfig is None, it means it is Native or Js, so in such case I put JvmBuildTarget(None, None) rather than bloop runtime jvm. I actually never fill in javaVersion, as it is not available in project.platform, and I can't assume that sys.props("java.version") is valid for given project.jdkConfig.javaHome

jvican · 2020-03-31T11:42:21Z

@lukaszwawrzyk Great, thanks for the changes!

so in such case I put JvmBuildTarget(None, None) rather than bloop runtime jvm.

Why do we return JvmBuildTarget(None, None) instead of None? This looks like a limitation in the BSP design of JvmBuildTarget, we should not assume every ScalaBuildTarget has a JvmBuildTarget, it should accept Option[JvmBuildTarget] instead.

@olafurpg Any thoughts?

olafurpg · 2020-03-31T16:38:27Z

Good catch! The ScalaBuildTarget.jvmBuildTarget field needs to be typed as Option[BuildTarget], otherwise new clients won't be able to parse ScalaBuildTarget from old servers.

olafurpg · 2020-03-31T16:39:44Z

We will need to do the same change for bsp4j, this field is optional.

olafurpg

CI is green now with the updated bsp4s dependency 👍

olafurpg · 2020-04-02T08:54:50Z

frontend/src/main/scala/bloop/bsp/BloopBspServices.scala

@@ -2,6 +2,7 @@ package bloop.bsp

 import java.io.InputStream
 import java.net.URI
+import java.nio.file.Paths


Unused import

jvican · 2020-04-02T11:41:38Z

Spoke with @olafurpg offline and we agreed on merging this so that we don't need to wait for another review cycle to merge this 😄 We'll remove the unused import whenever we set up scalafix automatically in the bloop build.

lukaszwawrzyk mentioned this pull request Mar 25, 2020

Read jdk from bsp server JetBrains/intellij-scala#514

Merged

lukaszwawrzyk force-pushed the bsp-jdk branch from 29b731e to 9ccd859 Compare March 27, 2020 13:07

olafurpg reviewed Mar 27, 2020

View reviewed changes

jvican added build server Any issue or pull request that has to do with hot compilers or BSP. enhancement upgrade labels Mar 28, 2020

lukaszwawrzyk force-pushed the bsp-jdk branch from 9ccd859 to 79e5d82 Compare March 30, 2020 08:02

olafurpg approved these changes Mar 30, 2020

View reviewed changes

Implement JvmBuildTarget

075843e

lukaszwawrzyk force-pushed the bsp-jdk branch from 79e5d82 to 075843e Compare March 30, 2020 12:45

olafurpg approved these changes Mar 30, 2020

View reviewed changes

olafurpg suggested changes Mar 30, 2020

View reviewed changes

olafurpg reviewed Mar 30, 2020

View reviewed changes

lukaszwawrzyk force-pushed the bsp-jdk branch from f7c5a7d to 31c3680 Compare March 31, 2020 08:09

olafurpg approved these changes Mar 31, 2020

View reviewed changes

jvican suggested changes Mar 31, 2020

View reviewed changes

lukaszwawrzyk force-pushed the bsp-jdk branch from 31c3680 to 348ccf2 Compare March 31, 2020 10:12

jvican reviewed Mar 31, 2020

View reviewed changes

Read javaHome from project.platform

894fec3

lukaszwawrzyk force-pushed the bsp-jdk branch from 348ccf2 to 894fec3 Compare March 31, 2020 10:42

lukaszwawrzyk force-pushed the bsp-jdk branch 2 times, most recently from ab49ffe to 3c0f7f1 Compare April 1, 2020 10:31

JvmBuildTarget is optional

2ffcab9

lukaszwawrzyk force-pushed the bsp-jdk branch from 3c0f7f1 to 2ffcab9 Compare April 1, 2020 12:12

olafurpg approved these changes Apr 1, 2020

View reviewed changes

olafurpg requested a review from jvican April 1, 2020 14:09

olafurpg reviewed Apr 2, 2020

View reviewed changes

jvican approved these changes Apr 2, 2020

View reviewed changes

jvican merged commit 1a13b52 into scalacenter:master Apr 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add buildTargets/jvmEnvironment endpoint #1214

Add buildTargets/jvmEnvironment endpoint #1214

lukaszwawrzyk commented Mar 25, 2020

olafurpg Mar 27, 2020 •

edited

Loading

olafurpg left a comment

olafurpg Mar 30, 2020

olafurpg Mar 30, 2020

jvican left a comment

jvican Mar 31, 2020

lukaszwawrzyk Mar 31, 2020

jvican Mar 31, 2020

jvican Mar 31, 2020

jvican Mar 31, 2020

jvican left a comment

jvican Mar 31, 2020

jvican Mar 31, 2020

lukaszwawrzyk Mar 31, 2020

lukaszwawrzyk commented Mar 31, 2020

jvican commented Mar 31, 2020

olafurpg commented Mar 31, 2020

olafurpg commented Mar 31, 2020

olafurpg left a comment

olafurpg Apr 2, 2020

jvican commented Apr 2, 2020

Add buildTargets/jvmEnvironment endpoint #1214

Add buildTargets/jvmEnvironment endpoint #1214

Conversation

lukaszwawrzyk commented Mar 25, 2020

olafurpg Mar 27, 2020 • edited Loading

Choose a reason for hiding this comment

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukaszwawrzyk commented Mar 31, 2020

jvican commented Mar 31, 2020

olafurpg commented Mar 31, 2020

olafurpg commented Mar 31, 2020

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican commented Apr 2, 2020

olafurpg Mar 27, 2020 •

edited

Loading