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.

Sign up for GitHub

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

Jump to bottom

service/dap: accept a string list as launch request's buildFlags #3496

Merged
merged 1 commit into from
Sep 19, 2023
Merged

service/dap: accept a string list as launch request's buildFlags #3496

merged 1 commit into from
Sep 19, 2023

Conversation

hyangah
Copy link
Contributor

@hyangah hyangah commented Sep 7, 2023

This change accepts both string type and []string type (actually, []interface{} type due to Go's json decoding behavior).

Fixes #2718
For golang/vscode-go#1831, golang/vscode-go#1027

cc @suzmue

aarzilli
aarzilli approved these changes Sep 8, 2023
View reviewed changes
Copy link
Member

@aarzilli aarzilli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aarzilli
Copy link
Member

aarzilli commented Sep 8, 2023

Actually, it's making TestBadLaunchRequest fail.

@aarzilli aarzilli self-requested a review September 8, 2023 06:53
@hyangah
Copy link
Contributor Author

hyangah commented Sep 8, 2023

Changed to detect the error from parsing time.
BTW I noticed TestBadAccess fails on my mac (intel, go1.21.1) - looks like the stopped reason description/format was changed, and sometimes it fails to get exception info. Hard to believe this is related to this PR though. Will see if the CI catches it.

@aarzilli
Copy link
Member

aarzilli commented Sep 9, 2023 •
edited
Loading

looks like the stopped reason description/format was changed, and sometimes it fails to get exception info

Only sometimes? That's weird. Are you sure it's 1.21 and not 1.22?

aarzilli
aarzilli approved these changes Sep 9, 2023
View reviewed changes
Copy link
Member

@aarzilli aarzilli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@hyangah
Copy link
Contributor Author

hyangah commented Sep 12, 2023

looks like the stopped reason description/format was changed, and sometimes it fails to get exception info

Only sometimes? That's weird. Are you sure it's 1.21 and not 1.22?

I was using 1.21.1, but @suzmue pointed out this could be something strange in our corp machine or XCode version (I was on XCode 14). So, we don't need to worry about it yet.

@hyangah hyangah force-pushed the master branch 2 times, most recently from 622c3db to a1c9615 Compare September 15, 2023 19:05
@hyangah
service/dap: accept a string list as launch request's buildFlags
2cd8132 
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
derekparker approved these changes Sep 19, 2023
View reviewed changes
Copy link
Member

@derekparker derekparker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@derekparker derekparker merged commit 30b70bc into go-delve:master Sep 19, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derekparker derekparker derekparker approved these changes

@aarzilli aarzilli aarzilli approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

dap: consider []string for Launch BuildFlags
3 participants
@hyangah @aarzilli @derekparker