Add support for remote debugging with experimental debugger #1230

DonJayamanne · 2018-03-28T20:39:35Z

This pull request:

Has a title summarizes what is changing
Includes a news entry file (remember to thank yourself!)
Has unit tests & code coverage is not adversely affected (within reason)
Works on all actively maintained versions of Python (e.g. Python 2.7 & the latest Python 3 release)
Works on Windows 10, macOS, and Linux (e.g. considered file system case-sensitivity)

codecov · 2018-03-29T17:20:27Z

Codecov Report

Merging #1230 into master will increase coverage by 0.13%.
The diff coverage is 78.07%.

@@            Coverage Diff             @@
##           master    #1230      +/-   ##
==========================================
+ Coverage   70.94%   71.07%   +0.13%     
==========================================
  Files         266      267       +1     
  Lines       12284    12350      +66     
  Branches     2172     2188      +16     
==========================================
+ Hits         8715     8778      +63     
+ Misses       3445     3432      -13     
- Partials      124      140      +16

Impacted Files	Coverage Δ
src/client/debugger/Common/Contracts.ts	`100% <ø> (ø)`	⬆️
...rc/client/debugger/configProviders/baseProvider.ts	`93.97% <100%> (+0.92%)`	⬆️
...lient/debugger/configProviders/pythonV2Provider.ts	`100% <100%> (ø)`	⬆️
.../client/debugger/DebugClients/RemoteDebugClient.ts	`35% <33.33%> (+17.35%)`	⬆️
...lient/debugger/DebugServers/RemoteDebugServerv2.ts	`66.66% <66.66%> (ø)`
src/client/debugger/mainV2.ts	`76.03% <72.72%> (+4.41%)`	⬆️
src/client/debugger/PythonProcess.ts	`45.83% <0%> (-2.92%)`	⬇️
src/client/linters/lintingEngine.ts	`90.35% <0%> (-0.88%)`	⬇️
...rc/client/debugger/PythonProcessCallbackHandler.ts	`53.61% <0%> (+0.65%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 962c850...9bf271a. Read the comment docs.

MikhailArkhipov · 2018-04-04T16:44:31Z

src/client/debugger/DebugServers/RemoteDebugServerv2.ts

+// tslint:disable:quotemark ordered-imports no-any no-empty curly member-ordering one-line max-func-body-length no-var-self prefer-const cyclomatic-complexity prefer-template
+
+import { DebugSession } from "vscode-debugadapter";
+import { IPythonProcess, IDebugServer, AttachRequestArguments } from "../Common/Contracts";


Evil double quotes?

aaah, copy paste.

MikhailArkhipov · 2018-04-04T16:45:33Z

src/client/debugger/configProviders/baseProvider.ts

-        this.provideDefaults(workspaceFolder, config);
+        if (config.request === 'attach') {
+            // tslint:disable-next-line:no-any
+            this.provideAttachDefaults(workspaceFolder, config as any as PythonAttachDebugConfiguration);


Interesting - why is getting casted twice

config can be of two types. Modified code accordingly to make it clearer (using a union type).

MikhailArkhipov · 2018-04-04T16:47:11Z

src/client/debugger/mainV2.ts

@@ -87,14 +81,24 @@ export class PythonDebugger extends DebugSession {
        this.sendResponse(response);
    }
    protected attachRequest(response: DebugProtocol.AttachResponse, args: AttachRequestArguments): void {
-        this.sendResponse(response);
+        const launcher = CreateAttachDebugClient(args as AttachRequestArguments, this);


args seems to be of that type already

MikhailArkhipov · 2018-04-04T16:49:00Z

src/client/debugger/mainV2.ts

@@ -211,6 +230,7 @@ class DebugManager implements Disposable {

        this.initializeRequestDeferred = createDeferred<DebugProtocol.InitializeRequest>();
        this.launchRequestDeferred = createDeferred<DebugProtocol.LaunchRequest>();
+        this.attachRequestDeferred = createDeferred<DebugProtocol.LaunchRequest>();


DebugProtocol.AttachRequest?

MikhailArkhipov · 2018-04-04T16:57:35Z

src/client/debugger/configProviders/baseProvider.ts

        const numberOfSettings = Object.keys(config);
-        const workspaceFolder = this.getWorkspaceFolder(folder, config);
+        const workspaceFolder = this.getWorkspaceFolder(folder, config as PythonLaunchDebugConfiguration);


getWorkspaceFolder doesn't seem to be using config (it passes it to getProgram which does not use it.

MikhailArkhipov · 2018-04-04T17:00:26Z

src/client/debugger/configProviders/baseProvider.ts


 // tslint:disable:no-invalid-template-strings

-export type PythonDebugConfiguration = DebugConfiguration & LaunchRequestArguments;
+export type PythonLaunchDebugConfiguration = DebugConfiguration & LaunchRequestArguments;
+export type PythonAttachDebugConfiguration = DebugConfiguration & AttachRequestArguments;

 @injectable()
 export abstract class BaseConfigurationProvider implements DebugConfigurationProvider {


Might be worth splitting into AttachConfigurationProvider and LaunchConfigurationProvider and have static factory like getConfigurationProvider(mode). Class has bunch of getLaunchX and getAttachX and casts and checks for the debug mode.

Unfortunately there can only be one debug configuration, that handles both launch and attach (& more in the future).
I've cleaned up the code,

There could be one class that contains a handler specific to the mode. I.e. debug configuration class acts as a proxy and simply forwards calls to 'launch or 'attach handler

MikhailArkhipov · 2018-04-04T17:09:29Z

src/client/debugger/mainV2.ts

@@ -263,7 +283,7 @@ class DebugManager implements Disposable {
            this.terminatedEventSent = true;
        }

-        if (this.killPTVSDProcess && this.ptvsdProcessId) {
+        if (this.launchOrAttach === 'launch' && this.ptvsdProcessId) {


In big VS Stop Debugging terminates debuggee even on attach. There is separate Detach command if user wants to keep debuggee running. I guess VSC does it differently.

In VSC, we don't have the ability to kill for attach scenarios, at least not yet (the protocol supports it for VS), the UI doesn't.
I'll create an issue so we add support for this, this way if the UI does add support for this in VSC, it'll work without much changes.

On second thought, we won't need to do anything. There's a ticket item in PTVSD to support this functionality ( required by VS).
in reality PTVSD is the debug adapter, hence it will kill the process if it needs to (the stuff we have here, is a temporary bridge to connect VSC to PTVSD - to perform initial handshake).
I.e. nothing to do here.

MikhailArkhipov · 2018-04-04T17:10:03Z

src/client/debugger/mainV2.ts

+        if (this.launchOrAttach === 'launch') {
+            const debugSoketProtocolParser = this.serviceContainer.get<IProtocolParser>(IProtocolParser);
+            debugSoketProtocolParser.connect(this.ptvsdSocket);
+            debugSoketProtocolParser.once('event_process', (proc: DebugProtocol.ProcessEvent) => {


MikhailArkhipov · 2018-04-04T17:11:17Z

src/test/.vscode/settings.json

@@ -17,6 +17,5 @@
    "python.linting.pylamaEnabled": false,
    "python.linting.mypyEnabled": false,
    "python.formatting.provider": "yapf",
-    "python.linting.pylintUseMinimalCheckers": false,
-    "python.pythonPath": "python"
+    "python.linting.pylintUseMinimalCheckers": false


Intentional?

DonJayamanne changed the title ~~WIP - Add support for remote debugging with experimental debugger~~ Add support for remote debugging with experimental debugger Mar 29, 2018

DonJayamanne force-pushed the issueAttachExperimental branch 2 times, most recently from fcf15dd to 673ef14 Compare April 2, 2018 21:46

DonJayamanne added 11 commits April 2, 2018 20:13

split compilation for faster compilation on windows

2fd50a7

✨ experimental debugger attach

b9d6c0b

🔨 refactor

4a1dcb0

✅ tests for experimental debugger attach

5d6223c

🔨 increase timeout of this test

15249b3

📝 change log [skip ci]

febe297

increase timeout for tests

35afa54

add templates for attaching

c24183b

change label [skip ci]

3275dd5

chane type [skip ci]

0e5f0f6

improvements tests (temporary workaround due to PTVSD issues)

9032acf

DonJayamanne force-pushed the issueAttachExperimental branch from 673ef14 to 9032acf Compare April 3, 2018 03:13

DonJayamanne added 2 commits April 2, 2018 20:46

🔨 connect vsc to ptvsd early on

4f855f0

🔨 use utils

08dc364

MikhailArkhipov reviewed Apr 4, 2018

View reviewed changes

DonJayamanne added 3 commits April 4, 2018 09:50

code review changes

39dcd34

🔨 fix typo

22d710d

🔨 fix code review comments

5c1dcfb

MikhailArkhipov reviewed Apr 4, 2018

View reviewed changes

fixed code review comments

8ec6576

MikhailArkhipov reviewed Apr 4, 2018

View reviewed changes

Further clean up

80b1903

MikhailArkhipov approved these changes Apr 4, 2018

View reviewed changes

DonJayamanne added 2 commits April 4, 2018 10:17

🔨 rename based on code review

10297ef

revert changes

9bf271a

DonJayamanne merged commit 3e40ce2 into microsoft:master Apr 4, 2018

lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for remote debugging with experimental debugger #1230

Add support for remote debugging with experimental debugger #1230

DonJayamanne commented Mar 28, 2018 •

edited

Loading

codecov bot commented Mar 29, 2018 •

edited

Loading

MikhailArkhipov Apr 4, 2018

DonJayamanne Apr 4, 2018

MikhailArkhipov Apr 4, 2018

DonJayamanne Apr 4, 2018

MikhailArkhipov Apr 4, 2018

DonJayamanne Apr 4, 2018

MikhailArkhipov Apr 4, 2018

DonJayamanne Apr 4, 2018

MikhailArkhipov Apr 4, 2018

DonJayamanne Apr 4, 2018

MikhailArkhipov Apr 4, 2018

DonJayamanne Apr 4, 2018

MikhailArkhipov Apr 4, 2018

DonJayamanne Apr 4, 2018

MikhailArkhipov Apr 4, 2018

DonJayamanne Apr 4, 2018

DonJayamanne Apr 4, 2018

MikhailArkhipov Apr 4, 2018

DonJayamanne Apr 4, 2018

MikhailArkhipov Apr 4, 2018

DonJayamanne Apr 4, 2018

Add support for remote debugging with experimental debugger #1230

Add support for remote debugging with experimental debugger #1230

Conversation

DonJayamanne commented Mar 28, 2018 • edited Loading

codecov bot commented Mar 29, 2018 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonJayamanne commented Mar 28, 2018 •

edited

Loading

codecov bot commented Mar 29, 2018 •

edited

Loading