Search for dlv in go.toolsGopath too #2108
Conversation
package.json
Outdated
| @@ -487,6 +487,11 @@ | |||
| "maxStructFields": -1 | |||
| } | |||
| }, | |||
| "dlvToolPath": { | |||
There was a problem hiding this comment.
Why do we need this setting? We should be able to determine the path just by using getBinPath like you have done in the goDebugConfiguration.ts file
There was a problem hiding this comment.
If we are already passing it via the debug configuration. We might as well add it to the schema. Otherwise it's still possible to specify it but VS Code will warn about it.
We are determining the path via getBinPath, this just allows to override it.
src/debugAdapter/goDebug.ts
Outdated
| @@ -356,7 +357,7 @@ class Delve { | |||
| return; | |||
| } | |||
|
|
|||
| let dlv = getBinPathWithPreferredGopath('dlv', [resolveHomeDir(env['GOPATH']), process.env['GOPATH']]); | |||
| let dlv = launchArgs.dlvToolPath || getBinPathWithPreferredGopath('dlv', [resolveHomeDir(env['GOPATH']), process.env['GOPATH']]); | |||
There was a problem hiding this comment.
Now that we use getBinPath to get the path, we dont have to use getBinPathWithPreferredGopath
There was a problem hiding this comment.
The code of the debug adapter seems, to me, to be designed to also run standalone from the extension, and as such, seems to implement defaults for many of its configuration properties, not relying on the extension always passing them in.
As such, I decided to keep it being able to find dlv by itself. If you still want to change it, just say so.
Note that we can also add a message informing the user that dlv is missing after the call to getBinPath, but I was not sure which code snippet to use for that.
There was a problem hiding this comment.
The code of the debug adapter seems, to me, to be designed to also run standalone from the extension, and as such, seems to implement defaults for many of its configuration properties, not relying on the extension always passing them in.
This is so because, historically there was no way for the extension to pass anything to the debug adapter. This has changed recently (as in last year) with the addition of vscode.DebugConfigurationProvider api using which we can now intercept the debug configuration, update it, before it gets passed on to the debug adapter. This code resides in the goDebugConfiguration.ts file.
From this point on, the guidance I am using is
- If a setting/configuration stays the same for all debugging scenarios, then they would be part of the common settings
- If users might want a setting/configuration to be different for different debugging scenarios (like buildFlags, delve api version, trace, etc), then they live as part of the debug configuration in the launch.json file.
Coming to our current case of the path to delve, there isn't a need for the user to specify different paths for different debugging scenarios. Which is why it need not be part of the official debug configuration in the launch.json file.
Note that we can also add a message informing the user that dlv is missing after the call to getBinPath, but I was not sure which code snippet to use for that.
Rejecting the promise just like in https://github.com/Microsoft/vscode-go/blob/master/src/debugAdapter/goDebug.ts#L276 with a message will show the message to the user.
There was a problem hiding this comment.
There is this code: https://github.com/Microsoft/vscode-go/blob/633d1f28605d295146d38b0975fafe5598aab097/src/debugAdapter/goDebug.ts#L361-L364
Which I'm not sure how to change in this PR. We can also technically move the check that dlv was found to the call site of getBinPath. But, yet again, I'm unsure how to correctly check (path.isAbsolute/existsSync/other?), or how to report the error (throw/Promise.reject (The code is not returning a Promise currently)/window.showErrorMessage/other?).
There was a problem hiding this comment.
Check if the value returned by getBinPath is an absolute path or not. That is what we follow for all other tools in the extension.
src/debugAdapter/goDebug.ts
Outdated
| @@ -206,6 +206,7 @@ interface LaunchRequestArguments extends DebugProtocol.LaunchRequestArguments { | |||
| output?: string; | |||
| /** Delve LoadConfig parameters **/ | |||
| dlvLoadConfig?: LoadConfig; | |||
| dlvToolPath?: string; | |||
There was a problem hiding this comment.
Any reason to keep this optional?
Fixes #2099