Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Removing common parts from program and remotePath #742

Merged
merged 3 commits into from
Feb 6, 2017
Merged

Removing common parts from program and remotePath #742

merged 3 commits into from
Feb 6, 2017

Conversation

f0zi
Copy link
Contributor

@f0zi f0zi commented Jan 24, 2017

also returning '\' as a separator on Windows paths with mixed separators.

PR as requested in #689.

also returning '\' as a separator on Windows paths with mixes separators.
@msftclas
Copy link

Hi @f0zi, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. Real humans will now evaluate your PR.

TTYL, MSBOT;

let remotePath = args.remotePath || '';
let port = args.port || random(2000, 50000);
let host = args.host || '127.0.0.1';

if (remotePath.length > 0) {
this.localPathSeparator = args.program.includes('/') ? '/' : '\\';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails for the (for me) common case where I share a project (and it's launch.json) and open it on different platforms and having a setting like "program": "${workspaceRoot}/test" because ${workspaceRoot} has backslashes on Windows but slashes on Unix.

let llist = localPath.split(/\/|\\(?! )/).reverse();
let rlist = remotePath.split(/\/|\\(?! )/).reverse();
for(var i = 0; i < llist.length; i++) if (llist[i] !== rlist[i]) break;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This finds (and counts) common path elements. Think:
/home/user/go/src/foo/bar
C:\Users\user\gohome\src\foo\bar

if (i) {
localPath = llist.reverse().slice(0, -i).join(this.localPathSeparator);
remotePath = rlist.reverse().slice(0, -i).join(this.remotePathSeparator);
} else if ((remotePath.endsWith('\\')) || (remotePath.endsWith('/'))) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This removes the common elements found above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this, based on how we do it for node debugging. The program field isn't used for remote debugging, right? So instead of specifying the program, the user could specify the final 'localPath' to match 'remotePath'. Then they are responsible for doing this in their launch config - finding the common path of the two.

Would that end up being more confusing? I'm not opposed to this, just thinking about it.

Other debug adapters have 'launch'-type and 'attach'-type configs, where 'launch' is when it has to start executing the binary, and 'attach' is when it has already been started and is in debug mode. We could introduce that concept here too. Then the 'program' field would not be allowed for remote debug scenarios, just 'remotePath' and 'localPath' to provide the mapping.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think an alternative would be to use the local GOPATH and add a setting for the remote GOPATH. Everything after the $GOPATH\src\ could be used, however this solution makes some assumptions that are not always true (like that the code actually is in $GOPATH and that there is a /src/ folder in there, which can be false especially when remote debugging) while the current solution "just works".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not a bad idea either, but I don't want to make assumptions like that. I think that for now, we should keep the current approach with your changes.

remotePath = remotePath.substring(0, remotePath.length - 1);
}
}

this.delve = new Delve(args.mode, remotePath, port, host, args.program, args.args, args.showLog, args.cwd, args.env, args.buildFlags, args.init);
this.delve = new Delve(args.mode, remotePath, port, host, localPath, args.args, args.showLog, args.cwd, args.env, args.buildFlags, args.init);
this.delve.onstdout = (str: string) => {
this.sendEvent(new OutputEvent(str, 'stdout'));
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result is that you can now set breakpoints and debug libraries in e.g. /home/user/go/src/foo/baz even though your main package is in /home/user/go/src/foo/bar

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that true, since foo != bar? Isn't it like /home/foo1/go/src/bar will match /home/foo2/go/src/bar because the two paths will be trimmed to /home/foo1 and /home/foo2.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember this is initialization. The actual paths we get from remote or send to remote are the path arguments to toDebuggerPath and toLocalPath. Now we have four components:

  • The main module path, not visible here, but worth noting as it played a role in the old code because localPath was equal to main module path, e.g. C:\Users\foo\go\src\foo/bar. Note the mixed separators come from ${workspace}/bar. Launch.json is shared between Windows & Unix.
  • The localPath, now shortened to e.g. C:\Users\foo
  • The remotePath, shortened to e.g. /home/foo2
  • The argument to the function, e.g. /home/foo2/go/src/foo/baz for toLocalPath or (think set BP) C:\User\foo\go\src\foo\baz for toDebuggerPath.

The functions toLocalPath and toDebuggerPath just replace the beginning of the paths and change the path separators.

@ramya-rao-a
Copy link
Contributor

@roblourens Can you take a look at this?

@@ -321,21 +321,36 @@ class GoDebugSession extends DebugSession {
log('InitializeEvent');
}

protected findPathSeperator(path) {
if (/^(\w:[\\/]|\\\\)/.test(path)) return '\\';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you just return \ if the path contains a ?

Copy link
Contributor Author

@f0zi f0zi Feb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how that would help. Trying to match absolute Windows paths like C:\ or C:/ and share paths like \\server\share. It also matches \\?\, so it does not need it's own check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would just be simpler. But I guess if the path is like C:/foo/bar you still want to return a backslash

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also for \\server\share

this.remotePathSeparator = this.findPathSeperator(remotePath);

let llist = localPath.split(/\/|\\(?! )/).reverse();
let rlist = remotePath.split(/\/|\\(?! )/).reverse();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure the (?! ) is necessary. Are you trying to skip an escaped space? I don't think there will be a literal backslash as an escape character at this point, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, trying to skip an escaped space, which is the only backslash I regularly encounter on unix systems. Not sure how delve and this plugin handles filenames on unix, so you might be right, it might not be necessary. I can't try this right now, but I'll give it a try once I have the chance to. That said, I think its save to leave it in as spaces are not allowed at the beginning of a filename on Windows.

Copy link
Member

@roblourens roblourens Feb 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that escaped characters will have been resolved by this point so when you have a\ b you'll have a string which contains a, literal space, b. Just making sure I understand the scenario.

remotePath = remotePath.substring(0, remotePath.length - 1);
}
}

this.delve = new Delve(args.mode, remotePath, port, host, args.program, args.args, args.showLog, args.cwd, args.env, args.buildFlags, args.init);
this.delve = new Delve(args.mode, remotePath, port, host, localPath, args.args, args.showLog, args.cwd, args.env, args.buildFlags, args.init);
this.delve.onstdout = (str: string) => {
this.sendEvent(new OutputEvent(str, 'stdout'));
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that true, since foo != bar? Isn't it like /home/foo1/go/src/bar will match /home/foo2/go/src/bar because the two paths will be trimmed to /home/foo1 and /home/foo2.

if (i) {
localPath = llist.reverse().slice(0, -i).join(this.localPathSeparator);
remotePath = rlist.reverse().slice(0, -i).join(this.remotePathSeparator);
} else if ((remotePath.endsWith('\\')) || (remotePath.endsWith('/'))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this, based on how we do it for node debugging. The program field isn't used for remote debugging, right? So instead of specifying the program, the user could specify the final 'localPath' to match 'remotePath'. Then they are responsible for doing this in their launch config - finding the common path of the two.

Would that end up being more confusing? I'm not opposed to this, just thinking about it.

Other debug adapters have 'launch'-type and 'attach'-type configs, where 'launch' is when it has to start executing the binary, and 'attach' is when it has already been started and is in debug mode. We could introduce that concept here too. Then the 'program' field would not be allowed for remote debug scenarios, just 'remotePath' and 'localPath' to provide the mapping.

Copy link
Contributor Author

@f0zi f0zi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bit confused with the review process here, hope you can see my comments...

I only see them if I click the "view changes" button to the right

@f0zi
Copy link
Contributor Author

f0zi commented Feb 4, 2017

@roblourens So, to move things forward, I don't mind to remove the negative forward assertion, are there any other changes you would like to see?

Again, please make sure you click 'view changes' as not all my comments appear here on this page.

@roblourens roblourens merged commit 958d4f8 into microsoft:master Feb 6, 2017
@roblourens
Copy link
Member

Thanks @f0zi!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants