Fix RemoteProcessPicker #1373

WardenGnaw · 2017-04-10T21:33:46Z

Allowing quoteArgs to handle the pipeProgram and pipeArgs quotes.

Moving the uname and ps command into a temp file so it can be redirected into the pipeProgram so the current shell does not mangle the command string.

Tested:
Windows -> Linux
Windows -> Docker Linux
Windows -> Mac
Mac -> Linux

Fixing configuration snippet formatting

Also moved list process command to be written to a temp file and redirected to the pipeProgram.

gregg-miskelly · 2017-04-10T21:45:07Z

src/features/processPicker.ts

+            pipeCmdList.push(pipeProgram);
+            pipeCmdList = pipeCmdList.concat(pipeArgs);
+
+            const scriptShellCmdList: string[] = ["bash", "-s"];


bash [](start = 51, length = 4)

I am not sure it really matters much for .NET, but should we use 'sh' instead of 'bash' to make it easier to port code between this extension and C++?

gregg-miskelly · 2017-04-10T21:51:22Z

src/features/processPicker.ts

+            pipeCmdList.push(pipeProgram);
+            pipeCmdList = pipeCmdList.concat(pipeArgs);
+
+            const scriptShellCmdList: string[] = ["bash", "-s"];


scriptShellCmdList [](start = 18, length = 18)

Maybe I am remembering wrong, but I think we are doing something different here than what vsdbg-ui does.

In vsdbg-ui, if quoteArgs is enabled the command line would be something like:

/mypipePath/myPipe -myPipeArg "/vsdbg/vsdbg --interpreter=vscode"

Here, it looks like we will wind up using:
/mypipePath/myPipe -myPipeArg /vsdbg/vsdbg --interpreter=vscode

And we would only quote an argument if one of the individual arguments contained a space.

I think instead you want:

let scriptShellCmd : string = "bash -s"; if (quoteArgs) { scriptShellCmd = `"${scriptShellCmd}"`; }

gregg-miskelly · 2017-04-10T21:53:36Z

src/features/processPicker.ts

-            let pipeCmd: string = `"${pipeProgram}" ${argList}`;
+            let pipeCmdList: string[] = [];
+            pipeCmdList.push(pipeProgram);
+            pipeCmdList = pipeCmdList.concat(pipeArgs);


pipeArgs [](start = 45, length = 8)

We need code to look for ${debuggerCommand}

gregg-miskelly · 2017-04-10T21:54:47Z

src/features/processPicker.ts

+
+        // Create a temp file to redirect commands to the pipeProgram to solve quoting issues.
+        const tempFile = tmp.fileSync();
+        fs.write(tempFile.fd, command);


Instead of creating a temp file with this, should we should stick this in a file in our extension?

Fixing syntax since = is for equality in sh. Also tested with Mac -> Linux w/ ZSH

gregg-miskelly · 2017-04-10T21:56:10Z

src/features/processPicker.ts

+
+        return execChildProcessAndOutputErrorToChannel(`${pipeCmd} < ${tempFile.name}`, null, RemoteAttachPicker._channel).then(output => {
+            // Remove temp file since the uname and ps commands have been executed.
+            tempFile.removeCallback();


tempFile [](start = 12, length = 8)

If we keep this, make sure to add a catch handler to clean it up in the case of failure as well.

gregg-miskelly

See comments

WardenGnaw · 2017-04-10T23:18:29Z

src/features/processPicker.ts

+
+            pipeCmdList = pipeCmdList.concat(scriptShellCmdList);
+
+            let pipeCmd: string = quoteArgs ? this.createArgumentList(pipeCmdList) : pipeCmdList.join(' ');


Should I modify createArgumentList to only insert quotes if it has a space in the string?

gregg-miskelly · 2017-04-10T23:19:35Z

scripts/remoteProcessPickerScript

@@ -0,0 +1 @@
+uname && if [ "$(uname)" = "Linux" ] ; then ps -axww -o pid=,comm=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,args= ; elif [ "$(uname)" = "Darwin" ] ; then ps -axww -o pid=,comm=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,args= -c; fi


Now that we don't need to have this as one big line, would it make sense to separate this out?

I believe there will be issues with \r\n vs \n if its separated on different lines.

You are probably right. Oh line endings...

gregg-miskelly · 2017-04-10T23:25:35Z

RE: Should I modify createArgumentList to only insert quotes if it has a space in the string?
A: I think so. The closer we can be to vsdbg-ui the better.

gregg-miskelly · 2017-04-10T23:20:17Z

scripts/remoteProcessPickerScript

@@ -0,0 +1 @@
+uname && if [ "$(uname)" = "Linux" ] ; then ps -axww -o pid=,comm=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,args= ; elif [ "$(uname)" = "Darwin" ] ; then ps -axww -o pid=,comm=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,args= -c; fi


Do we need a .gitattributes on this to make sure that it keeps Unix line endings?

I think you can 'Won't fix" this one since the script is a single line.

gregg-miskelly · 2017-04-10T23:22:43Z

src/features/processPicker.ts

+            pipeCmdList.push(pipeProgram);
+
+            // Remove debuggerCommand for remoteProcessPicker
+            pipeCmdList = pipeCmdList.concat(pipeArgs.filter(arg => arg !== "${debuggerCommand}"));


This doesn't work, as ${debuggerCommand} might be only part of one of the args, and because we want to preserve the order.

gregg-miskelly

Otherwise I think you have it.

gregg-miskelly · 2017-04-11T16:03:57Z

src/features/processPicker.ts

+            if (pipeArgs.filter(arg => arg === "${debuggerCommand}").length > 0) {
+                for (let arg of pipeArgs)
+                {
+                    if (arg === "${debuggerCommand}")


This should be:

while (arg.indexOf("${debuggerCommand}") >= 0) { arg = arg.replace("${debuggerCommand}", scriptShellCmd); }

gregg-miskelly · 2017-04-11T16:05:06Z

src/features/processPicker.ts

+            pipeCmdList.push(pipeProgram);

-            pipeCmdList.push(scriptShellCmd);
+            if (pipeArgs.filter(arg => arg === "${debuggerCommand}").length > 0) {


Replace: arg === "${debuggerCommand}"
With: arg.indexOf("${debuggerCommand}") >= 0

gregg-miskelly · 2017-04-11T17:29:26Z

src/features/processPicker.ts

+
+            const debuggerCommandString: string = "${debuggerCommand}";
+
+            if (pipeArgs.indexOf(debuggerCommandString) > 0) {


>= not >

And don't you still need your filter like you did before?

gregg-miskelly

LGTM

WardenGnaw added 2 commits April 10, 2017 13:41

Allowing quote args flag to handle pipeProgram

0a90ebd

Also moved list process command to be written to a temp file and redirected to the pipeProgram.

Fixing syntax

7f63bcf

WardenGnaw requested review from gregg-miskelly and rajkumar42 April 10, 2017 21:33

dnfclas added the cla-already-signed label Apr 10, 2017

gregg-miskelly reviewed Apr 10, 2017

View reviewed changes

Change bash to sh

3fc031b

Fixing syntax since = is for equality in sh. Also tested with Mac -> Linux w/ ZSH

gregg-miskelly reviewed Apr 10, 2017

View reviewed changes

gregg-miskelly requested changes Apr 10, 2017

View reviewed changes

Fixing CR issues

60cf495

WardenGnaw commented Apr 10, 2017

View reviewed changes

gregg-miskelly reviewed Apr 10, 2017

View reviewed changes

gregg-miskelly requested changes Apr 10, 2017

View reviewed changes

rajkumar42 approved these changes Apr 10, 2017

View reviewed changes

Handle debuggerCommand

10f4029

gregg-miskelly approved these changes Apr 11, 2017

View reviewed changes

Replacing all instances of debuggerCommand in str.

0f45030

gregg-miskelly reviewed Apr 11, 2017

View reviewed changes

Revert package.json and fix check for command

59d4bf2

gregg-miskelly approved these changes Apr 11, 2017

View reviewed changes

WardenGnaw merged commit 90c6d3b into dotnet:master Apr 11, 2017

WardenGnaw deleted the dev/waan/fixremoteprocesspick branch April 11, 2017 17:45

WardenGnaw mentioned this pull request Apr 11, 2017

Instructions on remote debugging with Docker? #1369

Closed


		pipeCmdList = pipeCmdList.concat(scriptShellCmdList);

		let pipeCmd: string = quoteArgs ? this.createArgumentList(pipeCmdList) : pipeCmdList.join(' ');

		@@ -0,0 +1 @@
		uname && if [ "$(uname)" = "Linux" ] ; then ps -axww -o pid=,comm=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,args= ; elif [ "$(uname)" = "Darwin" ] ; then ps -axww -o pid=,comm=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,args= -c; fi No newline at end of file


		const debuggerCommandString: string = "${debuggerCommand}";

		if (pipeArgs.indexOf(debuggerCommandString) > 0) {

Fix RemoteProcessPicker #1373

Fix RemoteProcessPicker #1373

Uh oh!

Conversation

WardenGnaw commented Apr 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregg-miskelly Apr 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

gregg-miskelly left a comment

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!

gregg-miskelly commented Apr 10, 2017

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!

gregg-miskelly left a comment

Choose a reason for hiding this comment

Uh oh!

gregg-miskelly Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

gregg-miskelly left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gregg-miskelly Apr 10, 2017 •

edited

Loading

gregg-miskelly Apr 11, 2017 •

edited

Loading