Fix BSP regressions and improved resource file handling (#2420)

* Restore proper streams handling; currently `mill --bsp` is dumping all output on `STDERR` which is very noisy * Better handling of the resource file * Fixed buildinfo parser for changed embedded dependencies format Pull request: #2420
com-lihaoyi · Apr 13, 2023 · 70c6e44 · 70c6e44
1 parent 75f0bb9
commit 70c6e44
Show file tree

Hide file tree

Showing 13 changed files with 158 additions and 70 deletions.
diff --git a/bsp/src/mill/bsp/BSP.scala b/bsp/src/mill/bsp/BSP.scala
@@ -19,10 +19,6 @@ object BSP extends ExternalModule with CoursierModule with BspServerStarter {
 
   private[this] val millServerHandle = Promise[BspServerHandle]()
 
-  private def bspWorkerIvyDeps: T[Agg[Dep]] = T {
-    Agg(Dep.parse(BuildInfo.millBspWorkerDep))
-  }
-
   private def bspWorkerLibs: T[Agg[PathRef]] = T {
     mill.modules.Util.millProjectModule(
       "MILL_BSP_WORKER",
@@ -45,15 +41,17 @@ object BSP extends ExternalModule with CoursierModule with BspServerStarter {
    * reason, the message and stacktrace of the exception will be
    * printed to stdout.
    */
-  def install(jobs: Int = 1): Command[Unit] = T.command {
+  def install(jobs: Int = 1): Command[(PathRef, ujson.Value)] = T.command {
     // we create a file containing the additional jars to load
-    val cpFile = T.workspace / Constants.bspDir / s"${Constants.serverName}.resources"
+    val libUrls = bspWorkerLibs().map(_.path.toNIO.toUri.toURL).iterator.toSeq
+    val cpFile =
+      T.workspace / Constants.bspDir / s"${Constants.serverName}-${mill.BuildInfo.millVersion}.resources"
     os.write.over(
       cpFile,
-      bspWorkerLibs().iterator.map(_.path.toNIO.toUri.toURL).mkString("\n"),
+      libUrls.mkString("\n"),
       createFolders = true
     )
-    BspWorker(T.ctx()).map(_.createBspConnection(jobs, Constants.serverName))
+    BspWorker(T.ctx(), Some(libUrls)).map(_.createBspConnection(jobs, Constants.serverName))
   }
 
   /**
@@ -73,23 +71,22 @@ object BSP extends ExternalModule with CoursierModule with BspServerStarter {
   override def startBspServer(
       initialEvaluator: Option[Evaluator],
       streams: SystemStreams,
+      logStream: Option[PrintStream],
       workspaceDir: os.Path,
       ammoniteHomeDir: os.Path,
       canReload: Boolean,
       serverHandle: Option[Promise[BspServerHandle]] = None
   ): BspServerResult = {
-
     val ctx = new Ctx.Workspace with Ctx.Home with Ctx.Log {
       override def workspace: Path = workspaceDir
       override def home: Path = ammoniteHomeDir
       // This all goes to the BSP log file mill-bsp.stderr
       override def log: Logger = new Logger {
         override def colored: Boolean = false
-
         override def systemStreams: SystemStreams = new SystemStreams(
-          streams.err,
-          streams.err,
-          DummyInputStream
+          out = streams.out,
+          err = streams.err,
+          in = DummyInputStream
         )
         override def info(s: String): Unit = streams.err.println(s)
         override def error(s: String): Unit = streams.err.println(s)
@@ -106,11 +103,20 @@ object BSP extends ExternalModule with CoursierModule with BspServerStarter {
         worker.startBspServer(
           initialEvaluator,
           streams,
+          logStream.getOrElse(streams.err),
           workspaceDir / Constants.bspDir,
           canReload,
           Seq(millServerHandle) ++ serverHandle.toSeq
         )
-      case _ => BspServerResult.Failure
+      case f: Result.Failure[_] =>
+        streams.err.println("Failed to start the BSP worker. " + f.msg)
+        BspServerResult.Failure
+      case f: Result.Exception =>
+        streams.err.println("Failed to start the BSP worker. " + f.throwable)
+        BspServerResult.Failure
+      case f =>
+        streams.err.println("Failed to start the BSP worker. " + f)
+        BspServerResult.Failure
     }
   }
 

diff --git a/bsp/src/mill/bsp/BspWorker.scala b/bsp/src/mill/bsp/BspWorker.scala
@@ -7,7 +7,7 @@ import mill.eval.Evaluator
 import mill.main.{BspServerHandle, BspServerResult}
 import mill.api.SystemStreams
 
-import java.io.{InputStream, PrintStream}
+import java.io.PrintStream
 import java.net.URL
 import scala.concurrent.Promise
 import scala.util.{Failure, Success, Try}
@@ -18,11 +18,12 @@ trait BspWorker {
   def createBspConnection(
       jobs: Int,
       serverName: String
-  )(implicit ctx: Ctx): Unit
+  )(implicit ctx: Ctx): (PathRef, ujson.Value)
 
   def startBspServer(
       initialEvaluator: Option[Evaluator],
       streams: SystemStreams,
+      logStream: PrintStream,
       logDir: os.Path,
       canReload: Boolean,
       serverHandles: Seq[Promise[BspServerHandle]]
@@ -35,23 +36,36 @@ object BspWorker {
 
   private[this] var worker: Option[BspWorker] = None
 
-  def apply(millCtx: Ctx.Workspace with Ctx.Home with Ctx.Log): Result[BspWorker] = {
+  def apply(
+      millCtx: Ctx.Workspace with Ctx.Home with Ctx.Log,
+      workerLibs: Option[Seq[URL]] = None
+  ): Result[BspWorker] = {
     worker match {
       case Some(x) => Result.Success(x)
       case None =>
-        // load extra classpath entries from file
-        val cpFile = millCtx.workspace / Constants.bspDir / s"${Constants.serverName}.resources"
-        if (!os.exists(cpFile)) return Result.Failure(
-          "You need to run `mill mill.bsp.BSP/install` before you can use the BSP server"
-        )
+        val urls = workerLibs.map { urls =>
+          millCtx.log.debug("Using direct submitted worker libs")
+          urls
+        }.getOrElse {
+          // load extra classpath entries from file
+          val cpFile =
+            millCtx.workspace / Constants.bspDir / s"${Constants.serverName}-${mill.BuildInfo.millVersion}.resources"
+          if (!os.exists(cpFile)) return Result.Failure(
+            "You need to run `mill mill.bsp.BSP/install` before you can use the BSP server"
+          )
 
-        // TODO: if outdated, we could regenerate the resource file and re-load the worker
+          // TODO: if outdated, we could regenerate the resource file and re-load the worker
 
-        val urls = os.read(cpFile).linesIterator.map(u => new URL(u)).toSeq
+          // read the classpath from resource file
+          millCtx.log.debug(s"Reading worker classpath from file: ${cpFile}")
+          os.read(cpFile).linesIterator.map(u => new URL(u)).toSeq
+        }
 
         // create classloader with bsp.worker and deps
         val cl = mill.api.ClassLoader.create(urls, getClass().getClassLoader())(millCtx)
-        val workerVersion = Try {
+
+        // check the worker version
+        Try {
           val workerBuildInfo = cl.loadClass(Constants.bspWorkerBuildInfoClass)
           workerBuildInfo.getMethod("millBspWorkerVersion").invoke(null)
         } match {

diff --git a/bsp/worker/src/mill/bsp/worker/BspWorkerImpl.scala b/bsp/worker/src/mill/bsp/worker/BspWorkerImpl.scala
@@ -49,7 +49,7 @@ class BspWorkerImpl() extends BspWorker {
   override def createBspConnection(
       jobs: Int,
       serverName: String
-  )(implicit ctx: Ctx): Unit = {
+  )(implicit ctx: Ctx): (PathRef, ujson.Value) = {
     // we create a json connection file
     val bspFile = ctx.workspace / Constants.bspDir / s"${serverName}.json"
     if (os.exists(bspFile)) ctx.log.info(s"Overwriting BSP connection file: ${bspFile}")
@@ -58,12 +58,15 @@ class BspWorkerImpl() extends BspWorker {
     if (withDebug) ctx.log.debug(
       "Enabled debug logging for the BSP server. If you want to disable it, you need to re-run this install command without the --debug option."
     )
-    os.write.over(bspFile, bspConnectionJson(jobs, withDebug), createFolders = true)
+    val connectionContent = bspConnectionJson(jobs, withDebug)
+    os.write.over(bspFile, connectionContent, createFolders = true)
+    (PathRef(bspFile), upickle.default.read[ujson.Value](connectionContent))
   }
 
   override def startBspServer(
       initialEvaluator: Option[Evaluator],
       streams: SystemStreams,
+      logStream: PrintStream,
       logDir: os.Path,
       canReload: Boolean,
       serverHandles: Seq[Promise[BspServerHandle]]
@@ -76,7 +79,7 @@ class BspWorkerImpl() extends BspWorker {
         bspVersion = Constants.bspProtocolVersion,
         serverVersion = MillBuildInfo.millVersion,
         serverName = Constants.serverName,
-        logStream = streams.err,
+        logStream = logStream,
         canReload = canReload
       ) with MillJvmBuildServer with MillJavaBuildServer with MillScalaBuildServer
 

diff --git a/build.sc b/build.sc
@@ -1097,8 +1097,7 @@ object bsp extends MillModule with BuildInfo{
   def buildInfoMembers = T{
     val workerDep = worker.publishSelfDependency()
     Seq(
-      BuildInfo.Value("bsp4jVersion", Deps.bsp4j.dep.version, "BSP4j version (BSP Protocol version)."),
-      BuildInfo.Value("millBspWorkerDep", s"${workerDep.group}:${workerDep.id}:${workerDep.version}", "BSP worker dependency.")
+      BuildInfo.Value("bsp4jVersion", Deps.bsp4j.dep.version, "BSP4j version (BSP Protocol version).")
     )
   }
 

diff --git a/contrib/buildinfo/src/mill/contrib/buildinfo/BuildInfo.scala b/contrib/buildinfo/src/mill/contrib/buildinfo/BuildInfo.scala
@@ -165,12 +165,16 @@ object BuildInfo {
          |package ${buildInfoPackageName}
          |
          |object $buildInfoObjectName {
-         |  private val buildInfoProperties = new java.util.Properties
+         |  private[this] val buildInfoProperties: java.util.Properties = new java.util.Properties()
          |
-         |  private val buildInfoInputStream = getClass
-         |    .getResourceAsStream("$buildInfoObjectName.buildinfo.properties")
+         |  private[this] val buildInfoInputStream = getClass
+         |      .getResourceAsStream("${buildInfoObjectName}.buildinfo.properties")
          |
-         |  buildInfoProperties.load(buildInfoInputStream)
+         |  try {
+         |    buildInfoProperties.load(buildInfoInputStream)
+         |  } finally {
+         |    buildInfoInputStream.close()
+         |  }
          |
          |  $bindingsCode
          |}
@@ -180,21 +184,21 @@ object BuildInfo {
          |package ${buildInfoPackageName};
          |
          |public class $buildInfoObjectName {
-         |  private static java.util.Properties buildInfoProperties = new java.util.Properties();
+         |  private static final java.util.Properties buildInfoProperties = new java.util.Properties();
          |
          |  static {
-         |    java.io.InputStream buildInfoInputStream = $buildInfoObjectName
+         |    java.io.InputStream buildInfoInputStream = ${buildInfoObjectName}
          |      .class
-         |      .getResourceAsStream("$buildInfoObjectName.buildinfo.properties");
+         |      .getResourceAsStream("${buildInfoObjectName}.buildinfo.properties");
          |
-         |    try{
+         |    try {
          |      buildInfoProperties.load(buildInfoInputStream);
-         |    }catch(java.io.IOException e){
+         |    } catch (java.io.IOException e) {
          |      throw new RuntimeException(e);
-         |    }finally{
-         |      try{
+         |    } finally {
+         |      try {
          |        buildInfoInputStream.close();
-         |      }catch(java.io.IOException e){
+         |      } catch (java.io.IOException e) {
          |        throw new RuntimeException(e);
          |      }
          |    }

diff --git a/main/api/src/mill/api/SystemStreams.scala b/main/api/src/mill/api/SystemStreams.scala
@@ -2,4 +2,8 @@ package mill.api
 
 import java.io.{InputStream, PrintStream}
 
-class SystemStreams(val out: PrintStream, val err: PrintStream, val in: InputStream)
+class SystemStreams(
+    val out: PrintStream,
+    val err: PrintStream,
+    val in: InputStream
+)
diff --git a/main/src/mill/main/BspServerStarter.scala b/main/src/mill/main/BspServerStarter.scala
@@ -3,13 +3,14 @@ package mill.main
 import mill.eval.Evaluator
 import mill.api.SystemStreams
 
-import java.io.{InputStream, PrintStream}
+import java.io.PrintStream
 import scala.concurrent.Promise
 
 trait BspServerStarter {
   def startBspServer(
       initialEvaluator: Option[Evaluator],
       streams: SystemStreams,
+      logStream: Option[PrintStream],
       workspaceDir: os.Path,
       ammoniteHomeDir: os.Path,
       canReload: Boolean,

diff --git a/main/src/mill/modules/Util.scala b/main/src/mill/modules/Util.scala
@@ -1,8 +1,8 @@
 package mill.modules
 
 import coursier.Repository
-import mill.BuildInfo
-import mill.api.{Ctx, IO, Loose, PathRef}
+import mill.{Agg, BuildInfo}
+import mill.api.{Ctx, IO, Loose, PathRef, Result}
 
 object Util {
 
@@ -70,7 +70,7 @@ object Util {
       resolveFilter: os.Path => Boolean = _ => true,
       // this should correspond to the mill runtime Scala version
       artifactSuffix: String = "_2.13"
-  ) = {
+  ): Result[Agg[PathRef]] = {
     millProperty(key) match {
       case Some(localPath) =>
         mill.api.Result.Success(

diff --git a/runner/src/mill/runner/BspContext.scala b/runner/src/mill/runner/BspContext.scala
@@ -4,14 +4,15 @@ import mill.api.internal
 import mill.main.{BspServerHandle, BspServerResult, BspServerStarter}
 import mill.api.SystemStreams
 
-import java.util.concurrent.Executors
+import java.io.PrintStream
+import java.util.concurrent.{Executors, TimeUnit}
 import scala.concurrent.duration.Duration
 import scala.concurrent.{Await, ExecutionContext, Future, Promise}
 import scala.util.chaining.scalaUtilChainingOps
 import scala.util.control.NonFatal
 
 @internal
-class BspContext(streams: SystemStreams, home: os.Path) {
+class BspContext(streams: SystemStreams, bspLogStream: Option[PrintStream], home: os.Path) {
   // BSP mode, run with a simple evaluator command to inject the evaluator
   // The command returns when the server exists or the workspace should be reloaded
   // if the `lastResult` is `ReloadWorkspace` we re-run the script in a loop
@@ -30,6 +31,7 @@ class BspContext(streams: SystemStreams, home: os.Path) {
       BspServerStarter().startBspServer(
         initialEvaluator = None,
         streams = streams,
+        logStream = bspLogStream,
         workspaceDir = os.pwd,
         ammoniteHomeDir = home,
         canReload = true,
@@ -43,7 +45,7 @@ class BspContext(streams: SystemStreams, home: os.Path) {
     }
   }(serverThreadContext)
 
-  val handle = Await.result(bspServerHandle.future, Duration.Inf).tap { _ =>
+  val handle = Await.result(bspServerHandle.future, Duration(10, TimeUnit.SECONDS)).tap { _ =>
     streams.err.println("BSP server started")
   }