From 70c6e4470e267d9733fff5440d94ee41099ea4d9 Mon Sep 17 00:00:00 2001 From: Tobias Roeser Date: Thu, 13 Apr 2023 18:51:35 +0200 Subject: [PATCH] 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: https://github.com/com-lihaoyi/mill/pull/2420 --- bsp/src/mill/bsp/BSP.scala | 34 +++++++------ bsp/src/mill/bsp/BspWorker.scala | 36 +++++++++----- .../src/mill/bsp/worker/BspWorkerImpl.scala | 9 ++-- build.sc | 3 +- .../mill/contrib/buildinfo/BuildInfo.scala | 28 ++++++----- main/api/src/mill/api/SystemStreams.scala | 6 ++- main/src/mill/main/BspServerStarter.scala | 3 +- main/src/mill/modules/Util.scala | 6 +-- runner/src/mill/runner/BspContext.scala | 8 ++-- runner/src/mill/runner/MillMain.scala | 48 +++++++++++++------ runner/src/mill/runner/MillServerMain.scala | 9 ++-- runner/src/mill/runner/TeePrintStream.scala | 36 ++++++++++++++ .../mill/scalalib/bsp/MillBuildModule.scala | 2 +- 13 files changed, 158 insertions(+), 70 deletions(-) create mode 100644 runner/src/mill/runner/TeePrintStream.scala diff --git a/bsp/src/mill/bsp/BSP.scala b/bsp/src/mill/bsp/BSP.scala index 04499c84260..4a02f92abd1 100644 --- 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 index 1020cd65a6e..181eb1967e9 100644 --- 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 index 7a6d6575f9a..42c73cef304 100644 --- 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 index 9475182536b..b53117771c8 100644 --- 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 index 2ea58e764c6..3d10cb24894 100644 --- 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 index a92366f1fd8..1df7a7ffe94 100644 --- 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 index a783d27eb60..81449709cc5 100644 --- 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 index 48223925c77..eff9bcf76c6 100644 --- 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 index cf8fbbaf79b..e77eaedbd4d 100644 --- 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") } diff --git a/runner/src/mill/runner/MillMain.scala b/runner/src/mill/runner/MillMain.scala index fd1b2b6b73d..bd1a3ce167f 100644 --- a/runner/src/mill/runner/MillMain.scala +++ b/runner/src/mill/runner/MillMain.scala @@ -16,33 +16,49 @@ object MillMain { def main(args: Array[String]): Unit = { val initialSystemStreams = new SystemStreams(System.out, System.err, System.in) // setup streams - val openStreams = + val (runnerStreams, cleanupStreams, bspLog) = if (args.headOption == Option("--bsp")) { + // In BSP mode, we use System.in/out for protocol communication + // and all Mill output (stdout and stderr) goes to a dedicated file val stderrFile = os.pwd / ".bsp" / "mill-bsp.stderr" os.makeDir.all(stderrFile / os.up) - val err = new PrintStream(new FileOutputStream(stderrFile.toIO, true)) - System.setErr(err) - System.setOut(err) - err.println(s"Mill in BSP mode, version ${BuildInfo.millVersion}, ${new java.util.Date()}") - Seq(err) - } else Seq() + val errFile = new PrintStream(new FileOutputStream(stderrFile.toIO, true)) + val errTee = new TeePrintStream(initialSystemStreams.err, errFile) + val msg = s"Mill in BSP mode, version ${BuildInfo.millVersion}, ${new java.util.Date()}" + errTee.println(msg) + ( + new SystemStreams( + // out is used for the protocol + out = initialSystemStreams.out, + // err is default, but also tee-ed into the bsp log file + err = errTee, + in = System.in + ), + Seq(errFile), + Some(errFile) + ) + } else { + // Unchanged system stream + (initialSystemStreams, Seq(), None) + } if (Properties.isWin && System.console() != null) io.github.alexarchambault.windowsansi.WindowsAnsi.setup() val (result, _) = try main0( - args, - RunnerState.empty, - mill.util.Util.isInteractive(), - initialSystemStreams, - System.getenv().asScala.toMap, - b => (), + args = args, + stateCache = RunnerState.empty, + mainInteractive = mill.util.Util.isInteractive(), + streams0 = runnerStreams, + bspLog = bspLog, + env = System.getenv().asScala.toMap, + setIdle = b => (), userSpecifiedProperties0 = Map(), initialSystemProperties = sys.props.toMap ) finally { - openStreams.foreach(_.close()) + cleanupStreams.foreach(_.close()) } System.exit(if (result) 0 else 1) } @@ -52,6 +68,7 @@ object MillMain { stateCache: RunnerState, mainInteractive: Boolean, streams0: SystemStreams, + bspLog: Option[PrintStream], env: Map[String, String], setIdle: Boolean => Unit, userSpecifiedProperties0: Map[String, String], @@ -170,7 +187,8 @@ object MillMain { } } - val bspContext = if (bspMode) Some(new BspContext(streams, config.home)) else None + val bspContext = + if (bspMode) Some(new BspContext(streams, bspLog, config.home)) else None val targetsAndParams = bspContext.map(_.millArgs).getOrElse(config.leftoverArgs.value.toList) diff --git a/runner/src/mill/runner/MillServerMain.scala b/runner/src/mill/runner/MillServerMain.scala index 2b455cc018c..383848d341c 100644 --- a/runner/src/mill/runner/MillServerMain.scala +++ b/runner/src/mill/runner/MillServerMain.scala @@ -66,11 +66,12 @@ object MillServerMain extends MillServerMain[RunnerState] { initialSystemProperties: Map[String, String] ): (Boolean, RunnerState) = { MillMain.main0( - args, - stateCache, + args = args, + stateCache = stateCache, mainInteractive = mainInteractive, - streams, - env, + streams0 = streams, + bspLog = None, + env = env, setIdle = setIdle, userSpecifiedProperties0 = userSpecifiedProperties, initialSystemProperties = initialSystemProperties diff --git a/runner/src/mill/runner/TeePrintStream.scala b/runner/src/mill/runner/TeePrintStream.scala new file mode 100644 index 00000000000..d54ec70c056 --- /dev/null +++ b/runner/src/mill/runner/TeePrintStream.scala @@ -0,0 +1,36 @@ +package mill.runner + +import java.io.PrintStream + +class TeePrintStream(out1: PrintStream, out2: PrintStream) extends PrintStream(out1) { + override def write(b: Int): Unit = synchronized { + out1.write(b) + out2.write(b) + } + + override def write(b: Array[Byte]): Unit = synchronized { + out1.write(b) + out2.write(b) + } + + override def write(b: Array[Byte], off: Int, len: Int): Unit = synchronized { + out1.write(b, off, len) + out2.write(b, off, len) + } + + override def flush(): Unit = { + out1.flush() + out2.flush() + } + + override def close(): Unit = { + try { + out1.close() + } finally { + out2.close() + } + } + + override def checkError(): Boolean = out1.checkError() || out2.checkError() + +} diff --git a/scalalib/src/mill/scalalib/bsp/MillBuildModule.scala b/scalalib/src/mill/scalalib/bsp/MillBuildModule.scala index 2e5be145a2e..fa94f3720fe 100644 --- a/scalalib/src/mill/scalalib/bsp/MillBuildModule.scala +++ b/scalalib/src/mill/scalalib/bsp/MillBuildModule.scala @@ -38,7 +38,7 @@ trait MillBuildModule } override def compileIvyDeps: T[Agg[Dep]] = T { - val deps = Agg.from(BuildInfo.millEmbeddedDeps.map(d => ivy"${d}".forceVersion())) + val deps = Agg.from(BuildInfo.millEmbeddedDeps.split(",").map(d => ivy"${d}".forceVersion())) T.log.errorStream.println(s"compileIvyDeps: ${deps}") deps }