improvement: Unify connecting to bazelbsp with other build servers #6164
Conversation
jkciesluk
force-pushed
the
unify-bazel
branch
3 times, most recently
from
3313918
to
473223a
Compare
jkciesluk
force-pushed
the
unify-bazel
branch
from
473223a
to
e968088
Compare
| @@ -400,6 +401,11 @@ object MetalsEnrichments | |||
| workspace.resolve(Directories.readonly).toNIO | |||
| ) | |||
|
|
|||
| def isInTmpDirectory(workspace: AbsolutePath): Boolean = | |||
There was a problem hiding this comment.
In Bazel I was getting errors like
ERROR: no such target '//:.metals/.tmp/Main-5012603866797659764.scala': target '.metals/.tmp/Main-5012603866797659764.scala' not declared in package '' defined by /.../BUILD;
however, a source file of this name exists. (Perhaps add 'exports_files([".metals/.tmp/Main-5012603866797659764.scala"])' to /BUILD?)
metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala
Outdated
Show resolved
Hide resolved
| @@ -2058,7 +2051,10 @@ class MetalsLspService( | |||
| buildTool, | |||
| chosenBuildServer, | |||
| ) | |||
| } yield buildChange | |||
| } yield { | |||
| buildServerPromise.trySuccess(()) | |||
There was a problem hiding this comment.
Shouldn't that happen only on success?
There was a problem hiding this comment.
In quickConnectToBuildServer it happens even if build server is not auto connectable, so I've used similar logic here.
metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala
Outdated
Show resolved
Hide resolved
| @@ -63,6 +63,7 @@ class ScalaCliBuildTool( | |||
|
|
|||
| override def possibleBuildServerNames = ScalaCli.names.toList | |||
|
|
|||
| override def isAutoConnectable: Boolean = true | |||
There was a problem hiding this comment.
Scala CLI is only auto-connectable if bsp config exists, and we discover Scala CLI also if project.scala exists.
On the other hand BspOnly is definitely auto-connectable.
To me it feels like buildTool.isBspGenerated(folder) and buildTool.isAutoConnectable should be the same thing. But my problem might be just with the name.
Let's at least add comments to such things so we know what it means.
There was a problem hiding this comment.
You're right, I've removed this method and used isBspGenerated instead
metals/src/main/scala/scala/meta/internal/metals/MetalsEnrichments.scala
Outdated
Show resolved
Hide resolved
| scribe.warn( | ||
| "No build session currently active to reload. Attempting to reconnect." | ||
| ) | ||
| reconnectToBuildServer() |
There was a problem hiding this comment.
If this is now a part of a happy path, I don't think we should emit a warning here.
There was a problem hiding this comment.
It's part of the happy path only on initialize. I think its best to leave the warning here, its still unhappy path is something breaks.
| ) | ||
| } yield () | ||
| _ <- slowConnectToBuildServer(forceImport = false), | ||
| } yield buildServerPromise.trySuccess(()) |
There was a problem hiding this comment.
buildServerPromise.trySuccess(()) already happens in slowConnectToBuildServer
jkciesluk
force-pushed
the
unify-bazel
branch
3 times, most recently
from
eb8166a
to
05c6d04
Compare
No description provided.