Enable smoother sbt bsp integration. #2154
Conversation
metals/src/main/scala/scala/meta/internal/bsp/BspConnector.scala
Outdated
Show resolved
Hide resolved
|
Thanks for the great work @ckipp01 ! I will give it a go and see if there is anything I could suggest. Overall, it looks like a very good approach. |
metals/src/main/scala/scala/meta/internal/bsp/BspConnector.scala
Outdated
Show resolved
Hide resolved
|
So from the original approach up above, I changed a few things that started to worry me the more I looked at them. Mainly, I didn't want to special-case sbt so much, especially seeing how the bsp support in mill was merged, I kept trying to think of a nice way to after we smooth out the sbt bsp support to look ahead to the mill bsp support. Ultimately this lead to the decision to get rid of the original Another decision related to the above one was that instead of the [Path 1] - User clones down a project, has never ran sbt on it before, opens it, and would like to use the sbt server.The user will ignore the first import prompt, and instead trigger the [Path 2] - User re-opens the workspace after they have previously chosen to use the sbt bsp server supportSame as above, the decision was saved when the user did the [Path 3] - User has already imported their build with Bloop, but now want to switch to sbt bspUser has three choice.
[Path 4] - User wants to switch back from sbt BSP to Bloop.Same as the first examples, the user can again use the This seems to all be working pretty smoothly at this point, so dare I say the core part of this is now done. I'll focus in on cleaning a few things up ensuring things are properly tested and documented. |
df08fde to
b68c1e4
Compare
|
So I think this is in a pretty good place now to have some more eyes on it and to be taken for a test drive. I've been playing around with it for a while, using it this last week some at work, and I'm overall pretty happy with it. I know this is a pretty large change, so from a functionality point of view, I've made an asciicast to show me starting with a fresh workspace, switching bsp servers to sbt, switching back to bloop again, reloading the workspace, playing with the There are also prs open in the VS Code and coc-metals repos to add in the extra command to help explore this as well. |
metals/src/main/scala/scala/meta/internal/builds/BloopInstallProvider.scala
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/builds/BloopInstallResult.scala
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/builds/SbtBuildTool.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/ChosenBuildServers.scala
Show resolved
Hide resolved
|
I was curious about this and I tried publishing a local version of this branch but it didn't seem to work. In a new project, there doesn't seem to be any option for As a side note, in addition to the |
Hey! Thanks for trying to take it for a spin @eatkins! As mentioned up above, there are actually prs open in scalameta/coc-metals#320 and scalameta/metals-vscode#426 to help people explore the support since not all the command have been added to the extensions. Since you're using coc-metals, I actually just went ahead and merged in the
Probably @olafurpg, @gabro, or @tgodzik will know a lot more since the decision was made so long ago, but it's a good point to bring up. I'll have to think about this a bit, and maybe this is a good discussion can start in another thread unless you think it's integral to consider those changes in this pr. I'm a bit hesitant at the moment since I imagine a lot of our users are still on sbt 1.3.x, so switching to default to |
The launcher is used if we don't know that the user has sbt installed and I think |
Whoops. I guess I glazed over that part.
The launcher does support |
|
@eatkins Ach, you are talking about using sbt script instead of the launcher. I missed that, we could for sure try to change it (at least for linux/macos), but I would still leave it for another issue. |
metals/src/main/scala/scala/meta/internal/builds/WorkspaceReloadStatus.scala
Outdated
Show resolved
Hide resolved
Previously we weren't really capturing the capabilities. Since now we need to check to see if the server supports reload requests, we carry it forward and check against it later. The `workspace/reload` refactor basically just allows now for all connections to be reload, not just the main one. Plus it captures the error that the CompletableFuture may have to display to the user that they tried to reload a workspace that isn't working.
Before this we have a SbtServer class to handle the initial and running and connecting of the sbt BSP process. Instead now, we have a BuildServerProvider that is meant to be mixed in to a BuildToold and provide the necessary functionality to generate a BSP config file, and then connect to it.
|
I noticed that if you first connected to a BSP server it is no longer possible to generate Bloop config. |
So part of this is expected. If you ignore the first prompt to import, the However, I do notice that if |
|
This is what I explained above.
|
Oh, it makes sense. I was experimenting with a workspace switching the server back and forth - that's probably not something most people do, so it's a pretty niche use case.
Sounds good! I did run |
Sounds good. I'll special case this then and then a user can no matter what always do a |
|
Alright, so I switched the logic up a bit. Now if a user has never imported their build and there is no
|
|
Here we go! |
📢 EDIT: A lot has changed since this was first opened, but I'm leaving this here to show my thought process about how I started this vs what it's like now. So if you want to know the full history of how this started, feel free to read this, if not scroll on down to the other large section with gifs and I'll explain the approach I landed on.
This pr is the first steps in addressing #2049 and #2152 by enabling the sbt bsp support to be much more integrated into Metals if a user would like choose it instead of using Bloop.
This is still heavily a WIP, I still have a bunch of TODOs as you'll see in the code, I need a bunch of testing, and even some docs updated. However, it's getting a bit large, so I'd like to throw this up here as it is working, and I'd love any feedback about the approach or things I may have missed before I get even farther into this.
Following along with the detailed conversations in the two threads listed above and also the POC that @kpbochenek started on (👍 thanks for that), I split the intended support into 4 paths.
[Path 1] - User clones down a project, has never ran sbt on it before, opens it, and would like to use the sbt server.
Current behavior
The current behavior for this is that they would have to ignore the prompt to import the build, start up the sbt shell with
-Dsbt.semanticdb=true, and then do abuild-connectcommand to connect to it.New behavior
Again, following the discussion, until all the kinks get smoothed out, we have proper testing, and the UX is on par with that of the Bloop integration, we'll keep Bloop as the default choice. That part doesn't change, so upon opening the project, the user will still get a prompt to import the build (with Bloop). If they'd like to instead use sbt server, they'd just ignore the prompt, and execute a new command (that may be named different in the client)
build-server-startwith a single parameter "sbt". The idea in the future would be for this new command to be the way we can trigger to start a server manually for whatever reason. In the case of sbt, this is sort of special-cased as there is aSbtServer.scalaclass that will handle this by ensuring the sbt version is > 1.4.0, starts up a process of sbt, and then establishes a bsp connection.Here is a gif showing that. The time is accurate to give you an idea of how long it will take. There will "progress" added so the user knows what's going on.
If you were to look at the logs in this situation, you'd see something like this, which will give you a good idea of what's going on behind the scenes:
[Path 2] - User re-opens the workspace after they have previously chosen to use the sbt bsp server support
Current behavior
This would currently be no different than above, the user would have to go through all the steps of connecting again.
New behavior
The users choice was saved up above, so that will be detected, sbt will be triggered to start, and then a bsp connection will be made. Some more work probably needs to happen here to ensure we don't have to many shells open that don't need to be.
[Path 3] - User has already imported their build with Bloop, but now want to switch to sbt bsp
Current behavior
Again, same as the others, they would need to basically remove their
.metals/and.bloop/directories, and then repeat the process from the beginning to connect.New behavior
Now the user is able to simply again trigger the new
start-build-servercommand. As an example in the gifs you're seeing it bound to astart-sbt-servercommand, which sends in the necessary command to Metals. I'll open that pr and one in VS Code shortly after this if someone wants to take it for a test drive.[Path 4] - User wants to switch back from sbt BSP to Bloop.
New behavior
I'm just going to skip the old behavior because that flow just doesn't exist right now. But there is now a couple ways to do this. Firstly, you could trigger an
reset-choiceto forget the build server choice, close your workspace, re-open, and then you'd be prompted again to import your build with Bloop.Switching back and forth is also supported, as you can see here where I use
bsp-switchto jump from the sbt server back to bloop (sbt is shutdown here):There are also some other notable things:
build-disconnect,build-restart, etc now have changed behavior to detect if the user is using Bloop or another bsp server and issue the commands accordingly.workspace/reloadis now introduced so when a build change is detected while using sbt bsp, it's fired. This also h 8000 appens for example if the user triggers a manualbuild-import, but they are using sbt server rather than Bloop.Note that we do need to figure out some stuff with this and notifying the user if they reload a bad definition ([BSP] - unexpected(This has been fixed)workspace/reloadresponse if given an invalid build definition sbt/sbt#5988)sbt-bloopplugin?I also want to add while I'm a avid Bloop user and fan, I've been very impressed with the BSP support in sbt. I remember first trying it very early on and was worried because of some issues I hit on, but I didn't notice anything like that while playing around with this. This coming week I'll probably be using this exclusively at work to really test drive it. Looking forward to feedback.