10011: Add DebugProtocolSource and DebugProtocolBreakpoint #10926

CamilleLetavernier · 2022-03-25T13:19:51Z

Signed-off-by: Camille Letavernier [email protected]

What it does

Introduce new APIs for the Theia plug-in API: DebugProtocolBreakpoint, DebugProtocolSource, and related methods:

DebugSession.getDebugProtocolBreakpoint()
debug.asDebugSourceUri()

How to test

I've created a small test extension that just prints breakpoints on the console, after using the new APIs. It can be used to test the changes:

https://github.com/CamilleLetavernier/debug-protocol-extension/releases

To use it:

Run the Test debug protocol command to start observing breakpoints of the active debug session
Start a debug session (Should work with any debug adapter) and toggle some breakpoints
The extension will print the "DebugProtocolBreakpoints" corresponding to each breakpoint in the active session. If the Breakpoint has a source, the source URI will be displayed as well (Actual data depends on the debug adapter)

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

refs #10011

CamilleLetavernier · 2022-03-25T13:22:58Z

Note: I wasn't sure how to actually match Breakpoints to DebugProtocolBreakpoint in debug-session.tsx#getBreakpoint(id), as it seems they don't use matching IDs.

I've modified the Breakpoint constructor to accept an optional initial ID (that can be copied from the plug-in Breakpoint), but I'm not sure that's the right way to go. I couldn't figure out a better way to match the various breakpoints, though.

colin-grant-work · 2022-03-28T16:46:33Z

@CamilleLetavernier, are you familiar with any existing plugins that use this API? Your test extension is a good proof of functionality, but to address your question about matching breakpoints, it would be good to be able to see what the API is used for elsewhere.

msujew · 2022-03-28T18:38:53Z

@colin-grant-work It looks like the profiling feature of the vscode-js-debug extension uses getDebugProtocolBreakpoint. The debug.asDebugSourceUri is also used in there.

JonasHelming · 2022-04-20T14:26:31Z

@colin-grant-work

colin-grant-work · 2022-04-20T14:44:38Z

@CamilleLetavernier, @JonasHelming, I'll take a look at this this week, either today or tomorrow.

vince-fugnitto · 2022-04-20T17:20:30Z

@CamilleLetavernier do you mind rebasing? Hopefully CI will kick in again after (currently does not show up on the pull-request).

colin-grant-work

I've made some comments on the code. It would be helpful if you could provide more details about how to use the plugin - what command to run to trigger its functionality and what output should be expected under different conditions. The plugin probably shouldn't add new listeners every time its command is run, since that can lead to confusing output if the same information is printed more than once.

packages/plugin-ext/src/plugin/node/debug/debug.ts

CamilleLetavernier · 2022-04-25T08:18:12Z

@CamilleLetavernier CamilleLetavernier requested a review from colin-grant-work 10 seconds ago

Sorry, that was a mis-click. No changes just yet!

vince-fugnitto

@CamilleLetavernier can you expand on #10926 (review) by updating the pull-request description on on how to properly test the features using the plugin provided, the steps are a bit vague at the moment.

vince-fugnitto · 2022-05-06T17:19:00Z

CHANGELOG.md

@@ -146,6 +146,7 @@
 - [vsx-registry] updated `requestretry` from `v3.1.0` to `v7.0.0` [#10831](https://github.com/eclipse-theia/theia/pull/10831)
 - [workspace] fixed `'save as'` for `untitled` schemes [#10608](https://github.com/eclipse-theia/theia/pull/10608)
 - [workspace] fixed the styling of the `path` in the dialog [#10814](https://github.com/eclipse-theia/theia/pull/10814)
+- [plugin] added support for `DebugProtocolBreakpoint` and `DebugProtocolSource` [#10011](https://github.com/eclipse-theia/theia/issues/10011) - Contributed on behalf of STMicroelectronics


The entry should be moved under 1.26.0.

vince-fugnitto · 2022-05-06T17:23:30Z

packages/plugin-ext/src/plugin/node/debug/debug.spec.ts

+
+    const debug = new DebugExtImpl(mockRPCProtocol);
+
+    it('Should use sourceReference, path and sessionId', () => {


minor: consistency

Suggested change

it('Should use sourceReference, path and sessionId', () => {

it('should use sourceReference, path and sessionId', () => {

vince-fugnitto · 2022-05-06T17:23:54Z

packages/plugin-ext/src/plugin/node/debug/debug.spec.ts

+import { DebugSession } from '@theia/plugin';
+import * as chai from 'chai';
+import { ProxyIdentifier, RPCProtocol } from '../../../common/rpc-protocol';
+


minor: remove extra newline.

vince-fugnitto · 2022-05-06T17:27:29Z

packages/plugin-ext/src/plugin/node/debug/debug.spec.ts

+
+const expect = chai.expect;
+
+describe('Debug source URI:', () => {


minor: in general we have a parent describe that describes the file, then we have individual tests:

Suggested change

describe('Debug source URI:', () => {

describe('Debug API', () => {

describe('#asDebugSourceUri', () => {

vince-fugnitto · 2022-05-06T17:28:41Z

packages/plugin-ext/src/plugin/node/debug/debug.ts

+    static SCHEME = 'debug';
+    static SCHEME_PATTERN = /^[a-zA-Z][a-zA-Z0-9\+\-\.]+:/;


minor: move the static members to the top of the class.

It also appears that both the scheme and the pattern are already declared in debug-source.ts as DebugSource.SCHEME and DebugSource.SCHEME_PATTERN. Would it be possible to reuse those declarations, perhaps by moving them to the common area of the debug package? In fact the SCHEME_PATTERN should really be somewhere very generally accessible, since it isn't specific to debug at all.

colin-grant-work · 2022-05-31T21:37:43Z

@CamilleLetavernier, please ping me or Vince when this is ready for another review.

CamilleLetavernier · 2022-06-03T13:37:02Z

Hi @colin-grant-work, @vince-fugnitto,

Thank you for your feedback. I've updated the pull request according to your comments. I also edited the PR description to include some testing steps.

I've also pushed a new version of the small extension I used to print the breakpoints, to avoid installing the listeners multiple times.

Unfortunately, for some reason, I can't get the Java Debugger to work in my local branch anymore; so I can't actually test the behavior of the debug session. I tried with Redhat Java 1.6 and 1.7 (And Java debug 0.40, 0.41), but I'm getting these errors when I start debugging:

root INFO [hosted-plugin: 653754] Debug configuration provider has been registered: java, trigger: 1
root ERROR [hosted-plugin: 653754] Promise rejection not handled in one second: Error: Connection got disposed. , reason: Error: Connection got disposed.
root ERROR [hosted-plugin: 653754] With stack trace: Error: Connection got disposed.
    at Object.dispose (/tmp/vscode-unpacked/redhat.java-1.6.0.vsix/extension/dist/extension.js:2:1025551)
    at Object.dispose (/tmp/vscode-unpacked/redhat.java-1.6.0.vsix/extension/dist/extension.js:2:872493)
    at b.handleConnectionClosed (/tmp/vscode-unpacked/redhat.java-1.6.0.vsix/extension/dist/extension.js:2:872681)
    at b.handleConnectionClosed (/tmp/vscode-unpacked/redhat.java-1.6.0.vsix/extension/dist/extension.js:2:946354)
    at t (/tmp/vscode-unpacked/redhat.java-1.6.0.vsix/extension/dist/extension.js:2:870795)
    at invoke (/tmp/vscode-unpacked/redhat.java-1.6.0.vsix/extension/dist/extension.js:2:1027177)
    at s.fire (/tmp/vscode-unpacked/redhat.java-1.6.0.vsix/extension/dist/extension.js:2:1027938)
    at Q (/tmp/vscode-unpacked/redhat.java-1.6.0.vsix/extension/dist/extension.js:2:1014690)
    at invoke (/tmp/vscode-unpacked/redhat.java-1.6.0.vsix/extension/dist/extension.js:2:1027177)
    at s.fire (/tmp/vscode-unpacked/redhat.java-1.6.0.vsix/extension/dist/extension.js:2:1027938)

colin-grant-work

@CamilleLetavernier, would you mind rebasing. At the moment #11263 is preventing the execution of debug sessions with your code, but #11268 was merged this morning to fix it.

colin-grant-work · 2022-06-09T16:37:59Z

packages/plugin-ext/src/plugin/node/debug/debug.ts

+            return new TheiaURI().withScheme(DEBUG_SCHEME).withPath(raw.path || '').withQuery(query);
+        }
+        if (!raw.path) {
+            throw new Error('Unrecognized source type: ' + JSON.stringify(raw));
+        }
+        if (raw.path.match(SCHEME_PATTERN)) {
+            return new TheiaURI(raw.path);
+        }
+        return new TheiaURI(URI.file(raw.path));
+    }


Is there a reason to use the Theia URI implementation rather than VSCode-style URI's from the start here?

I'm not sure. I have to say, I'm a bit confused with all the classes named "URI" delegating to each other, and are sometimes renamed during import :)

CamilleLetavernier · 2022-07-11T13:15:38Z

The branch has been rebased (I've also squashed all commits, as the last rebase was a bit more complex). I've been able to run the debug plug-in/command with the current version of Theia, so it should all be good now

JonasHelming · 2022-07-20T08:23:29Z

@colin-grant-work Are you fine with merging this now?

colin-grant-work · 2022-07-20T13:58:31Z

@JonasHelming, I'll take a look today

packages/plugin-ext/src/plugin/debug/debug-ext.ts

colin-grant-work

The behavior of the example plugin is compatible between Theia and VSCode 👍. Please see the suggestion for removing use of TheiaURI.

Add "DebugProtocolSource" and "DebugProtocolEndpoint" to the plug-in API, as well as the methods using these interfaces: - DebugSession.getDebugProtocolBreakpoint() - debug.asDebugSourceUri() 10011: Fix asDebugSourceUri - Rename asDebugSourceURI to getDebugSourceUri to avoid name clash - Fix the way the debug source query is built - Add tests 10011: Minor code improvements - Improve test description to be consistent with other tests - Move the SCHEME and SCHEME_PATTERN constants to debug/common for reusability 10011: Rebase on master - Rebase on master and resolve conflicts Contributed on behalf of STMicroelectronics Signed-off-by: Camille Letavernier <[email protected]>

JonasHelming requested a review from vince-fugnitto March 25, 2022 14:01

colin-grant-work self-requested a review April 5, 2022 20:46

msujew mentioned this pull request Apr 16, 2022

API Gap theia master vs. VS Code 1.55.2 #9863

Closed

48 tasks

colin-grant-work reviewed Apr 20, 2022

View reviewed changes

packages/plugin-ext/src/plugin/node/debug/debug.ts Outdated Show resolved Hide resolved

packages/plugin-ext/src/plugin/node/debug/debug.ts Outdated Show resolved Hide resolved

packages/plugin-ext/src/plugin/node/debug/debug.ts Outdated Show resolved Hide resolved

CamilleLetavernier requested a review from colin-grant-work April 25, 2022 08:17

JonasHelming added the vscode issues related to VSCode compatibility label May 2, 2022

CamilleLetavernier force-pushed the issues/10011 branch 2 times, most recently from d75ba90 to 2d060fb Compare May 5, 2022 15:04

vince-fugnitto reviewed May 6, 2022

View reviewed changes

colin-grant-work removed their request for review May 31, 2022 21:37

CamilleLetavernier force-pushed the issues/10011 branch from 2d060fb to 56aef99 Compare June 3, 2022 13:24

colin-grant-work self-requested a review June 3, 2022 14:00

colin-grant-work reviewed Jun 9, 2022

View reviewed changes

CamilleLetavernier force-pushed the issues/10011 branch 2 times, most recently from ef2ce1e to 4e66bf9 Compare July 8, 2022 13:54

JonasHelming requested a review from colin-grant-work July 11, 2022 20:01

colin-grant-work reviewed Jul 20, 2022

View reviewed changes

packages/plugin-ext/src/plugin/debug/debug-ext.ts Outdated Show resolved Hide resolved

colin-grant-work reviewed Jul 20, 2022

View reviewed changes

CamilleLetavernier added 2 commits July 21, 2022 10:59

10011: Use VSCode URI instead of Theia URI

611ab0a

CamilleLetavernier force-pushed the issues/10011 branch from 4e66bf9 to 611ab0a Compare July 21, 2022 09:04

JonasHelming self-requested a review July 21, 2022 09:50

JonasHelming approved these changes Jul 21, 2022

View reviewed changes

JonasHelming merged commit 6aa0d78 into eclipse-theia:master Jul 21, 2022

JonasHelming added this to the 1.28.0 milestone Jul 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

10011: Add DebugProtocolSource and DebugProtocolBreakpoint #10926

10011: Add DebugProtocolSource and DebugProtocolBreakpoint #10926

CamilleLetavernier commented Mar 25, 2022 •

edited

Loading

CamilleLetavernier commented Mar 25, 2022

colin-grant-work commented Mar 28, 2022

msujew commented Mar 28, 2022

JonasHelming commented Apr 20, 2022

colin-grant-work commented Apr 20, 2022

vince-fugnitto commented Apr 20, 2022

colin-grant-work left a comment

CamilleLetavernier commented Apr 25, 2022

vince-fugnitto left a comment

vince-fugnitto May 6, 2022

vince-fugnitto May 6, 2022

vince-fugnitto May 6, 2022

vince-fugnitto May 6, 2022

vince-fugnitto May 6, 2022

colin-grant-work May 6, 2022 •

edited

Loading

colin-grant-work commented May 31, 2022

CamilleLetavernier commented Jun 3, 2022 •

edited

Loading

colin-grant-work left a comment

colin-grant-work Jun 9, 2022

CamilleLetavernier Jul 11, 2022

CamilleLetavernier commented Jul 11, 2022

JonasHelming commented Jul 20, 2022

colin-grant-work commented Jul 20, 2022

colin-grant-work left a comment


		const debug = new DebugExtImpl(mockRPCProtocol);

		it('Should use sourceReference, path and sessionId', () => {


		const expect = chai.expect;

		describe('Debug source URI:', () => {

-describe('Debug source URI:', () => {
+describe('Debug API', () => {
+    describe('#asDebugSourceUri', () => {

		static SCHEME = 'debug';
		static SCHEME_PATTERN = /^[a-zA-Z][a-zA-Z0-9\+\-\.]+:/;

10011: Add DebugProtocolSource and DebugProtocolBreakpoint #10926

10011: Add DebugProtocolSource and DebugProtocolBreakpoint #10926

Conversation

CamilleLetavernier commented Mar 25, 2022 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

CamilleLetavernier commented Mar 25, 2022

colin-grant-work commented Mar 28, 2022

msujew commented Mar 28, 2022

JonasHelming commented Apr 20, 2022

colin-grant-work commented Apr 20, 2022

vince-fugnitto commented Apr 20, 2022

colin-grant-work left a comment

Choose a reason for hiding this comment

CamilleLetavernier commented Apr 25, 2022

vince-fugnitto left a comment

Choose a reason for hiding this comment

vince-fugnitto May 6, 2022

Choose a reason for hiding this comment

vince-fugnitto May 6, 2022

Choose a reason for hiding this comment

vince-fugnitto May 6, 2022

Choose a reason for hiding this comment

vince-fugnitto May 6, 2022

Choose a reason for hiding this comment

vince-fugnitto May 6, 2022

Choose a reason for hiding this comment

colin-grant-work May 6, 2022 • edited Loading

Choose a reason for hiding this comment

colin-grant-work commented May 31, 2022

CamilleLetavernier commented Jun 3, 2022 • edited Loading

colin-grant-work left a comment

Choose a reason for hiding this comment

colin-grant-work Jun 9, 2022

Choose a reason for hiding this comment

CamilleLetavernier Jul 11, 2022

Choose a reason for hiding this comment

CamilleLetavernier commented Jul 11, 2022

JonasHelming commented Jul 20, 2022

colin-grant-work commented Jul 20, 2022

colin-grant-work left a comment

Choose a reason for hiding this comment

CamilleLetavernier commented Mar 25, 2022 •

edited

Loading

colin-grant-work May 6, 2022 •

edited

Loading

CamilleLetavernier commented Jun 3, 2022 •

edited

Loading