Plugin API for debug #3804

tolusha · 2018-12-11T09:33:30Z

This PR provides implementation of plugin API for debug

How to test

Download c++ extension into ~/theia-plugins folder.
THEIA_DEFAULT_PLUGINS="local-dir:///home/$USER/theia-plugins"
Start Theia and wait until c++ extension is activated
It is possible to work with c++ debugger

Prepare test file

compile your test file with -g option, for instance: g++ -g test.c++
modify debug configuration to set path to resulted program: "program": "${workspaceFolder}/a.out"
add breakpoints or modify configuration file "stopAtEntry": true

Know issues

1 . Debug adapter sends events in such sequence: 'exited', 'terminated', 'telemetry'.
Since we dispose `debug-session-connection' on 'exited' event we can see some traces in console:

root ERROR [hosted-plugin: 18775] (node:18775) UnhandledPromiseRejectionWarning: Error: the debug session connection is disposed, id: b886d0ce-4622-4a09-a01e-7bd82f55181c
    at DebugSessionConnection.push.../../packages/debug/lib/browser/debug-session-connection.js.DebugSessionConnection.checkDisposed (http://localhost:3000/28.bundle.js:2251:19)
    at DebugSessionConnection.push.../../packages/debug/lib/browser/debug-session-connection.js.DebugSessionConnection.newEmitter (http://localhost:3000/28.bundle.js:2447:14)
    at DebugSessionConnection.push.../../packages/debug/lib/browser/debug-session-connection.js.DebugSessionConnection.getEmitter (http://localhost:3000/28.bundle.js:2441:55)
    at DebugSessionConnection.push.../../packages/debug/lib/browser/debug-session-connection.js.DebugSessionConnection.doFire (http://localhost:3000/28.bundle.js:2438:14)
    at DebugSessionConnection.push.../../packages/debug/lib/browser/debug-session-connection.js.DebugSessionConnection.handleEvent (http://localhost:3000/28.bundle.js:2421:22)
    at DebugSessionConnection.push.../../packages/debug/lib/browser/debug-session-connection.js.DebugSessionConnection.handleMessage (http://localhost:3000/28.bundle.js:2368:18)
    at PluginMessageReader.callback (http://localhost:3000/28.bundle.js:2273:79)
    at PluginMessageReader.push.../../packages/plugin-ext/lib/common/plugin-message-reader.js.PluginMessageReader.readMessage (http://localhost:3000/74.bundle.js:2391:18)
    at ConnectionMainImpl.<anonymous> (http://localhost:3000/74.bundle.js:4539:53)
    at step (http://localhost:3000/74.bundle.js:4510:23)

benoitf · 2018-12-11T09:43:15Z

.vscode/launch.json

@@ -4,6 +4,7 @@
    // For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387
    "version": "0.2.0",
    "configurations": [
+


remove this useless change ?

benoitf · 2018-12-11T09:47:48Z

packages/debug/src/node/vscode/vscode-debug-adapter-contribution.ts

@@ -97,7 +96,7 @@ export abstract class AbstractVSCodeDebugAdapterContribution implements DebugAda
    }

    protected async parse(): Promise<VSCodeExtensionPackage> {
-        let text = (await fs.readFile(this.pckPath)).toString();
+        let text = (await fs.readFileSync(this.pckPath)).toString();


hello, quick question: why fs-extra/readFile has been replaced by fs/readFileSync ?

benoitf · 2018-12-11T09:51:48Z

packages/plugin-ext/src/hosted/browser/worker/debug-stub.ts

+    }
+
+    get onDidReceiveDebugSessionCustomEvent(): theia.Event<theia.DebugSessionCustomEvent> {
+        throw new Error('Debug API works only in plugin container');


maybe call a method that throw this message everywhere ? or to use a proxy instead as all methods are throwing errors ?

What are the benefits?

if new methods are added, then proxy will always be up-to-date and it's avoid 100 exact same lines

benoitf · 2018-12-11T09:54:01Z

packages/plugin-ext/src/plugin/command-registry.ts

    constructor(rpc: RPCProtocol) {
        this.proxy = rpc.getProxy(Ext.COMMAND_REGISTRY_MAIN);
        this.converter = new CommandsConverter(this);
+
+        // register internal VS Code commands


c++ extension use this command to show release notes.
We need to register this command at least with fake handler.
If you have any suggestion you are welcome.

benoitf · 2018-12-11T09:54:14Z

packages/plugin-ext/src/plugin/command-registry.ts

@@ -50,6 +56,7 @@ export class CommandRegistryImpl implements CommandRegistryExt {
        this.proxy.$registerCommand(command);

        return Disposable.create(() => {
+            this.commands.delete(command.id);


We don't have to keep command if it is disposed.
some extension (c++ in particular) register/dispose/register the same commands.
If we don't delete command we will fail.

benoitf · 2018-12-11T10:02:02Z

packages/plugin/src/theia.d.ts

+		 * @param nameOrConfiguration Either the name of a debug or compound configuration or a [DebugConfiguration](#DebugConfiguration) object.
+		 * @return A thenable that resolves when debugging could be successfully started.
+		 */
+        export function startDebugging(folder: WorkspaceFolder | undefined, nameOrConfiguration: string | DebugConfiguration): Thenable<boolean>;


PromiseLike

benoitf · 2018-12-11T10:02:08Z

packages/plugin/src/theia.d.ts

+		 * @param key A string.
+		 * @param value A value. MUST not contain cyclic references.
+		 */
+        update(key: string, value: any): Thenable<void>;


PromiseLike

benoitf · 2018-12-11T10:02:15Z

packages/plugin-ext/src/hosted/browser/worker/debug-stub.ts

+        throw new Error('Debug API works only in plugin container');
+    }
+
+    startDebugging(folder: theia.WorkspaceFolder | undefined, nameOrConfiguration: string | theia.DebugConfiguration): Thenable<boolean> {


PromiseLike

benoitf · 2018-12-11T10:02:29Z

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

+        this.proxy.$removeBreakpoints(breakpoints);
+    }
+
+    startDebugging(folder: theia.WorkspaceFolder | undefined, nameOrConfiguration: string | theia.DebugConfiguration): Thenable<boolean> {


PromiseLike

benoitf · 2018-12-11T10:02:37Z

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

+        return value || defaultValue;
+    }
+
+    update(key: string, value: T): Thenable<void> {


PromiseLike

akosyakov · 2018-12-12T08:49:27Z

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

+/**
+ * Describes what plugin contribution has to provide for a client.
+ */
+export interface DebugPluginContributor {


debug extension should not have notion of plugins

plugins should be encapsulated into @theia/plugin-ext

akosyakov · 2018-12-12T08:53:44Z

packages/debug/src/node/debug-model.ts

 import { WebSocketChannel } from '@theia/core/lib/common/messaging/web-socket-channel';
 import { DebugConfiguration } from '../common/debug-configuration';
 import { IJSONSchema, IJSONSchemaSnippet } from '@theia/core/lib/common/json-schema';
+import { Disposable } from '@theia/core/lib/common/disposable';
+import { MaybePromise } from '@theia/core/src/common/types';


always import from lib

akosyakov · 2018-12-12T08:56:04Z

packages/debug-nodejs/src/node/node-debug-adapter-contribution.ts


 export const INSPECTOR_PORT_DEFAULT = 9229;
 export const LEGACY_PORT_DEFAULT = 5858;

 @injectable()
-export class NodeDebugAdapterContribution extends AbstractVSCodeDebugAdapterContribution {
+export class NodeDebugAdapterContribution implements DebugAdapterContribution {


Can you elaborate why it cannot extend VSCodeDebugAdapterContribution? the same for node2 and java

In case of extension I have to use inversify.
But in this case I can't use it in plugin-ext

I think VsCodePluginScanner should be improved to handle vscode extension localization generally and support debugger relevant contributions. The plugin extension then should use PluginModel to provide DebugAdapterContribution. Maybe some code should be copied there from VSCodeDebugAdapterContribution to achieve it.

@akosyakov resolved
2aa725a

akosyakov · 2018-12-12T08:59:24Z

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

+    protected readonly debugPreferences: DebugPreferences;
+
+    protected readonly onDidContributionAddEmitter = new Emitter<string>();
+    readonly onDidContributionAdd: Event<string> = this.onDidContributionAddEmitter.event;


the pattern is on[Will|Did]VerbNoun? - https://code.visualstudio.com/docs/extensionAPI/patterns-and-principles#_events

akosyakov · 2018-12-12T09:03:47Z

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

+ * Session factory for a client debug session that communicates with debug adapter contributed as plugin.
+ * The main difference is to use a connection factory that creates [IWebSocket](#IWebSocket) over Rpc channel.
+ */
+class PluginDebugSessionFactory implements DebugSessionFactory {


a class should go to its own file, a file name should be a class name

we try not to put several classes in one file with a random name, also we try to avoid classes with the same name in the repo

it is to easy navigation

akosyakov · 2018-12-12T09:19:16Z

It introduces implicit dependencies from the debug to the plugin extension. Dependencies should be only in on direction from the plugin to debug extension. The debug extension should not have notion of plugins or plugin contributions. The plugin extension can rebind and customize services from the debug extension to add this notion.

akosyakov · 2018-12-12T15:31:43Z

Functional-wise, after battling with g++/gdb I was able to run it eventually :) It works for me nicely and I did not find any breaking behavior changes.

akosyakov · 2018-12-13T07:14:23Z

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

+    }
+
+    async resolveDebugConfiguration(config: DebugConfiguration, workspaceFolderUri: string | undefined): Promise<DebugConfiguration> {
+        const contributor = this.pluginContributors.get(config.type);


it is bogus, all debug providers should participate in the resolution of any DebugConfiguration, regardless of whether they come from extensions or plugins. Like in DebugAdapterContributionRegistry.resolveDebugConfiguration. I think the whole integration should happen behind DebugService on the level of debug adapter contributions that the code in DebugService and DebugAdapterContributionRegistry is reused.

akosyakov · 2018-12-13T07:19:50Z

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

+    }
+}
+
+class DebugPluginContribution extends VSCodeDebugAdapterContribution {


it looks strange, plugins should not rely on VSCodeDebugAdapterContribution from the debug extension, but should read metadata from plugins, the same way as metadata for commands and menu items and so on, read by the plugin extension.

I hoped that this PR will fix #3771 (comment) by improving localization of metadata from VS Code extensions.

tolusha · 2018-12-14T10:12:11Z

@akosyakov I've addressed all your remarks.

svenefftinge

LGTM, whatever Anton's concern was can be fixed after merge as well. There is . a missing ,in plugin-api.ts and would be good if you could squash the commits.

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

tolusha added plug-in system issues related to the plug-in system debug issues that related to debug functionality labels Dec 11, 2018

tolusha self-assigned this Dec 11, 2018

tolusha requested review from svenefftinge, evidolob and akosyakov December 11, 2018 09:33

tolusha requested a review from benoitf as a code owner December 11, 2018 09:33

tolusha requested a review from tsmaeder December 11, 2018 09:34

tolusha force-pushed the ab/DebugPluginApi branch from 8d6d29b to d289124 Compare December 11, 2018 09:35

benoitf reviewed Dec 11, 2018

View reviewed changes

evidolob approved these changes Dec 12, 2018

View reviewed changes

akosyakov reviewed Dec 12, 2018

View reviewed changes

tolusha added dap debug adapter protocol vscode issues related to VSCode compatibility Team: Che-Languages issues regarding the che-languages team labels Dec 12, 2018

akosyakov reviewed Dec 13, 2018

View reviewed changes

tolusha force-pushed the ab/DebugPluginApi branch from 2aa725a to a805e75 Compare December 17, 2018 10:13

svenefftinge approved these changes Dec 19, 2018

View reviewed changes

tolusha force-pushed the ab/DebugPluginApi branch 4 times, most recently from 385dd3f to 45dfe07 Compare December 22, 2018 07:58

Plugin API for debug

c20b224

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

tolusha force-pushed the ab/DebugPluginApi branch from 6dd99d0 to c20b224 Compare December 22, 2018 09:35

tolusha merged commit 9cf1895 into master Dec 22, 2018

tolusha deleted the ab/DebugPluginApi branch December 22, 2018 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plugin API for debug #3804

Plugin API for debug #3804

tolusha commented Dec 11, 2018 •

edited

Loading

benoitf Dec 11, 2018

benoitf Dec 11, 2018

benoitf Dec 11, 2018

tolusha Dec 11, 2018

benoitf Dec 11, 2018

benoitf Dec 11, 2018

tolusha Dec 11, 2018

benoitf Dec 11, 2018

tolusha Dec 11, 2018

benoitf Dec 11, 2018

benoitf Dec 11, 2018

benoitf Dec 11, 2018

benoitf Dec 11, 2018

benoitf Dec 11, 2018

akosyakov Dec 12, 2018

akosyakov Dec 12, 2018

akosyakov Dec 12, 2018

tolusha Dec 13, 2018

akosyakov Dec 14, 2018 •

edited

Loading

tolusha Dec 17, 2018

akosyakov Dec 12, 2018

akosyakov Dec 12, 2018 •

edited

Loading

akosyakov commented Dec 12, 2018 •

edited

Loading

akosyakov commented Dec 12, 2018

akosyakov Dec 13, 2018 •

edited

Loading

akosyakov Dec 13, 2018 •

edited

Loading

tolusha commented Dec 14, 2018

svenefftinge left a comment

Plugin API for debug #3804

Plugin API for debug #3804

Conversation

tolusha commented Dec 11, 2018 • edited Loading

How to test

Prepare test file

Know issues

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov Dec 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

akosyakov Dec 12, 2018 • edited Loading

Choose a reason for hiding this comment

akosyakov commented Dec 12, 2018 • edited Loading

akosyakov commented Dec 12, 2018

akosyakov Dec 13, 2018 • edited Loading

Choose a reason for hiding this comment

akosyakov Dec 13, 2018 • edited Loading

Choose a reason for hiding this comment

tolusha commented Dec 14, 2018

svenefftinge left a comment

Choose a reason for hiding this comment

tolusha commented Dec 11, 2018 •

edited

Loading

akosyakov Dec 14, 2018 •

edited

Loading

akosyakov Dec 12, 2018 •

edited

Loading

akosyakov commented Dec 12, 2018 •

edited

Loading

akosyakov Dec 13, 2018 •

edited

Loading

akosyakov Dec 13, 2018 •

edited

Loading