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

debug: be explicit about supporting conditional breakpoints #781

Closed
polinasok opened this issue Oct 14, 2020 · 7 comments
Closed

debug: be explicit about supporting conditional breakpoints #781

polinasok opened this issue Oct 14, 2020 · 7 comments
Labels
Debug Issues related to the debugging functionality of the extension. FrozenDueToAge
Milestone

Comments

@polinasok
Copy link
Contributor

polinasok commented Oct 14, 2020

According to the dap spec:

/**
   * An optional expression for conditional breakpoints.
   * It is only honored by a debug adapter if the capability 'supportsConditionalBreakpoints' is true.
   */
  condition?: string;

Our debug adapter does not set this capability on initialization, but it does appear to support conditional breakpoints (via "Edit Breakpoint"). We should set the capability and add tests for this to solidify the support. This way when the breakpoint is set, it won't have the "!" and an error message on hover, which are misleading because it will actually work.

We would also need to set this correctly in debugAdapter2 as well as it reports capabilities before dlv is launched. This feature has recently been added to dlv-dap (now tracked under #844)

image

@polinasok polinasok added Debug Issues related to the debugging functionality of the extension. DelveDAP labels Oct 14, 2020
@suzmue suzmue self-assigned this Oct 14, 2020
@suzmue
Copy link
Contributor

suzmue commented Oct 14, 2020

I'll add the tests update the configuration for the old debug adapter.

@gopherbot
Copy link
Collaborator

Change https://golang.org/cl/262346 mentions this issue: src/debugAdapter: indicate that conditional breakpoints are supported

gopherbot pushed a commit that referenced this issue Oct 15, 2020
Set supportsConditionalBreakpoints to be true on initialization to let
clients of the debug adapter know that we provide this capability.

Test that the capability is true and run a simple test for stopping on
conditional breakpoints.

Updates #781

Change-Id: I7b1dd5156fd346bb293582914fd809fabed368d1
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/262346
Trust: Suzy Mueller <suzmue@golang.org>
Run-TryBot: Suzy Mueller <suzmue@golang.org>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Polina Sokolova <polina@google.com>
@suzmue suzmue removed their assignment Oct 16, 2020
@gopherbot
Copy link
Collaborator

Change https://golang.org/cl/264198 mentions this issue: src/debugAdapter2: indicate that conditional breakpoints are supported

@hyangah
Copy link
Contributor

hyangah commented Oct 21, 2020

Related

#822: initializeRequest is still handled by the thin adapter; it's desirable to forward this to delve dap

#794: the stable version of delve may not have the capability so this can be a lie if we set it now. OTOH, we are currently operating with an implicit assumption - users should use the latest master version of delve to use. So, I think setting this true isn't too bad. cc @polinasok

@hyangah hyangah added this to the v0.18.0 milestone Oct 21, 2020
@hyangah
Copy link
Contributor

hyangah commented Oct 23, 2020

I will remove this from v0.18.0 milestone, and let it be tracked as a DelveDAP bug.

@polinasok
Copy link
Contributor Author

polinasok commented Oct 27, 2020

I mentioned setting this in debugAdapter2 in my opening description, but technically that's a separate issue and can be generalized to all new capabilities. As @hyangah reflected above, there are multiple options here for addressing this, so let's move the tracking/discussion for the long-term solution to another issue #844 that could be an umbrella for comparing and contrasting these options.

@hyangah hyangah modified the milestones: Backlog, v0.18.0 Oct 28, 2020
@hyangah
Copy link
Contributor

hyangah commented Oct 28, 2020

Thanks for the followup @suzmue and @polinasok . I will close this and place it back to v0.18.0 milestone for archive purpose.
Thanks @suzmue for the fixes.

@hyangah hyangah closed this as completed Oct 28, 2020
@golang golang locked and limited conversation to collaborators Jul 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Debug Issues related to the debugging functionality of the extension. FrozenDueToAge
Projects
None yet
Development

No branches or pull requests

4 participants