Adding support for Remote .NET Attach and tsc fix on Windows #798

WardenGnaw · 2016-10-03T19:41:56Z

Feature:
package.json adds listRemoteProcesses as a command, including debuggerPath into pipeTransport. The
command is registered into commands.ts. processPicker.ts has its process listing parser refactored
and now includes calling process listing to remote processes. Only works on Linux and OSX. Windows
remote attach will result in a rejected promise.

Fix:
Fixing tsc error TS1252 in platform.ts.
Moved the getValue function within getCurrentPlatform function outside of function scope. Added string array parameter to getValue function because lines was originally a local variable within the getCurrentPlatform function.

Moved the getValue function within getCurrentPlatform function outside of function scope. Added string array parameter to getValue function because lines was originally a local variable within the getCurrentPlatform function.

package.json adds listRemoteProcesses as a command, including debuggerPath into pipeTransport. The command is registered into commands.ts. processPicker.ts has its process listing parser refactored and now includes calling process listing to remote processes. Only works on Linux and OSX. Windows remote attach will result in a rejected promise.

dnfclas · 2016-10-03T19:42:00Z

Hi @WardenGnaw, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

dnfclas · 2016-10-03T19:47:40Z

@WardenGnaw, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

rajkumar42 · 2016-10-04T16:36:17Z

@edmunoz is added to the review. #Closed

Added checking for platformSpecificPipeTransportOptions and added more error checks. Required debuggerPath for attach.pipeTransport.

DustinCampbell · 2016-10-10T20:28:54Z

package.json

+      {
+        "command" : "csharp.listRemoteProcess",
+        "title" : "List processes on remote connection for attach",
+        "category": "CSharp"


I notice that this command and "csharp.listProcess" have a category of "CSharp", but chsarp.downloadDebugger" has a category of "Debug". Is that intentional? Also, should the category be "C#"?

This I am not to sure of, @rajkumar42 or @gregg-miskelly

I don't know what category is supposed to be for. You can see if the schema will tell you. @chuckries created the downladDebugger command, so he might know. Otherwise you can look at the VS Code source to see if that has the answer (http://github.com/Microsoft/VSCode)

category appears in the Command Palette:

Looking at https://github.com/Microsoft/vscode-node-debug/search?utf8=%E2%9C%93&q=pickNodeProcess, I wonder if there is a way to avoid declaring this command entirely since it shouldn't actually show up in the command well.

Removing it from the commands JSON prevents it from showing up in the command well but functionality is kept.

DustinCampbell · 2016-10-10T20:29:55Z

src/features/commands.ts

+    let d9 = vscode.commands.registerCommand('csharp.listRemoteProcess', (args) => RemoteAttachPicker.ShowAttachEntries(args));

-    return vscode.Disposable.from(d1, d2, d3, d4, d5, d6, d7, d8);
+    return vscode.Disposable.from(d1, d2, d3, d4, d5, d6, d7, d8, d9);


This will conflict with #795. First one merged wins! 😄

I lost. 😢

DustinCampbell · 2016-10-10T20:34:55Z

src/features/processPicker.ts

+            // Config name not found. 
+            return new Promise<string>((resolve, reject) => {
+                reject(new Error("Name not defined in current configuration."));
+            });


This could be written simpler as

return Promise.reject<string>(new Error("Name not defined in current configuration"));

Should the error message mention the name that was passed in?

This is the case where name is null.

Ah of course. I was reading quickly. 😄

DustinCampbell · 2016-10-10T20:36:06Z

src/features/processPicker.ts

+        if (configIdx == json.configurations.length) {
+            // Name not found in list of given configurations. 
+            return new Promise<string>((resolve, reject) => {
+                reject(new Error("Could not find configuration that matches given name"));


Similar feedback to my comments above.

DustinCampbell · 2016-10-10T20:36:22Z

src/features/processPicker.ts

+            return new Promise<string>((resolve, reject) => {
+                reject(new Error("Configuration \"" + args.name + "\" in launch.json does not have a " + 
+                "pipeTransport argument with debuggerPath for pickRemoteProcess. Use pickProcess for local attach."));
+            });


Same here too.

DustinCampbell · 2016-10-10T20:43:30Z

src/features/processPicker.ts

+                if (remoteOS != "Linux" && remoteOS != "Darwin") {
+                    return new Promise<AttachItem[]>((resolve, reject) => {
+                        reject(new Error("Could not determine operating system or operating system not supported. " + remoteOS));
+                    }); 


return Promise.reject<AttackItem[]>(new Error(`Could not determine operating system or operating system not supported. ${remoteOS}`));

DustinCampbell · 2016-10-10T20:44:01Z

src/features/processPicker.ts

+                if (lines.length == 1) {
+                    return new Promise<AttachItem[]>((resolve, reject) => {
+                        reject(new Error("Transport attach could not obtain processes list.")); 
+                    }); 


Same as above. You can just return Promise.reject.

DustinCampbell · 2016-10-10T20:47:01Z

src/features/processPicker.ts

+
+    public static getRemoteProcesses(pipeCmd: string, os: string) : Promise<AttachItem[]> {
+        const commColumnTitle = Array(PsOutputParser.secondColumnCharacters).join("a");
+        const psCommand = `ps -axww -o pid=,comm=${commColumnTitle},args=` + (os === 'darwin' ? ' -c' : '');


Also could be:

const psCommand = `ps -axww -o pid=,comm=${commColumnTitle},args=${(os === 'darwin' ? ' -c' : '')}`;

DustinCampbell · 2016-10-10T20:47:26Z

src/features/processPicker.ts

+            if (lines.length == 0) {
+                return new Promise<AttachItem[]>((resolve, reject) => {
+                    reject(new Error("Pipe transport failed to get OS and processes."));
+                });


Another spot to return Promise.reject.

DustinCampbell · 2016-10-10T20:51:26Z

src/features/processPicker.ts

+        // Note that comm on Linux systems is truncated to 16 characters:
+        // https://bugzilla.redhat.com/show_bug.cgi?id=429565
+        // Since 'args' contains the full path to the executable, even if truncated, searching will work as desired.
+        const psCommand = `ps -axww -o pid=,comm=${commColumnTitle},args=` + (os.platform() === 'darwin' ? ' -c' : '');


Is this code copied and pasted? Could it be shared?

Instead of creating Promise objects, used Promise.reject. Used template strings properly and formatting.

gregg-miskelly

We are almost there :). I tried to delete all the stale comments.

gregg-miskelly · 2016-10-10T23:59:30Z

src/features/processPicker.ts

+                pipeArgs = platformSpecificPipeTransportOptions.pipeArgs || pipeArgs;
+            }
+
+            let pipeCmd: string = pipeProgram + " " + pipeArgs.join(" ");


If pipeProgram contains spaces I believe you will find you need to quote it. If so, you should probably should do the same for pipeArgs also.

gregg-miskelly · 2016-10-11T00:08:56Z

src/features/processPicker.ts

+        const command = `uname && if [ $(uname) == "Linux" ] ; then ${RemoteAttachPicker.linuxPsCommand} ; elif [ $(uname) == "Darwin" ] ; ` +
+            `then ${RemoteAttachPicker.osxPsCommand}; fi`;
+
+        return execChildProcess(`${pipeCmd} "${command}"`, null).then(output => {


I added this comment earlier, but to a spot where it got lost -- currently we will just reject the promise if the command returns stderror text. This might be confusing now that we are using execChildProcess for the pipe program. I would suggest doing something like putting in the wrong ip address in your plink or ssh command line and see if the experience is compressible.

After 15 seconds, it results in a "Command failed" error with the important bits cut off. Most of the output is the pipe command and arguments.

gregg-miskelly · 2016-10-11T00:11:30Z

src/features/processPicker.ts

+                let remoteOS = lines[0].replace(/[\r\n]+/g, '');
+
+                if (remoteOS != "Linux" && remoteOS != "Darwin") {
+                    return Promise.reject<AttachItem[]>(new Error("Could not determine operating system " +


Doesn't this always been the OS is unsupported?

Adding output channel for pipe transport errors. Fixing error messages and escaping pipe transport commands.

gregg-miskelly · 2016-10-11T01:24:49Z

src/features/processPicker.ts

+        // Create remote attach output channel for errors.
+        if (!RemoteAttachPicker._channel) {
+            RemoteAttachPicker._channel = vscode.window.createOutputChannel('remote-attach');
+        }


I think you should probably clear the channel if it already exists

gregg-miskelly · 2016-10-11T01:25:19Z

src/features/processPicker.ts

+}
+
+function execChildProcessAndOutputErrorToChannel(process: string, workingDirectory: string, channel: vscode.OutputChannel): Promise<string> {
+    return new Promise<string>((resolve, reject) => {


Might want to write out the command be executed to the channel for easy debugging.

gregg-miskelly · 2016-10-11T01:27:08Z

src/features/processPicker.ts

+                return;
+            }
+
+            if (stderr && stderr.length > 0) {


I would probably suggest keeping all the stdout and stderr text in a buffer so that on an error you can dump it all.

Something like this? Using the channel as the buffer.

if (stdout && stdout.length > 0) { channel.appendLine('stdout:'); channel.append(stdout); } if (stderr && stderr.length > 0) { channel.appendLine('stderr:'); channel.append(stderr); } if (error) { channel.appendLine('Error message:'); channel.append(error.message); } if (error || (stderr && stderr.length > 0)) { reject(new Error("See remote-attach output")); channel.show(); return; } resolve(stdout);

Yes, except that you probably don't want to send it to the channel in the success case. So I would suggest keeping an internal string buffer until there is an error. You probably don't need the stderr/stdout prefix -- if something is an error or normal output is usually apparent to humans by reading the text.

gregg-miskelly · 2016-10-11T01:28:03Z

src/features/processPicker.ts

+            }
+
+            if (stderr && stderr.length > 0) {
+                reject(new Error("See remote-attach output"));


We might want to wait till the command finishes in case there are multiple error lines.

I'm confused about how to do this. I thought this would be part of the callback function after it finishes the exec.

I think you just keep state as to if you saw an error and then you can reject the promise at the end if there was an error.

Fixing error handling on exec calls, and clearing channel on remoteProcessPicker startup.

gregg-miskelly · 2016-11-12T00:50:32Z

This PR has been replaced with #918

asyschikov · 2016-11-21T12:33:36Z

@WardenGnaw Is there any documentation on remote debugging, including how to use pipeTransport?

gregg-miskelly · 2016-11-21T15:43:06Z

@asyschikov I wrote up https://github.com/OmniSharp/omnisharp-vscode/wiki/Attaching-to-remote-processes. Let me know if you need more information/find problems.

asyschikov · 2016-11-22T11:36:41Z

@gregg-miskelly looks good thanks. Is debugging (locally) over SSH advantageous in any way?

gregg-miskelly · 2016-11-22T15:45:47Z

@asyschikov no - I wouldn't expect it to make a big difference, but it would add a bit of extra latency in every interaction between the UI and the backend.

WardenGnaw added 4 commits September 29, 2016 13:00

Fixing tsc error TS1252 in platform.ts.

fabd9e7

Moved the getValue function within getCurrentPlatform function outside of function scope. Added string array parameter to getValue function because lines was originally a local variable within the getCurrentPlatform function.

Putting back accidentally deleted lines

c35c116

Matching style

72a3dea

dnfclas added the cla-required label Oct 3, 2016

dnfclas added cla-signed and removed cla-required labels Oct 3, 2016

Reverting platform.ts and fixing code review issues

f81f8c2

WardenGnaw added 3 commits October 5, 2016 13:04

Updating processPicker RemoteAttach to chain pipe commands

39e6649

Merge remote-tracking branch 'upstream/master'

479750b

Fixing code review issues

9cb8f0f

Added checking for platformSpecificPipeTransportOptions and added more error checks. Required debuggerPath for attach.pipeTransport.

DustinCampbell requested changes Oct 10, 2016

View reviewed changes

Fixed code review issues: errors & formatting

a9102a4

Instead of creating Promise objects, used Promise.reject. Used template strings properly and formatting.

gregg-miskelly reviewed Oct 11, 2016

View reviewed changes

More code review fixes.

ee1ad71

Adding output channel for pipe transport errors. Fixing error messages and escaping pipe transport commands.

gregg-miskelly reviewed Oct 11, 2016

View reviewed changes

WardenGnaw added 3 commits October 11, 2016 10:52

Merge remote-tracking branch 'upstream/master'

cf53b64

More code review fixes, again.

9adb2dc

Fixing error handling on exec calls, and clearing channel on remoteProcessPicker startup.

Merge remote-tracking branch 'upstream/master'

83659b5

gregg-miskelly mentioned this pull request Nov 11, 2016

Debugger should support pipeTransport for attach #602

Closed

gregg-miskelly closed this Nov 12, 2016

Adding support for Remote .NET Attach and tsc fix on Windows #798

Adding support for Remote .NET Attach and tsc fix on Windows #798

Uh oh!

Conversation

WardenGnaw commented Oct 3, 2016

Uh oh!

dnfclas commented Oct 3, 2016

Uh oh!

dnfclas commented Oct 3, 2016

Uh oh!

rajkumar42 commented Oct 4, 2016

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!

DustinCampbell Oct 10, 2016 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregg-miskelly Oct 11, 2016 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

DustinCampbell Oct 10, 2016 •

edited

Loading

gregg-miskelly Oct 11, 2016 •

edited

Loading