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

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
634973d
Removing common parts from program and remotePath
f0zi Jan 24, 2017
df14353
replacing var with let
f0zi Jan 24, 2017
fe9550b
Found the whitespace and returned it to its spot.
f0zi Jan 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions src/debugAdapter/goDebug.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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

return path.includes('/') ? '/' : '\\';
}

protected launchRequest(response: DebugProtocol.LaunchResponse, args: LaunchRequestArguments): void {
// Launch the Delve debugger on the program
let localPath = args.program;
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 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('/') ? '/' : '\\';
this.remotePathSeparator = remotePath.includes('/') ? '/' : '\\';
if ((remotePath.endsWith('\\')) || (remotePath.endsWith('/'))) {
this.localPathSeparator = this.findPathSeperator(localPath);
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 •
edited
Loading

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.

let i = 0;
for (; i < llist.length; i++) if (llist[i] !== rlist[i]) break;

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.

Expand Down