Extending lsp.client with support for DAP

[jtulach]

I'd like NetBeans to have a support for DAP. To debug GraalVM based languages as well as Python, for example.

[jtulach]

Debugging Python inside NetBeans

Run the Python file like this:

$ python -m debugpy --listen localhost:5678 --wait-for-client <file>.py

It should now wait for connections from the IDE. In the IDE, select Debug/Attach Debugger...:

In the dialog, fill in:

• Debugger: Debugger Adapter Protocol (DAP) Debugger
• Hostname: localhost
• Port: 5678
• Connection type: Attach
• Additional configuration:

{
    "arguments": {}
}

• Attach after configure - unchecked

Confirm the dialog - the IDE should now debug the Python code.

[jtulach]

The debugging works with GraalVM based languages as well. Just download GraalVM for JDK 17 and Graal.js 17 and debug sieve.js with following command sieve/js$ /graalvm-17/bin/js --dap sieve.js:

debugger connects and one can step over the code.

[jtulach]

All the CI checks are green. Is there anything else to do before merging this functionality in?

[matthiasblaesing]

Interesting addition! I left a few inline comments from experiences with LSP/CDT.

[matthiasblaesing]

So am I understanding this right, that the java lsp server invented its own DAP protocol?

[jlahoda]

Sorry, not sure what you mean here?

[jtulach]

The java lsp server has provided its own DAP protocol implementation since ages to allow JVM debugging in the VSCode. However that's the other side of the DAP protocol - VSCode was DAP client and NetBeans was DAP server.

The changes in lsp.client aim towards NetBeans being DAP client and the server can be anything.

[jlahoda]

FWIW, based on @matthiasblaesing's comments, I tried to do some more cleanup:
jtulach#5

notably:

• cleaning up use of String urls, using URIs internally
• cleaning up the breakpoint to not hold paths, but rather only hold FileObjects
• cleaning up the BreakpointAnnotationProvider to not use a weak listener, and holder for the listeners. Using ordinary listeners, which will be held as long as the DataObject exists, and should be automatically GCed when the DO is removed, which is presumably the intent
• renaming (DAP)Utils to DAPStackTraceAnnotationHolder

I believe the only two unhandled comments are:

• Extending lsp.client with support for DAP #8098 (comment) where I didn't understand the comment at all
• Extending lsp.client with support for DAP #8098 (comment) where I don't believe there's any issue - the implementors of the convertors should not need to touch LineBreakpointData.

[matthiasblaesing]

For the URI discussion: I did not mean, that usage should switch over to URI. That maybe a valid target, but it highly depends on the actual protocol promises. I see the type Source (https://microsoft.github.io/debug-adapter-protocol/specification#Types_Source) and the attribute is named path.

I looked a bit further and this is I think the relevant part:

https://microsoft.github.io/debug-adapter-protocol/overview#initialization

the format of file paths, native or uri

The default according to https://microsoft.github.io/debug-adapter-protocol/specification#Requests_Initialize default is path which corresponds to native paths, not URI!

So we need to decide: How to handle this wildcard. I bet there will be DAPs, that only support path, others will only support URI (its unsetteling, that there is even a wildcard there that opens another can or worms).

Each has drawbacks:

• path might fail in "Web" scenarios. Consider a webbrowser, where there is no mapping between an URI and a local path.
• URI might fail for simple adapters, that don't consider URIs as useless for the local usecase

It would be interesting to get samples of both and see how they react if they detected, that they can't comply with the request of the IDE. I.e. will the adapters correctly fail initialization if a source path type is requested, that it can't provide or will it pretend to work and just silently send invalid data.

[jlahoda]

For the URI discussion: I did not mean, that usage should switch over to URI. That maybe a valid target, but it highly depends on the actual protocol promises. I see the type Source (https://microsoft.github.io/debug-adapter-protocol/specification#Types_Source) and the attribute is named path.
[snip]

I guess I don't quite see the huge problem. The protocol by default uses path, and this code apparently does not change the default, i.e. what goes over the wire is a path. (And to make things work, both client and server must see the same file using the same path.) What URLPathConvertor used to do, and what URIPathConvertor is doing now, is the translation between what the IDE uses internally (which used to be a String-encoded URL before, and is a java.net.URI now) to what the wire protocol expects (i.e. a path). The protocol side was not changed at all, only the IDE side.

The main point of the URIPathConvertor is to leave the door half-open to provide a different conversion (e.g. when both the sources and the debugger server would be remote), I don't see why it would need to be solved now - the current code should work for local paths (at least for local paths on UNIX systems). When there's a need to make things work for remote stuff, we can do whatever tweaks are necessary.

[mbien]

all tests green, removing the label again (only added it to check it at least once)

[jtulach]

I did not mean, that usage should switch over to URI.

The review has revealed bugs in treating the two kinds of strings. By using URI in the code we clearly identify what is supposed to be a URL.

(which used to be a String-encoded URL before, and is a java.net.URI now)

I like the separation.

point of the URIPathConvertor is to leave the door half-open to provide a different conversion

For now, let's make the interface package private in 8049bec650

[matthiasblaesing]

@jtulach @jlahoda thank you both for the updates. I think my points were all addressed. I did not look at all the code, but what I saw looks reasonable to me.

One think I'm still fuzzy about is the story about breakpoints (concrete BreakpointConvertor) what is its role here?

[dbalek]

Looks reasonable to me. Good job.

[jlahoda]

One think I'm still fuzzy about is the story about breakpoints (concrete BreakpointConvertor) what is its role here?

To my knowledge: in the NetBeans API, every mime type (language) needs to provide its own breakpoint implementations. And there can be only sensibly one breakpoint implementation for each language (technically, we can have multiple breakpoint implementations, I think, but the UI for that would not be sensible). So while this PR adds a "generic" implementation of breakpoints that can be used for languages that don't provide other breakpoint implementations, this generic implementation cannot (to my knowledge) be used for the languages that already provide breakpoint implementations, like Java, C/C++, PHP or JavaScript.

But, it may still be sensible to permit the use of the DAP debugger even for these languages in some cases. Hence the convertor - it can provide data from language-specific breakpoints to the DAP debugger, so that the DAP debugger can use them.

That's my understanding at least.

So, we have breakpoints for Java, C/C++, PHP, JavaScript, and possibly other languages. And, with

[jtulach]

To my knowledge: in the NetBeans API, every mime type (language) needs to provide its own breakpoint implementations.

So, we have breakpoints for Java, C/C++, PHP, JavaScript, and possibly other languages.

Yeah, now when I see how other systems like VSCode handle breakpoints I agree we made a mistake letting/forcing every language to implement its own line breakpoint. Rather than that there should have been a generic line breakpoint assignable to "almost" any file. When a new debugging session is about to start each language debugger would read such a generic line breakpoints and convert them to whatever is reasonable for the language debugger.

Alas, we don't have such a system and we end up with BreakpointConvertor.

"Almost"

Languages shall control whether clicking the editor gutter adds line breakpoint or not and whether they display breakpoint line in the editor, but technically the line breakpoint could be anywhere.

[jtulach]

I think the contribution is reviewed and tested. Let's integrate it and hope it won't need to be reverted (as sometimes happens to my PRs).

[mbien]

was this just merged without squashing?

[jtulach]

was this just merged without squashing?

This PR already contains contributions from other authors. It would be against netiquete to squash it all into a single commit. However yes, I could do better with my own commits as I see I left a little more noise in the history than necessary. I'll learn to behave. I promise.

[matthiasblaesing]

This might be helpful: https://lists.apache.org/thread/l2bkws1cz0s68n471wqsn3h6yz92hqjo