improvement: add custom bsp as possible build tool #5791

kasiaMarek · 2023-10-26T12:22:41Z

Previously: For custom BSP we would still use some other build tool if discovered.
Now: Custom BSP is in itself treated as a build tool, we look for .bsp configs in:

root directory,
custom project root directory,
bspGlobalInstallDirectories.

As a followup we should deduplicate code in build servers and build tools discovery and selection:

BspServers.findAvailableServers repeats a lot from BuildTools.loadSupported,
build tool selection adjust build server selection when needed and the other way around.

metals/src/main/scala/scala/meta/internal/bsp/BspConnector.scala

tgodzik · 2023-11-09T15:14:51Z

metals/src/main/scala/scala/meta/internal/builds/BspOnly.scala

+    override val executableName: String,
+    override val projectRoot: AbsolutePath,
+) extends BuildTool {
+  override def digest(workspace: AbsolutePath): Option[String] = Some("OK")


This should be the digest of the json, no?

I thought so as well. But then again on change we do workspaceReload and that would require a reconnect to have the changes apply.

We can also do a custom logic if the current server's json changes to reconnect. Would that make more sense?

I think so, I added this logic.

tgodzik · 2023-11-09T15:27:24Z

metals/src/main/scala/scala/meta/internal/builds/BuildTools.scala

+    val bspFolder = workspace.resolve(".bsp")
+    val root = customProjectRoot.getOrElse(workspace)


Suggested change

val bspFolder = workspace.resolve(".bsp")

val root = customProjectRoot.getOrElse(workspace)

val root = customProjectRoot.getOrElse(workspace)

val bspFolder = root.resolve(".bsp")

?

Because we always create .bsp in the root I assumed we search only there. The root only says where the bsp command will be run. We can change this behaviour though.

I think ideally, we would look for .bsp in the custom root also.

metals/src/main/scala/scala/meta/internal/builds/BuildTools.scala

tgodzik · 2023-11-09T15:30:10Z

metals/src/main/scala/scala/meta/internal/metals/PopupChoiceReset.scala

@@ -28,7 +28,6 @@ class PopupChoiceReset(
    val result = if (value == BuildTool) {
      scribe.info("Resetting build tool selection.")
      tables.buildTool.reset()
-      tables.buildServers.reset()


Why not reset the choice here?

Because we only change the build tool? So either we trust the user to be reasonable and not chose mill build tool for sbt build server. I they do it is still mostly fine. If we reset buildServer choice we'd have to do full reconnect (quickConnect & slowConnect) to be connected to correct build server for metals state consistency.

Let's say that we have to servers in .bsp without resetting we will have to manually do the switch to the other tool, no?

No, since those "forceBuildServer", so it will switch automatically in slowConnect.

tgodzik · 2023-11-09T15:51:32Z

metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala

@@ -918,8 +944,7 @@ class MetalsLspService(
          .sequence(
            List[Future[Unit]](
              Future(buildTools.initialize()),
-              quickConnectToBuildServer().ignoreValue,
-              slowConnectToBuildServer(forceImport = false).ignoreValue,


I wonder how we should rename these methods. firstConnectToBuildServer and connectToBuildServer ?

We can, though I'm not sure those names explain it much better.

We can do it later on if anyone has a good idea about the names

metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala

kasiaMarek · 2023-11-22T11:01:25Z

I wonder if this line:

metals/metals/src/main/scala/scala/meta/internal/bsp/BspServers.scala

Line 60 in a6e1ac4

name <- tables.buildServers.selectedServer(md5)

even makes sense anymore. I don't see that we ever chooseServer with anything else than md5 = "EXPLICIT". I don't know what was the original reasoning behind this. I'm guessing it had something to do with discovering new .bsp configs.

jkciesluk

I love these changes!

About didChangeWatchedFiles (in line 1357), do we still need this check for creating file in .bsp directory, if it's already handled in the other didChangeWatchedFiles method?
Also, I think we can remove starting fileWatcher in supportedBuildTool, since we now have .bsp directory in Config

jkciesluk · 2023-11-24T09:55:54Z

tests/unit/src/main/scala/tests/TestingClient.scala

@@ -306,17 +305,6 @@ class TestingClient(workspace: AbsolutePath, val buffers: Buffers)
    // NOTE: (ckipp01) Just for easiness of testing, we are going to just look
    // for sbt and mill builds together, which are most common. The logic however
    // is identical for all build tools.
-    def isSameMessageFromList(


Leftover comment above

… tool

jkciesluk

LGTM!

tgodzik

Some minor comments, but otherwise looks good!

metals/src/main/scala/scala/meta/internal/metals/Indexer.scala

tgodzik · 2023-12-13T16:38:01Z

metals/src/main/scala/scala/meta/internal/bsp/BspConnector.scala

+        case buildTool: BuildServerProvider
+            if buildTool.buildServerName.contains(buildServerName) =>
+          true
+        case buildTool => buildTool.executableName == buildServerName


Could you add a comment when this would be different?

tgodzik · 2023-12-13T16:42:18Z

metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala

@@ -918,8 +944,7 @@ class MetalsLspService(
          .sequence(
            List[Future[Unit]](
              Future(buildTools.initialize()),
-              quickConnectToBuildServer().ignoreValue,
-              slowConnectToBuildServer(forceImport = false).ignoreValue,


We can do it later on if anyone has a good idea about the names

tgodzik

Great work!

Since scalameta#5791 Pants can be used as custom BSP. This should be more reliable, since most of the support was alredy removed.

Since #5791 Pants can be used as custom BSP. This should be more reliable, since most of the support was alredy removed.

kasiaMarek force-pushed the custom-bsp branch 3 times, most recently from 64eaad4 to f64429e Compare October 31, 2023 15:55

kasiaMarek force-pushed the custom-bsp branch 3 times, most recently from eac1a09 to 7e18b77 Compare November 9, 2023 10:22

kasiaMarek marked this pull request as ready for review November 9, 2023 11:29

kasiaMarek requested review from jkciesluk and tgodzik November 9, 2023 11:29

tgodzik reviewed Nov 9, 2023

View reviewed changes

kasiaMarek force-pushed the custom-bsp branch 2 times, most recently from e5de41b to 8e60ab1 Compare November 21, 2023 11:41

kasiaMarek requested a review from tgodzik November 21, 2023 11:42

kasiaMarek added 3 commits November 22, 2023 11:11

improvement: add custom bsp as possible build tool

34a05ba

reconnect on .bsp change

ca72002

look for .bsp also in the project root

a6e1ac4

kasiaMarek force-pushed the custom-bsp branch from 8e60ab1 to a6e1ac4 Compare November 22, 2023 10:44

jkciesluk reviewed Nov 24, 2023

View reviewed changes

delete additional bsp config discovery, now discovered as a new build…

253a171

… tool

kasiaMarek requested a review from jkciesluk December 5, 2023 09:33

jkciesluk approved these changes Dec 11, 2023

View reviewed changes

tgodzik requested changes Dec 13, 2023

View reviewed changes

tgodzik approved these changes Dec 15, 2023

View reviewed changes

add doc for optSetBuildTool

67269f0

kasiaMarek merged commit 73706fa into scalameta:main Dec 18, 2023
24 of 25 checks passed

jkciesluk added a commit to jkciesluk/metals that referenced this pull request Dec 21, 2023

chore: Remove Pants specific checks

4b78234

Since scalameta#5791 Pants can be used as custom BSP. This should be more reliable, since most of the support was alredy removed.

jkciesluk mentioned this pull request Dec 21, 2023

chore: Remove Pants specific checks #5968

Merged

jkciesluk added a commit that referenced this pull request Dec 21, 2023

chore: Remove Pants specific checks

1d0b9ba

Since #5791 Pants can be used as custom BSP. This should be more reliable, since most of the support was alredy removed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improvement: add custom bsp as possible build tool #5791

improvement: add custom bsp as possible build tool #5791

kasiaMarek commented Oct 26, 2023 •

edited

Loading

tgodzik Nov 9, 2023

kasiaMarek Nov 9, 2023

tgodzik Nov 9, 2023

kasiaMarek Nov 20, 2023

tgodzik Nov 9, 2023

kasiaMarek Nov 9, 2023

tgodzik Nov 9, 2023

tgodzik Nov 9, 2023

kasiaMarek Nov 9, 2023

tgodzik Nov 9, 2023

kasiaMarek Nov 20, 2023

tgodzik Nov 9, 2023

kasiaMarek Nov 20, 2023

tgodzik Dec 13, 2023

kasiaMarek commented Nov 22, 2023

jkciesluk left a comment

jkciesluk Nov 24, 2023

jkciesluk left a comment

tgodzik left a comment

tgodzik Dec 13, 2023

tgodzik Dec 13, 2023

tgodzik left a comment

		val bspFolder = workspace.resolve(".bsp")
		val root = customProjectRoot.getOrElse(workspace)

improvement: add custom bsp as possible build tool #5791

improvement: add custom bsp as possible build tool #5791

Conversation

kasiaMarek commented Oct 26, 2023 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasiaMarek commented Nov 22, 2023

jkciesluk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkciesluk left a comment

Choose a reason for hiding this comment

tgodzik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik left a comment

Choose a reason for hiding this comment

kasiaMarek commented Oct 26, 2023 •

edited

Loading