Add support for remote debugging with experimental debugger by DonJayamanne · Pull Request #1230 · microsoft/vscode-python

Don Jayamanne (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.

Mikhail Arkhipov (MikhailArkhipov) · 2018-04-04T16:44:31Z

+// 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.

Mikhail Arkhipov (MikhailArkhipov) · 2018-04-04T16:45:33Z

-        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).

Mikhail Arkhipov (MikhailArkhipov) · 2018-04-04T16:47:11Z

    }
    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

Mikhail Arkhipov (MikhailArkhipov) · 2018-04-04T16:49:00Z


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


DebugProtocol.AttachRequest?

Mikhail Arkhipov (MikhailArkhipov) · 2018-04-04T16:57:35Z

+        const config = debugConfiguration as PythonLaunchDebugConfiguration | PythonAttachDebugConfiguration;
        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.

Mikhail Arkhipov (MikhailArkhipov) · 2018-04-04T17:00:26Z

+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

Mikhail Arkhipov (MikhailArkhipov) · 2018-04-04T17:09:29Z

        }

-        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.

Mikhail Arkhipov (MikhailArkhipov) · 2018-04-04T17:10:03Z

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


Mikhail Arkhipov (MikhailArkhipov) · 2018-04-04T17:11:17Z

    "python.formatting.provider": "yapf",
-    "python.linting.pylintUseMinimalCheckers": false,
-    "python.pythonPath": "python"
+    "python.linting.pylintUseMinimalCheckers": false


Intentional?

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

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

Don Jayamanne (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

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

Don Jayamanne (DonJayamanne) added 2 commits April 2, 2018 20:46

🔨 connect vsc to ptvsd early on

4f855f0

🔨 use utils

08dc364

Mikhail Arkhipov (MikhailArkhipov) reviewed Apr 4, 2018

View reviewed changes

Don Jayamanne (DonJayamanne) added 3 commits April 4, 2018 09:50

code review changes

39dcd34

🔨 fix typo

22d710d

🔨 fix code review comments

5c1dcfb

Mikhail Arkhipov (MikhailArkhipov) reviewed Apr 4, 2018

View reviewed changes

fixed code review comments

8ec6576

Mikhail Arkhipov (MikhailArkhipov) reviewed Apr 4, 2018

View reviewed changes

Further clean up

80b1903

Mikhail Arkhipov (MikhailArkhipov) approved these changes Apr 4, 2018

View reviewed changes

Don Jayamanne (DonJayamanne) added 2 commits April 4, 2018 10:17

🔨 rename based on code review

10297ef

revert changes

9bf271a

Don Jayamanne (DonJayamanne) merged commit 3e40ce2 into microsoft:master Apr 4, 2018

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

Conversation

Don Jayamanne (DonJayamanne) commented Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Don Jayamanne (DonJayamanne) commented Mar 28, 2018 •

edited

Loading

codecov Bot commented Mar 29, 2018 •

edited

Loading