launch.json support changes

[bbenzikry]

This is a draft PR as I wanted to get some suggestions as to if this even makes sense @mfussenegger

• Add support for a non compliant nvimKey in debugger configurations for launch.json
  The problem that arose for me was that many configurations used by default vscode have a type that is not the file extension expected by nvim-dap to be the key in the configurations table.

• Make sure adapterID is set to the type instead of nvim-dap.
  Some adapters actually check the adapter ID and fail when it's not the correct type.

• Allow overriding clientID and clientName for a configuration definition. This allows for interesting scenarios such as simulating vscode-only debuggers

[mfussenegger]

Thank you for the PR. Overall I'm not opposed, some more detailed feedback:

Add support for a non compliant nvimKey in debugger configurations for launch.json
The problem that arose for me was that many configurations used by default vscode have a type that is not the file extension expected by nvim-dap to be the key in the configurations table.

If the motivation is to re-use existing vscode specific launch.json files, wouldn't it make sense if the mapping would be part of the load_launchjs call? E.g. load_launchjs(nil, {<type>: <nvimKey>}) or something like that.

I'm not sure if I understand the problem correct. Do you have a concrete example?

Make sure adapterID is set to the type instead of nvim-dap.
Some adapters actually check the adapter ID and fail when it's not the correct type.

I wonder if that shouldn't be a property within the dap.adapter definitions instead of the configuration.
So that there is an adapter.id property. Although that would require some more changes so that initialize has access to the used adapter definition.

Allow overriding clientID and clientName for a configuration definition. This allows for interesting scenarios such as simulating vscode-only debuggers

Are there any debug adapters that are vscode-only and reject other clients?
I'd also tend to put the override values elsewhere. I'm not too keen on overloading configuration for multiple purposes. The values there should be used to configure the debug adapter.

It would be possible to use the M.defaults table instead.

[bbenzikry]

Thanks for the feedback, much appreciated.

If the motivation is to re-use existing vscode specific launch.json files, wouldn't it make sense if the mapping would be part of the load_launchjs call? E.g. load_launchjs(nil, {: }) or something like that.
I'm not sure if I understand the problem correct. Do you have a concrete example?

The reasoning I had was to create a way that's embedded into the structure instead of letting this become too customizable and dependent on individual user configuration.
My far-reaching thought was to see if something we come up with could be added to the launch.json schema so users can easily add this ( and understand what it does ) when working in a team that's split on vscode / nvim without too much friction.

To be honest IMO this entire thing could also be solved with a bigger refactor where the adapter holds the extensions instead of the configuration key and with some kind of way to decide on the right adapter to launch.
This is what I wanted to do initially but thought it to be too much of a change without starting some kind of discussion here first.

As for the example scenario - this is from the launch.json in my test project

    "configurations": [
        {
            "name": "netcoredbg test",
            "nvimKey": "cs",
            "type": "netcoredbg",
            "request": "launch",
            "cwd": "${workspaceFolder}/testconsole",
            "program": "${workspaceFolder}/testconsole/bin/Debug/net6.0/testconsole.dll"
        },

I wonder if that shouldn't be a property within the dap.adapter definitions instead of the configuration.
So that there is an adapter.id property. Although that would require some more changes so that initialize has access to the used adapter definition.

Yeah, that's what I initially did but decided against it to not introduce a breaking change.

Are there any debug adapters that are vscode-only and reject other clients?

Yeah, especially microsoft adapters :) I wanted to reverse a DRM mechanism in one of those ( just for fun, microsoft, I swear) and noticed that clientID is verified

I'd also tend to put the override values elsewhere. I'm not too keen on overloading configuration for multiple purposes. The values there should be used to configure the debug adapter.
It would be possible to use the M.defaults table instead.

Can you elaborate on how you would approach this with defaults? I wanted something simple that would allow me to simply change something in launch.json and try again. Having the ID in the config may also be a good idea to support adapters that act differently based on clientID ( haven't seen that yet ) without changing the adapter setup.

[mfussenegger]

The reasoning I had was to create a way that's embedded into the structure instead of letting this become too customizable and dependent on individual user configuration.
My far-reaching thought was to see if something we come up with could be added to the launch.json schema so users can easily add this ( and understand what it does ) when working in a team that's split on vscode / nvim without too much friction.

I'm not sure how to solve this.
I kinda dislike the nvimKey solution because the configurations are supposed to configure the debug adapter, not the client.

But the other solutions I can think of are not ideal either:

• nvim-dap maintains a table to map between type and filetype
• nvim-dap supports a companion mapping file, next to launch.json that specifies how to map type to configuration key/filetype and load_launchjs would attempt to read that first. This is basically the same as the prior solution, except that it can be maintained external on a per project basis.
• Users need to link the configurations manually in their neovim config by adding a dap.configurations.cs = dap.configurations.netcoredbg line after your load_launchjs call or similar.

To be honest IMO this entire thing could also be solved with a bigger refactor where the adapter holds the extensions instead of the configuration key and with some kind of way to decide on the right adapter to launch.

Can you elaborate how that would work?

Yeah, that's what I initially did but decided against it to not introduce a breaking change.

I think it would even be possible with a soft deprecation.

adapter gains an optional id. If not present, it behaves as is.
launch already receives the adapter so there it would work.
attach receives argument overloading: If the first argument is a string, then the assumed function signature is host, port, config, opts and a deprecation warning is printed that the new signature is adapter, config, opts.
If the first argument is a table, then no warning is printed and the arguments are used as adapter, config, opts.

Can you elaborate on how you would approach this with defaults? I wanted something simple that would allow me to simply change something in launch.json and try again.

It would require setting the value in your vim config.

require('dap').settings.fallback.clientId = '...'

And you could set it also per adapter type:

require('dap').settings.python.clientId = '...'

But I'm not sure if this should be configurable at all.

[LogeshG5]

This is how I was able to load the .vscode/launch.json for cpp, I also needed this PR #217 for the predefined variables.

First create an adaptor

dap.adapters.cppdbg = dap.adapters.cpp --create an adapter for type cppdbg from an existing one
require('dap.ext.vscode').load_launchjs()  --load launch.json configuration

Then reassign the cppdbg configuration to cpp

-- launch.json is configured with `type = "cppdbg"`, so reassign those configuration `cpp`
dap.configurations.cpp = dap.configurations.cppdbg

or add the configurations loaded from launch.json to your existing configuration

-- Add to an existing configuration
for _, launchjs_config in pairs(dap.configurations.cppdbg) do 
   table.insert(dap.configurations.cpp, launchjs_config) 
end

@mfussenegger I was only trying to solve the type problem like you suggested. It would be helpful if this comes part of the documentation. Also I hope you will merge PR #217.

[mfussenegger]

6767b43 got merged which may help with this.

[NickHu]

Could we think about incorporating this line at least? It would make defining the adapter for cppdbg a lot easier.

[mfussenegger]

Could you explain why this would make defining the cppdbg adapter easier?

[NickHu]

Sure, it removes the necessity for the step

* Copy extension/cppdbg.ad7Engine.json to extension/debugAdapters/bin/nvim-dap.ad7Engine.json

on this page.

This is helpful because you might want to install cppdbg via a system package manager or something, and don't want to be messing around in system directories like this. Especially helpful if you're using the nix package manager too like me

[mfussenegger]

I've created a PR that allows to add a id property to the adapter definition: #264 / ccf14ca

[mfussenegger]

@bbenzikry Is there still anything in this PR that you think would be helpful to get in?

[bbenzikry]

@mfussenegger
Firstly, extremely sorry for abandoning the thread, was swamped with work.

I think nothing from the PR itself can help at this point and still maintain a reasonable way for users to configure.
I also feel my scenario is really specific and isn't likely to be encountered by most users unless they want to supply non standard parameters to a specific adapter ( the id addition provides enough customizability for most )

I will say that I really like your suggestion of adding a companion file. I think it can allow users to deal with any type of custom configuration ( maybe a file that automatically merges itself a-la Kustomize, with a proper schema ) but that probably requires a separate PR that takes other details / requirements into consideration

[mfussenegger]

Firstly, extremely sorry for abandoning the thread, was swamped with work.

No problem. I take my time to respond on issues too :)

I think nothing from the PR itself can help at this point and still maintain a reasonable way for users to configure.

Alright, in this case I'm closing the PR for now. If you have other suggestions on how to make your use-cases easier to accomplish - let me know.