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

dap: consider []string for Launch BuildFlags #2718

Closed
hyangah opened this issue Sep 23, 2021 · 8 comments · Fixed by #3496
Closed

dap: consider []string for Launch BuildFlags #2718

hyangah opened this issue Sep 23, 2021 · 8 comments · Fixed by #3496

Comments

@hyangah
Copy link
Contributor

hyangah commented Sep 23, 2021

DAP LaunchConfig.BuildFlags is defined as string type.
It is string because VS Code Go defined the buildFlags in that way in its legacy debug adapter that invokes dlv debug or dlv test, and dlv command line takes the build flags (--build-flags) as a string.

DAP can take Launch configuration in JSON, which can support more rich types than a simple command line flag.
Historically, the subtle difference between how dlv is parsing the build flags and how go command is parsing the flags was annoying and sometimes frustrating (e.g. golang/vscode-go#1027).

Please consider to take []string and bypass delve's flag parsing.

@hyangah hyangah changed the title dap: consider []string for Launch dap: consider []string for Launch BuildFlags Sep 23, 2021
@aarzilli
Copy link
Member

I believe if you bypass config.SplitQuotedFields completely and users write this:

buildFlags: [ "-ldflags='-X main.V=Hello'" ],

they will have the same problem.

@aarzilli
Copy link
Member

I wonder if it's better to just add double-quote handling to SplitQuotedFields

@hyangah
Copy link
Contributor Author

hyangah commented Sep 24, 2021

I guess users should write If []string is allowed

buildFlags: ["-ldflags=-X main.V=Hello"]

Right? Launching go commands in node.js seems fine with both ["-ldflags='-X main.V=Hello'"] and ["-dlflags=-X main.V=Hello"], but it looks like go command doesn't seem happy with the former as you guessed.

Currently vscode-go and gopls both use []string for buildFlags. So, the flag plumbing from vscode-go to gopls is a bit easier. For delve, if we want to the the flag plumbing, is it ok to simply concatenate buildFlags setting with ' '?

@aarzilli
Copy link
Member

is it ok to simply concatenate buildFlags setting with ' '?

With a space? No it will not work if they contain spaces themselves.
They need to be quoted and then concatenated with spaces if they are to be passed to SplitQuotedFields, or just call SplitQuotedFields on each. Regardless if double quotes are used it won't work.

It probably works with node.js because it's doing some shell-like quote processing magic (or just going through the shell).

@aarzilli
Copy link
Member

or just call SplitQuotedFields on each.

This would actually be wrong, BTW. But something like it that strips quotes. OTOH I think doing nothing and letting

buildFlags: [ "-ldflags='-X main.V=Hello'" ],

fail would also be ok.

@hyangah
Copy link
Contributor Author

hyangah commented Oct 13, 2021

@aarzilli We are currently debating whether you're against this feature request completely or not. Can you please clarify to end our confusion :-)?

@aarzilli
Copy link
Member

aarzilli commented Oct 14, 2021

No, it's fine. I was just pointing out that:

Historically, the subtle difference between how dlv is parsing the build flags and how go command is parsing the flags was annoying and sometimes frustrating

is really about the subtle difference between dlv parsing and the shell parsing and it wouldn't really go away and:

buildFlags: [ "-ldflags='-X main.V=Hello'" ],

which is the naive thing to write, wouldn't work. But maybe it's fine that it doesn't.

@aarzilli
Copy link
Member

Sorry for the confusion.

hyangah added a commit to hyangah/delve that referenced this issue Sep 7, 2023
This change accepts both string type and []string (actually,
[]interface{} type due to Go's json decoding behavior.

Fixes go-delve#2718
For golang/vscode-go#1831, golang/vscode-go#1027
hyangah added a commit to hyangah/delve that referenced this issue Sep 8, 2023
This change accepts both string type and []string. dap.BuildFlags
is a union of string and []string.

Fixes go-delve#2718
For golang/vscode-go#1831, golang/vscode-go#1027
hyangah added a commit to hyangah/delve that referenced this issue Sep 13, 2023
This change accepts both string type and []string. dap.BuildFlags
is a union of string and []string.

Fixes go-delve#2718
For golang/vscode-go#1831, golang/vscode-go#1027
hyangah added a commit to hyangah/delve that referenced this issue Sep 15, 2023
This change accepts both string type and []string. dap.BuildFlags
is a union of string and []string.

Fixes go-delve#2718
For golang/vscode-go#1831, golang/vscode-go#1027
hyangah added a commit to hyangah/delve that referenced this issue Sep 15, 2023
This change accepts both string type and []string. dap.BuildFlags
is a union of string and []string.

Fixes go-delve#2718
For golang/vscode-go#1831, golang/vscode-go#1027
derekparker pushed a commit that referenced this issue Sep 19, 2023
This change accepts both string type and []string. dap.BuildFlags
is a union of string and []string.

Fixes #2718
For golang/vscode-go#1831, golang/vscode-go#1027
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants