Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for debugSession/start in sbt-metals plugin #168

Closed
adpi2 opened this issue Dec 7, 2020 · 18 comments
Closed

Add support for debugSession/start in sbt-metals plugin #168

adpi2 opened this issue Dec 7, 2020 · 18 comments

Comments

@adpi2
Copy link
Member

adpi2 commented Dec 7, 2020

Is your feature request related to a problem? Please describe.

It is currently not possible to run the tests or start an application in Metals with sbt server, because debugSession/start is not implemented.

Metals expects the debugSession/start request to start a DAP server.

Describe the solution you'd like

Implement debugSession/start and the DAP protocol in the sbt-metals plugin.

Describe alternatives you've considered

It could be implemented in sbt but we don't want to add the dependency to vs-code-java-debug in sbt.

Additional context

More information about implementation in Bloop: https://scalacenter.github.io/bloop/docs/debugging-reference
Microsoft implementation of DAP for java: https://github.com/microsoft/vscode-java-debug
Implementation in Bloop that extends vscode-java-debug: https://github.com/scalacenter/bloop/tree/master/frontend/src/test/scala/bloop/dap
Implementation of BSP in sbt: https://github.com/sbt/sbt/blob/develop/main/src/main/scala/sbt/internal/server/BuildServerProtocol.scala

Search terms:

sbt, server, DAP, debug, run, test

@tgodzik
Copy link
Contributor

tgodzik commented Dec 7, 2020

Thanks for reporting! We should have put it here a while ago 😓

@ckipp01
Copy link
Member

ckipp01 commented Dec 7, 2020

Thanks for the request @adpi2!

My initial opinion would be that this really shouldn't belong in Metals. Having the build server provide DAP capabilities seems to be the more logical place for this to live. From my understanding the Build Server in the future will even declare whether or not it's a DAP provider in the buildCapabilities. The way that this currently works for Bloop (from my understanding) works well in that we can simply be a client, and forward everything to Bloop. If possible I wouldn't want to depart from that and build in our own DAP interface/implementation in sbt-metals specifically for sbt, but rather re-use the more generic solution that we have. By doing this, we'd also defeat the purpose somewhat of DAP by implementing it client side, so other Build Clients won't be able to use it when using sbt BSP. I truly believe this belongs in sbt or in another plugin that sbt maintains, not in Metals.

@adpi2
Copy link
Member Author

adpi2 commented Dec 7, 2020

The way that this currently works for Bloop is great, and I wouldn't want to depart from that and build in our own DAP interface/implementation in sbt-metals. By doing this, we'd also defeat the purpose somewhat of DAP by implementing it client side, so other Build Clients won't be able to use it when using sbt BSP. I truly believe this belongs in sbt or in another plugin that sbt maintains, not in Metals.

It sounds right @ckipp01. Also the dependency to vscode-java-debug in sbt is a real problem because it transitively depends on a bunch of stuff (see here) and we try hard not to add dependencies in sbt. So maybe the good way to go is to implement in a separate plugin that can be depended upon by sbt-metals.

From my understanding the Build Server in the future will even declare whether or not it's a DAP provider in the buildCapabilities.

Nice! I think it is possible to override sbt buildCapabilities in plugins. I hope it can be done neatly though.

@tgodzik
Copy link
Contributor

tgodzik commented Dec 7, 2020

We've been talking about it with @adpi2 and we decided to put it here as a feature request, since we are the most invested party here. Overall, I was thinking we would:

  • create a separate library that we can depend from Bloop also
  • add a new plugin the the scalameta organisation
  • include that plugin whenever sbt-metals is brought in

I am not yet sure how to do it in a plugin infrastructure and much more work might be needed here also on the sbt side.

@ckipp01 do you think it makes sense? I really appreciate your feedback here, I have left out a lot of information before.

@adpi2
Copy link
Member Author

adpi2 commented Dec 7, 2020

@er1c asked (Twitter)

I started trying to stub some of it out: https://github.com/sbt/sbt/compare/develop...er1c:bsp-debug-session?expand=1 but in the sbt project, this boilerplate needs to be in the sbt-metals?

I don't have a strong opinion. It may seem weird that the sbt implementation is a stub but also it makes the interface more stable for users. @eed3si9n what's your take on it?

@er1c
Copy link

er1c commented Dec 7, 2020

@adpi2 I guess I was going down the path of implementing it in sbt - that's where the stub comes in trying to rough it out and wrap my head around it too. I hadn't read this ticket or thought far enough along on the added dependency this would incur.

It seems like sbt's BSP protocol needs to handle the debugSession/start and at least hand if off to the sbt-metals plugin? Or am I wrong here?

@adpi2
Copy link
Member Author

adpi2 commented Dec 7, 2020

It seems like sbt's BSP protocol needs to handle the debugSession/start and at least hand if off to the sbt-metals plugin?

Not really because a plugin can extend the sbt server with any any json-rpc request (or notification):

object DAPPlugin extends AutoPlugin {
   val buildSettings = Seq(
     serverHandlers += ServerHandler { callback => 
       request {
            case r: JsonRpcRequestMessage if r.method == "debugSession/start") => ???
      }
    }
  )
}

Also of course you can add your own tasks and settings in a plugin: https://www.scala-sbt.org/1.x/docs/Plugins.html

EDIT: Fundamentally the BuildServerProtocol could be a plugin in sbt

@er1c
Copy link

er1c commented Dec 7, 2020

I am not yet sure how to do it in a plugin infrastructure and much more work might be needed here also on the sbt side.

@tgodzik Does the above response from @adpi2 solve this issue, or are there other plugin infrastructure/work on the sbt side that you are thinking about?

@tgodzik
Copy link
Contributor

tgodzik commented Dec 7, 2020

I am not yet sure how to do it in a plugin infrastructure and much more work might be needed here also on the sbt side.

@tgodzik Does the above response from @adpi2 solve this issue, or are there other plugin infrastructure/work on the sbt side that you are thinking about?

Awesome, I think that solves it really. It seems it should be doable by extracting some code from Bloop https://github.com/scalacenter/bloop/tree/master/frontend/src/test/scala/bloop/dap

@ckipp01
Copy link
Member

ckipp01 commented Dec 7, 2020

We've been talking about it with @adpi2 and we decided to put it here as a feature request, since we are the most invested party here

True, but I also think that it's important that the DAP implementation for sbt be as client-agnostic as it's able to be so that down the road if another tool wants to tie into it, it's easier.

create a separate library that we can depend from Bloop also

I'm all for taking the dap package out of Bloop and publishing as something that's standalone. If we can reduce the amount of work that's needed on the sbt side to implement this plus share code with the Bloop integration, that's a win win from my point of view. I'm all for de-duplicating efforts.

add a new plugin the the scalameta organisation

I guess this is where I may differ in opinion a bit, but does this really make the most sense to have this under scalameta? Will the DAP implementation for sbt not be generic to use with another client? With my limited knowledge of DAP I sort of assumed that this wouldn't be really coupled to Metals really at all. I mainly bring this up because as we are all probably aware, more repos/projects add to maintenance time and upkeep. Since this one seems quite sbt specific, it makes more sense for this to be done on the sbt side. Again, that's just my opinion, and me trying to be careful with the amount of stuff we are committing to maintaining. Please note that I don't speak for all of Scalameta, this is just my opinion and something that I think we should be mindful of.

It seems like sbt's BSP protocol needs to handle the debugSession/start and at least hand if off to the sbt-metals plugin? Or am I wrong here?

Well there really isn't a handoff to sbt-metals. sbt-metals is purposefully very small. I prefer to keep it as small as possible on our side and to do as little as possible. Basically, my idea would be for it to just set what is needed to use sbt as a Build Server and to handle everything else. The main reason I say that is that we'll only see more build servers included (👀 looking at you https://github.com/scalameta/metals-feature-requests/issues/160), so the less Metals needs to do outside of BSP to interact at all with the Build Server the better. This hopefully means very thin plugins or no plugins that Metals doesn't need to maintain, but I understand there is a balance there.

All this to say, let's work together, come up with a solution that hopefully works well for everyone. It's been fantastic to see the sbt BSP support growing, more people using it, and things getting ironed out.

@er1c
Copy link

er1c commented Dec 7, 2020

To summarize:

  • create a separate debugSession/start library that we can depend from bloop
  • bloop uses library
  • sbt-metals adds support for library
  • ???
  • Profit

Questions:

@adpi2
Copy link
Member Author

adpi2 commented Dec 8, 2020

Where does the debugSession/start library live? (bloop, scalameta, build-server-protocol) (perhaps it should just continue to live in dap & dap-tests just split into a separate publish package?

I propose to create a scala-debug repo in the scalacenter org and to add you all as maintainer of it. It makes sense because:

  • DAP implementation is not directly related to scalameta
  • Bloop already is in the scalacenter org

Also I think we can put the sbt plugin inside the same repo in a distinct module. As @ckipp01 puts it, DAP is not coupled to Metals, so its preferable to separate those two.

In the future, once the project is mature enough, we can always discuss with sbt maintainers to merge it in sbt.

@adpi2
Copy link
Member Author

adpi2 commented Dec 8, 2020

@ckipp01
Copy link
Member

ckipp01 commented Dec 8, 2020

That all sounds great @adpi2! Can we maybe call it scala-dap instead of scala-debug just to be crystal clear with the name that it's an adapter not a debugger of some sort?

@adpi2
Copy link
Member Author

adpi2 commented Dec 8, 2020

dap is cryptic for most people so maybe scala-debug-adapter?

@ckipp01
Copy link
Member

ckipp01 commented Dec 8, 2020

dap is cryptic for most people so maybe scala-debug-adapter?

Good point. Sounds good to me!

@tgodzik
Copy link
Contributor

tgodzik commented Dec 8, 2020

@er1c if you plan on working on it and need any help at all please let me know anytime.

@adpi2
Copy link
Member Author

adpi2 commented Feb 12, 2021

The v1.0.0 of the scala-debug-adapter is released: https://github.com/scalacenter/scala-debug-adapter/releases/tag/v1.0.0
The PR in Bloop that uses this first version is ready to be reviewed: scalacenter/bloop#1454
The next step is the proper integration of the sbt-debug-adapter plugin in Metals, which is also related to #183.

@adpi2 adpi2 closed this as completed Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants