Refactored BSP module and use isolated classloading for implementations #2245
Conversation
lefou
changed the title
lefou
force-pushed
the
bsp-isolate
branch
2 times, most recently
from
aee1228 to
350b47e
Compare
| def generatedBuildInfo: T[Seq[PathRef]] = T { | ||
| val workerDep = worker.publishSelfDependency() |
There was a problem hiding this comment.
Huh interesting, I never noticed this pattern before. I didn't know publishSelfDependency existed.
| def generatedBuildInfo: T[Seq[PathRef]] = T { | ||
| val workerDep = worker.publishSelfDependency() |
There was a problem hiding this comment.
Huh interesting, I never noticed this pattern before. I didn't know publishSelfDependency existed.
There was a problem hiding this comment.
This is only done this way because we needed in in our BuildInfo right?
There was a problem hiding this comment.
Yeah. As we want to load (and resolve) it at runtime and don't want to bundle it in the assembly, we need to know the correct dependency coordinates.
| ZincWorkerUtil.scalaBinaryVersion(scalaVersion()), | ||
| ScalaPlatform.JVM, | ||
| scalaCompilerClasspath().map(_.path.toNIO.toUri.toString).iterator.toSeq.asJava | ||
| "scala", |
There was a problem hiding this comment.
| "scala", | |
| ScalaBuildTarget.dataKind, |
| "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 |
There was a problem hiding this comment.
This would be nice, detect the Mill version somehow and warn the user if there is a mismatch.
There was a problem hiding this comment.
Ahhh, I see we actually do this down below, so is this a leftover comment?
There was a problem hiding this comment.
No, it's still valid. We detect that we are outdated, but at this point we can't do anything about it, except logging or failing. Failing is too rude here, as we would force the user to understand and handle it. Logging is only helpful to understand the issue when we get access to the logs.
At this stage (firing up BSP server without having a completely initialized Evaluator) we can't trigger a re-build of the BSP connection file, as we can't use the evaluator and the task graph.
There was a problem hiding this comment.
One way to handle a detected outdated worker could be to:
- remember that fact in the BSP service
- Wait for an Evaluator
- Trigger a rebuild of the connection file
- Send an event to the client, that we need to re-start or (if possible) are going to restart the server
"Worker" is a somewhat overloaded term. I don't use a
That would be nice. We still have no proper automated test setup that can guarantee a workable server. I typically just delete all |
Sort of an aside, but we talked about this a bit at the Center. It's on our radar and something we'd like to explore and potentially work on to better ensure the BSP ecosystem as a whole is more tested and less error-prone. I'll loop you in when I have more information there. |
|
Just a head up, using this locally for a bit, everything seems to work expected when using Metals. |
I also excluded bsp4j from transitive Ammonite dependencies, as we don't use Ammonites BSP support and it brought older versions into the tree.
I removed any use of bsp4j API in scalalib, which was only there previously by accident, as I overlooked it's presence through Ammonites transitive dependencies.
scalalibnow does not depend on bsp4j. I added more case classes (equivalents to the bsp4j classes) if the BSP support required it.The
bspmodule is now only a thin module, which provides the entry point to BSP and also manages the classloading of the implementation, which is now contained inbsp.workermodule.I also minimized the use of reflection in
MillMainto start the BSP server, which should reduce the potential for errors which can only be catched in (manual) tests.