VSCode debug adapter protocol implementation #2447

tolusha · 2018-07-26T13:15:19Z

Reference issues

#1573
eclipse-che/che#8383

Description

This PR brings VSCode debug adapter protocol to the Theia IDE

Implemented features:

Establish Debug Sessions

Connect/Disconnect remote target
Start/Stop debug targets

Threads

Show threads of debug targets
Show Stack of Suspended Threads
Show Source for Suspended Threads

Suspend/Resume Actions

Suspend/Resume Actions vsx-registry: sanitize readme #9424

Stack Frame Variables

Display
Modify

Step actions

In/out/over

Breakpoints

Show list of Breakpoints
Remove Breakpoint

Line Breakpoint

DAP Create Line Breakpoint
display in editor

Demo vidoes

Demo 1
Demo 2

paul-marechal

Nice work !

Although here are the things that I picked up:

I am able to put breakpoints in launch.json (?)
The only breakpoints that show up in the debug panel are the ones placed in the json file, placing breakpoints in packages/filesystem/src/node/node-filesystem.ts doesn't show up in the panel.
Couldn't find a way to debug using Electron for Theia (maybe just a missing dep in the package.json)

Regarding the following coding pattern:

function shouldBeAsync(...args): Promise<void> {
    stuff();
    return Promise.resolve();
}

The issue here is that if an error is thrown, it will be done synchronously, and you will not be able to catch it using shouldBeAsync().catch(e => ...).

If this is intended, it's fine I guess.

I might have missed some things, but I'm sure Anton will have sharper eyes :)

paul-marechal · 2018-07-27T11:04:04Z

packages/debug/src/browser/breakpoint/breakpoint-manager.ts

+        @inject(ActiveLineDecorator) protected readonly lineDecorator: ActiveLineDecorator,
+        @inject(BreakpointDecorator) protected readonly breakpointDecorator: BreakpointDecorator,
+        @inject(BreakpointStorage) protected readonly storage: BreakpointStorage,
+        @inject(BreakpointsApplier) protected readonly breakpointApplier: BreakpointsApplier,


BreakpointDecorator, BreakpointStorage, so why BreakpointsApplier ?

Maybe it would be more consistent to call it BreakpointApplier too? (-s)

(even the variable is called breakpointApplier)

paul-marechal · 2018-07-27T11:28:33Z

packages/debug/src/browser/breakpoint/breakpoint-marker.ts

+            super.setMarkers(uri, BREAKPOINT_OWNER, newBreakpoints);
+        });
+
+        return Promise.resolve();


async update(data: Ext...) { ... }

Then you don't need the return Promise.resolve().

paul-marechal · 2018-07-27T11:29:19Z

packages/debug/src/browser/breakpoint/breakpoint-marker.ts

+        existedBreakpoints.push(breakpoint);
+        super.setMarkers(uri, BREAKPOINT_OWNER, existedBreakpoints);
+
+        return Promise.resolve();


Convert function to async and you won't need the return.

paul-marechal · 2018-07-27T11:31:44Z

packages/debug/src/browser/breakpoint/breakpoint-marker.ts

+        const breakpoints = super.findMarkers({ uri, dataFilter: b => DebugUtils.makeBreakpointId(b) !== id }).map(m => m.data);
+
+        super.setMarkers(uri, BREAKPOINT_OWNER, breakpoints);
+        return Promise.resolve();


paul-marechal · 2018-07-27T11:34:06Z

packages/debug/src/browser/breakpoint/breakpoint-marker.ts

+     * @returns true if breakpoint exists and false otherwise
+     */
+    exists(id: string): Promise<boolean> {
+        return Promise.resolve(super.findMarkers().some(m => DebugUtils.makeBreakpointId(m.data) === id));


function async exists(id: string): Promise<boolean> { return super.findMarkers().some(m => DebugUtils.makeBreakpointId(m.data) === id); }

(added the function keyword here for github to color the code)

thegecko · 2018-07-28T19:07:35Z

Really nice work @tolusha

I can confirm this works as expected when attaching to an existing node process and I think it's a really good base to build a debug experience matching vscode!

I took the testing further by implementing the vscode example mock debug extension and have this working as a launch request type debugger instead of the attach request type used in your node-debug package.

I'm happy to share this if anyone needs it, but I'm not sure how valuable it is to the Theia project.

In order to create a launch debugger, I needed to fill in a couple of very small bits in the debug code base. These can be seen here:

https://github.com/thegecko/theia/commits/vscode-debug-launch

Please let me know if you'd like to see this as a PR onto the vscode-debug-protocol branch before merging this PR or as a separate PR onto master later.

tolusha · 2018-07-30T06:16:53Z

https://github.com/thegecko/theia/commits/vscode-debug-launch
@thegecko Let's create a PR into vscode-debug-protocol

thegecko · 2018-07-30T11:09:31Z

@tolusha Done: #2463

svenefftinge · 2018-07-30T19:04:34Z

@tolusha Great work!

Is there a specific reason you made the debug view horizontal? I think chrome devtools and vs code do it really well by laying it out vertically, because with the collapsable sections you can have all the important sections in one view without wasting screen estate if you are not interested (because you can collapse). Also it would be more familiar to devs because of the two tools mentioned above are widely used.

tolusha · 2018-07-31T06:21:06Z

@marechal-p
PR is updated

tolusha · 2018-07-31T08:34:31Z

@svenefftinge
In my opinion a horizontal panel is more useful than a vertical one.
It is a matter of restyling to make it vertical but I prefer to postpone it until the full design of the debug panel is ready.

thegecko · 2018-07-31T08:42:31Z

IMO, if in doubt we should copy the layout used in vscode. People are used to it and debug adapters have been built with that layout in mind.

svenefftinge · 2018-08-10T07:18:24Z

It is a matter of restyling to make it vertical but I prefer to postpone it until the full design of the debug panel is ready.

Agreed. Could you clean up / squash the history a bit? Also the DCO check fails.

tolusha · 2018-08-14T08:58:11Z

tolusha · 2018-08-14T08:59:35Z

@svenefftinge
Commits are squashed and a new style is applied.

svenefftinge · 2018-08-14T11:44:43Z

@tolusha the build fails. Looks like it is because you run theia test but don't have any ./src/**/*.*spec.ts.

https://travis-ci.org/theia-ide/theia/jobs/415824849#L4037

In other cases we have put a dummy file.

akosyakov · 2018-08-14T13:48:49Z

Please add new packages to travis file for caching. I see there is a binary file, it is supposed to be checked in or downloaded each time?

tolusha · 2018-08-14T14:26:13Z

@akosyakov
The binary file is downloaded once.
https://github.com/theia-ide/theia/blob/vscode-debug-protocol/packages/debug-nodejs/gulpfile.js#L23

paul-marechal · 2018-08-14T14:38:10Z

@tolusha: it makes sense, although the file is currently checked in:
https://github.com/theia-ide/theia/pull/2447/files#diff-9253598657d98c7141891c1e62abaf23

git rm --cached packages/debug-nodejs/download/vscode-node-debug.tar.gz to remove it from the index, unless this is intended.

AlexTugarev · 2018-08-14T14:40:48Z

packages/debug-nodejs/package.json

+  },
+  "devDependencies": {
+    "@theia/ext-scripts": "^0.3.13",
+    "gulp": "^3.9.1",


Please, if there is nothing else used from gulp, could you convert it to plain nodejs like in @theia/java it's huge library to just download an archive in the end. Thanks!

In @tolusha 's defence, I'm surprised Theia doesn't use a task runner like gulp already. IMO, the npm scripts and theia-cli are alien and unwieldy. Perhaps a discussion for another time, however :)

We can bring the matter for next week's dev meeting?
I would be interested to hear about it :)

It is not about Theia, but about downstream projects, they will have to install gulp just to download binaries.

Using npm scripts as build tools is a common practice in JS world: https://www.keithcirkel.co.uk/how-to-use-npm-as-a-build-tool/
As well as abstracting common scripts via packages: https://blog.kentcdodds.com/automation-without-config-412ab5e47229

I don't really see a need to pull more dev dependencies and write additional js files for them.

tolusha · 2018-08-15T08:49:04Z

@AlexTugarev @marechal-p
fixed

tolusha · 2018-08-20T11:48:25Z

@svenefftinge
Any chance to merge?

akosyakov · 2018-08-21T04:23:39Z

packages/monaco/src/browser/monaco-editor.ts

@@ -58,6 +59,7 @@ export class MonacoEditor implements TextEditor, IEditorReference {
    protected readonly onSelectionChangedEmitter = new Emitter<Range>();
    protected readonly onFocusChangedEmitter = new Emitter<boolean>();
    protected readonly onDocumentContentChangedEmitter = new Emitter<TextDocumentChangeEvent>();
+    protected readonly onMouseDownEmitter = new Emitter<EditorMouseEvent>();


please add it toDispose in the constructor

akosyakov · 2018-08-21T04:26:47Z

packages/monaco/src/browser/monaco-editor.ts

+            const { lineNumber, column } = e.target.position;
+            const event = {
+                target: {
+                    ...e.target,


Why to copy other properties if they are not exposed? range and mosueColumn should not be copied but properly converted from 1-bazed to 0-based positions.

akosyakov

it looks safe, there almost no changes in other packages.

Please take care of comments in monaco package and merge.

Signed-off-by: Anatoliy Bazko <abazko@redhat.com>

tolusha · 2018-08-21T13:09:00Z

@akosyakov
CI failed, but I don't see how it related to my PR

paul-marechal · 2018-08-21T13:16:38Z

@tolusha if you are worried about appveyor: don't... The current error log is something rather common for some reason.

Maybe someone can restart the build, but travis linux + mac is green, so it should be good!

akosyakov · 2018-08-21T13:19:48Z

@tolusha if travis is green it is enough, tests are flaky on windows all the time

@tolusha

Theia support DAP since [this PR](eclipse-theia/theia#2447) cc @tolusha @slemeur @tsmaeder

tolusha added the dap debug adapter protocol label Jul 26, 2018

tolusha self-assigned this Jul 26, 2018

tolusha requested a review from akosyakov July 26, 2018 13:15

paul-marechal reviewed Jul 27, 2018

View reviewed changes

tolusha force-pushed the vscode-debug-protocol branch from 5b10181 to 73546f2 Compare July 31, 2018 06:17

tolusha force-pushed the vscode-debug-protocol branch from c7034ac to d718177 Compare August 14, 2018 08:53

tolusha force-pushed the vscode-debug-protocol branch from d718177 to 3636325 Compare August 14, 2018 08:58

AlexTugarev reviewed Aug 14, 2018

View reviewed changes

tolusha force-pushed the vscode-debug-protocol branch 2 times, most recently from 5828aea to 4eafe9d Compare August 17, 2018 11:42

dvc94ch mentioned this pull request Aug 19, 2018

Ship a custom LLDB with Rust support rust-lang/rust#48168

Open

5 tasks

akosyakov reviewed Aug 21, 2018

View reviewed changes

akosyakov approved these changes Aug 21, 2018

View reviewed changes

tolusha force-pushed the vscode-debug-protocol branch from 1d3c0f7 to 8c047bf Compare August 21, 2018 08:26

Introduce debug adapter protocol

6a03487

Signed-off-by: Anatoliy Bazko <abazko@redhat.com>

tolusha force-pushed the vscode-debug-protocol branch from 8c047bf to 6a03487 Compare August 21, 2018 08:35

tolusha merged commit 18a37db into master Aug 21, 2018

l0rd added a commit to l0rd/debug-adapter-protocol that referenced this pull request Oct 22, 2018

Add Theia among the Dev Tools

51628a9

Theia support DAP since [this PR](eclipse-theia/theia#2447) cc @tolusha @slemeur @tsmaeder

l0rd mentioned this pull request Oct 22, 2018

Add Theia among the Dev Tools microsoft/debug-adapter-protocol#10

Merged

tolusha deleted the vscode-debug-protocol branch June 13, 2019 18:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VSCode debug adapter protocol implementation #2447

VSCode debug adapter protocol implementation #2447

tolusha commented Jul 26, 2018

paul-marechal left a comment •

edited

Loading

paul-marechal Jul 27, 2018 •

edited

Loading

paul-marechal Jul 27, 2018

paul-marechal Jul 27, 2018

paul-marechal Jul 27, 2018

paul-marechal Jul 27, 2018 •

edited

Loading

thegecko commented Jul 28, 2018

tolusha commented Jul 30, 2018 •

edited

Loading

thegecko commented Jul 30, 2018

svenefftinge commented Jul 30, 2018

tolusha commented Jul 31, 2018

tolusha commented Jul 31, 2018

thegecko commented Jul 31, 2018

svenefftinge commented Aug 10, 2018 •

edited

Loading

tolusha commented Aug 14, 2018

tolusha commented Aug 14, 2018

svenefftinge commented Aug 14, 2018 •

edited

Loading

akosyakov commented Aug 14, 2018

tolusha commented Aug 14, 2018

paul-marechal commented Aug 14, 2018 •

edited

Loading

AlexTugarev Aug 14, 2018

thegecko Aug 15, 2018

paul-marechal Aug 15, 2018

akosyakov Aug 15, 2018

akosyakov Aug 15, 2018

tolusha commented Aug 15, 2018

tolusha commented Aug 20, 2018

akosyakov Aug 21, 2018

akosyakov Aug 21, 2018

akosyakov left a comment

tolusha commented Aug 21, 2018

paul-marechal commented Aug 21, 2018 •

edited

Loading

akosyakov commented Aug 21, 2018

VSCode debug adapter protocol implementation #2447

VSCode debug adapter protocol implementation #2447

Conversation

tolusha commented Jul 26, 2018

Reference issues

Description

Implemented features:

Demo vidoes

paul-marechal left a comment • edited Loading

Choose a reason for hiding this comment

paul-marechal Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-marechal Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

thegecko commented Jul 28, 2018

tolusha commented Jul 30, 2018 • edited Loading

thegecko commented Jul 30, 2018

svenefftinge commented Jul 30, 2018

tolusha commented Jul 31, 2018

tolusha commented Jul 31, 2018

thegecko commented Jul 31, 2018

svenefftinge commented Aug 10, 2018 • edited Loading

tolusha commented Aug 14, 2018

tolusha commented Aug 14, 2018

svenefftinge commented Aug 14, 2018 • edited Loading

akosyakov commented Aug 14, 2018

tolusha commented Aug 14, 2018

paul-marechal commented Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tolusha commented Aug 15, 2018

tolusha commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov left a comment

Choose a reason for hiding this comment

tolusha commented Aug 21, 2018

paul-marechal commented Aug 21, 2018 • edited Loading

akosyakov commented Aug 21, 2018

paul-marechal left a comment •

edited

Loading

paul-marechal Jul 27, 2018 •

edited

Loading

paul-marechal Jul 27, 2018 •

edited

Loading

tolusha commented Jul 30, 2018 •

edited

Loading

svenefftinge commented Aug 10, 2018 •

edited

Loading

svenefftinge commented Aug 14, 2018 •

edited

Loading

paul-marechal commented Aug 14, 2018 •

edited

Loading

paul-marechal commented Aug 21, 2018 •

edited

Loading