New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Draft debug adapter #160

Merged
merged 2 commits into from Apr 26, 2018

Conversation

Projects
None yet
6 participants
@radu-matei
Member

radu-matei commented Apr 3, 2018

This PR is a work in progress to add native VS Code experience to run and debug apps deployed with Draft.

It is still rough around the edges and there's lots of things to add and review:

  • check if Draft is installed before starting debug sessions (solved by #136)
  • prompt to install Draft if draft.toml or specific launch file is detected ( solved by #136)
  • draftRuntime.ts needs a complete rewrite
  • a way to ensure the debugger is only attached after the connection is actually active (and not just the port is used)
  • add the actual debug configuration as part of launch.json
  • make sure the Draft cycle is not started when not using Draft
  • find way to stop debugging session if Draft is not present or folder doesn't contain a Draft app
  • fix tests
  • add specific way of checking connection is ready based on the debugger type (fixed for NodeJS)
  • decide on adding an attach target that only attaches to existing deployments
  • add timeout in waiting to attach the debugger

This is a first attempt at integrating with the extension - please provide all types of feedback on debug sessions and how the debugger integrates with the extension.

@bacongobbler

This comment has been minimized.

Member

bacongobbler commented Apr 4, 2018

Hey! I had a few bits of feedback on this PR:

check if Draft is installed before starting debug sessions
prompt to install Draft if draft.toml or specific launch file is detected

Are these both solved via #136?

make sure the Draft cycle is not started when not using Draft

Can you elaborate a little more on this point? Does this mean that we only enable "the Draft debugging experience" only when launch.json specifies the "draft" target?

@radu-matei

This comment has been minimized.

Member

radu-matei commented Apr 5, 2018

Hi, @bacongobbler!
Yes, #136 should fix both of those problems (I updated the checklist accordingly) - thanks for the heads-up, I wasn't aware of that PR 😄

Regarding your second point yes - there should be a check so that if you add a correct launch.json (with draft as the target), but there is no draft.toml in your folder, it should notify you about it, not silently fail (as the current behavior).

@bnookala bnookala requested review from testforstephen and bnookala Apr 7, 2018

@kyliel

This comment has been minimized.

kyliel commented Apr 9, 2018

@akaroml @testforstephen , Rome and Jinbo, could you please help review debug scenarios? Thank you.

"program": "./out/src/draft/debugAdapter.js",
"runtime": "node",
"configurationAttributes": {
"launch": {}

This comment has been minimized.

@testforstephen

testforstephen Apr 10, 2018

Collaborator

Need add scheme description for each configure field, it's very helpful for intellisense when editing launch.json manually. See the java debug sample https://github.com/Microsoft/vscode-java-debug/blob/master/package.json#L56

}
}
],
"extensions": []

This comment has been minimized.

@testforstephen

testforstephen Apr 10, 2018

Collaborator

what is the purpose of this field? extensions seems not a standard configuration item supported in vscode package.json scheme.

// issue setting localRoot to worspaceFolder in nested config
//
// see https://github.com/Microsoft/vscode-node-debug/issues/178
cfg['localRoot'] = vscode.workspace.rootPath;

This comment has been minimized.

@testforstephen

testforstephen Apr 10, 2018

Collaborator

vscode has supported multi-root feature, and vscode.workspace.rootPath is deprecated.

In the debug configuration provider resolveDebugConfiguration(folder), the first argument folder represents the active workspace and suggest using that as workspace root path.

This comment has been minimized.

@radu-matei

radu-matei Apr 10, 2018

Member

This is true. However, the reason I'm currently hardcoding that value is because you would usually use ${workspaceFolder} - however, that is not available in nested configurations.

The workaround for this seems to: have the active workspace as the default value and check if the user overwrote it - but you have to use the absolute path it seems.

Nice catch on the multi-root!

@@ -62,6 +63,9 @@ const createClusterUI = createCluster.uiProvider();
const clusterProviderRegistry = clusterproviderregistry.get();
const git = new Git(shell);
const EMBED_DEBUG_ADAPTER: boolean = true;

This comment has been minimized.

@testforstephen

testforstephen Apr 10, 2018

Collaborator

What's the purpose of this variable EMBED_DEBUG_ADAPTER? don't get it.

This comment has been minimized.

@radu-matei

radu-matei Apr 10, 2018

Member

Nice catch, will remove it - it is used for debugging the extension itself.

const { Subject } = require('await-notify');
export class DraftDebugSession extends LoggingDebugSession {

This comment has been minimized.

@testforstephen

testforstephen Apr 10, 2018

Collaborator

From the implementation, every F5 will create two debug sessions, one is draft, another is the real logic application debug session. Feel a little weird. Is the draft debug session necessary?

This comment has been minimized.

@radu-matei

radu-matei Apr 10, 2018

Member

I haven't found a way to intercept the F5 event and execute commands before attaching the debugger - the workaround here would be to stop all debugging sessions when the actual debugger is detached - however, I haven't found a way to do that.

This is where I intercept the detaching of the actual debugger - https://github.com/radu-matei/vscode-kubernetes-tools/blob/afeba57a0d3cbedcf00a15831e94fe7c128a68a3/src/draft/draftRuntime.ts#L57

But I've yet to find a way to stop the other debugging session as well.

The Draft session is needed to enable the file watcher feature - while the Draft debug session is active, we watch on the file system and re-deploy the application on every save.

This comment has been minimized.

@testforstephen

testforstephen Apr 23, 2018

Collaborator

DraftConfigurationProvider#resolveDebugConfiguration is actually the recommended way to intercept F5. Inside the function, you can run draft command to prepare the environment, and then invoke startDebugging API to launch third-party debug session like node/java/go, but let resolveDebugConfiguration function itself return undefined. If its return value is undefined, VS Code will not launch the second debug session.

The second way is to take advantage of preLaunchTask. It's designed to run build and deploy before launching debug session.

This comment has been minimized.

@radu-matei

radu-matei Apr 23, 2018

Member

@testforstephen For now I think we need the Draft session to support the file watcher functionality. For the next iteration I'm looking into not having to press the stop debug two times.

@squillace

This comment has been minimized.

Contributor

squillace commented Apr 22, 2018

@radu-matei whenever you get a chance to rebase... want this in. :-) got another few to follow! @testforstephen have you had answers to your review? We will iterate as we go forward. Specifically to your #160 (comment), we need to identify how VS code needs to hand off "gracefully" or at least provide a pre-debugger hook to make this all one debugger. But I'm not clear that this is a blocker for this release. One step at a time.

@radu-matei radu-matei force-pushed the radu-matei:draft-debug-adapter branch 3 times, most recently from c460dbf to 139cb46 Apr 23, 2018

Add draft debug adapter
Add initial Draft runtime

Add initial Draft debug session

Add Draft debug adapter and configuration provider

Integrate debugger with extension

Create correct terminal for Windows and Unix

Cleanup on semi-colons

Add shelljs typings

Fix race condition in attaching node debugger

Add debug config as part of launch.json

Workaround for localRoot path problem

Cleanup and kill connect process on save

Add NodeJS and Golang preconfigured debug configuration

Add check before attaching the debugger based on debugging type, for NodeJS and Golang

Fix file watcher for Draft debug

Change check from null to undefined

Remove unused config

More cleanup

Even more cleanup

Add info about debug parameters

@radu-matei radu-matei force-pushed the radu-matei:draft-debug-adapter branch from 139cb46 to ad092a3 Apr 23, 2018

@radu-matei

This comment has been minimized.

Member

radu-matei commented Apr 23, 2018

This is up for a round of reviews.

Thanks!

@radu-matei

This comment has been minimized.

Member

radu-matei commented Apr 24, 2018

Pushed a workaround for having to stop two debug sessions - at the moment we still need the Draft debug session for the file watcher, but when you stop the actual debug session, both of them are stopped.

@radu-matei

This comment has been minimized.

Member

radu-matei commented Apr 25, 2018

To test:

  • git clone https://github.com/radu-matei/angular-todo-app

  • open node-backend in VS Code

  • F5 should start draft up and draft connect, then attach the debugger

  • you can add breakpoints and step through the code

  • you can change and save a file - this will trigger the cycle again

  • stop the debug session

@itowlson

I don't feel very qualified to review this! But I do have a few picky comments...

@@ -0,0 +1,3 @@
import { DraftDebugSession } from "./debugDebug";

This comment has been minimized.

@itowlson

itowlson Apr 26, 2018

Member

Can we come up with a better file name than debugDebug?

This comment has been minimized.

@squillace

squillace Apr 26, 2018

Contributor

Well, we could use debug^2

export class DraftDebugSession extends LoggingDebugSession {
private _runtime: DraftRuntime;

This comment has been minimized.

@itowlson

itowlson Apr 26, 2018

Member

Should this and the next field be readonly?

private _runtime: DraftRuntime;
private _configurationDone = new Subject();
public config: vscode.DebugConfiguration;

This comment has been minimized.

@itowlson

itowlson Apr 26, 2018

Member

Not enthusiastic about public mutable fields...

This comment has been minimized.

@squillace

squillace Apr 26, 2018

Contributor

is there a reason it's mutable?

@@ -0,0 +1,30 @@
import { WorkspaceFolder, DebugConfiguration, CancellationToken, ProviderResult, DebugConfigurationProvider } from "vscode";
import * as Net from 'net';
import { DraftDebugSession } from "./debugDebug";

This comment has been minimized.

@itowlson

itowlson Apr 26, 2018

Member

Super picky: inconsistent quoting styles on imports (e.g. "vscode" vs. 'net').

This comment has been minimized.

@squillace

squillace Apr 26, 2018

Contributor

nope; pick is as pick does. consistency solves some strange problem that may occur.

export class DraftConfigurationProvider implements DebugConfigurationProvider {
private _server?: Net.Server;
constructor() {

This comment has been minimized.

@itowlson

itowlson Apr 26, 2018

Member

Is the empty constructor needed?

await waitForProcessToExit(createProcess('draft', ['up'], output));
// wait for `draft connect` to be ready
this._connectProccess = createProcess('draft', ['connect'], output);

This comment has been minimized.

@itowlson

itowlson Apr 26, 2018

Member

Looks like this assumes draft is on the system path. We also support having a configuration setting that specifies where the draft binary is.

This comment has been minimized.

@squillace

squillace Apr 26, 2018

Contributor

that I would like supported.

this._connectProccess = createProcess('draft', ['connect'], output);
await waitConnectionReady(this._connectProccess, config);
host.showInformationMessage(`attaching debugger`);

This comment has been minimized.

@itowlson

itowlson Apr 26, 2018

Member

This message might appear after a bit of a delay, so it's good to provide a bit of extra context to let the user know what it refers to. Also, the all-lower-case is fine for diagnostic logging but doesn't look right for the UI - I think we should sentence-case UI messages.

This comment has been minimized.

@squillace

squillace Apr 26, 2018

Contributor

+1. Good application of typesetting.

function createProcess(cmd: string, args: string[], output: OutputChannel): ChildProcess {
// notify that cmd started
console.log(`started ${cmd} ${args.toString()}`);
host.showInformationMessage(`starting ${cmd} ${args.toString()}`);

This comment has been minimized.

@itowlson

itowlson Apr 26, 2018

Member

Does this require a notification? If so I'd suggest more context, and sentence-casing.

host.showInformationMessage(`starting ${cmd} ${args.toString()}`);
let proc = spawn(cmd, args, { cwd: vscode.workspace.rootPath });
console.log(process.env.PATH);

This comment has been minimized.

@itowlson

itowlson Apr 26, 2018

Member

Probably remove for release?

});
// if there is an active Draft debugging session, restart the cycle
if (draftDebugSession != undefined) {

This comment has been minimized.

@itowlson

itowlson Apr 26, 2018

Member

I don't quite get what's happening in this little dance of callbacks. Can we pull some of this code out into a separate class or module to make the relationships clearer, or at least better document what we're doing and why it's needed?

@itowlson

This comment has been minimized.

Member

itowlson commented Apr 26, 2018

We want to get this in ASAP so merging now and will address outstanding review comments (most critically the draft-path support) in hotfix PRs.

@itowlson itowlson merged commit 2000214 into Azure:master Apr 26, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment