Fix issue with cwd being set to untitled file path #656
Conversation
src/features/DebugSession.ts
Outdated
| @@ -51,6 +54,7 @@ export class DebugSessionFeature implements IFeature { | |||
| if (currentDocument.isUntitled) { | |||
| if (currentDocument.languageId === 'powershell') { | |||
| config.script = currentDocument.uri.toString(); | |||
| config.cwd = vscode.workspace.rootPath; | |||
There was a problem hiding this comment.
Actually now that I think about it, this line is probably not needed anymore because the || statement above should cover that case. Leftover from a previous attempt
There was a problem hiding this comment.
Is vscode.workspace.rootPath set when you are in a no-folder workspace?
There was a problem hiding this comment.
Nope, that's why this logic will drop to the next case which checks whether the current file is untitled and sets it as undefined so that the cwd doesn't get set at all.
Though I just realize that this logic has a problem: if the user has an untitled file open but their launch config specifically references a different script file, the untitled file will cause the cwd of the configuration to be overwritten. Sending a fix now.
This change fixes an issue with untitled file paths being sent in the cwd parameter to debug adapter when in a window that doesn't have a workspace loaded. The fix is to change our cwd selection logic to be more aware of an untitled file being used. Resolves PowerShell#655.
| // In this case, the user has explicitly defined a script path | ||
| // so make sure to set the cwd to that path if the cwd wasn't | ||
| // explicitly set | ||
| config.cwd = config.cwd || config.script; |
There was a problem hiding this comment.
What if the config.script is set to Invoke-Pester? Will we try to set that to the CWD?
There was a problem hiding this comment.
I don't know about you, but it's getting to be a bit harder keeping straight the matrix of no-folder vs folder workspaces (ie existence of launch.json) and unsaved vs saved file debugging.
There was a problem hiding this comment.
Yeah, it is. Any ideas?
Regarding Invoke-Pester, I think we'll have to check the string for validity as a path before trying to use it. Though that might not exactly be reliable...
There was a problem hiding this comment.
That check is something we do on the PSES side.
Regarding alternatives, I think around line 37, where we handle the no-folder workspace case we assign cwd there. It would be the filepath if the document has been saved otherwise the workspace rootPath. That leaves the unsaved file with a launch.json case. That by default want's to use "${file}" for the cwd. After line 53 we could set the CWD to the workspace root. Then move the sanity check on CWD to the end of the if (config.request === 'launch'). Does that make sense?
There was a problem hiding this comment.
Let me take a crack at a PR, would be easier to visualize anyway. :-)
There was a problem hiding this comment.
When I let undefined slip through as cwd, the debug session seems to hang. Wasn't that the original issue?
There was a problem hiding this comment.
Weird, it was working fine for me. But yeah, you're right... In theory that would cause the workingDir to be set to the path of the file in the debug adapter. Maybe we should take out this logic in one of the places? Maybe make the debug adapter dumb and the extension code smart?
There was a problem hiding this comment.
OK so if we pass undefined or null we get to this code:
string workingDir =
launchParams.Cwd ??
launchParams.Script ??
#pragma warning disable 618
launchParams.Program;
#pragma warning restore 618That tries to set the CWD to untitled:Untitled-1. Then we try to get file attrs on that and it throws. In the catch handler, we reset workingDir to null and then later that gets set to Environment.CurrentDirectory which is initially $Home. But that can change in the integrated console as the user cd's around. Not sure if that is a feature or a problem. Anyway, specifically for unsaved files, seems like we should handle setting the working dir on the PSES side since it eventually knows where it will save the text in order to run it. I assume you actually save the unsaved file contents to a file to run it. Is that correct? If so, then we could set the CWD to path holding the temp file. Do you think that makes sense? I'm not so sure. It is unlikely that would be advantageous. So maybe we force it $home (always writable and any relative path file output generated by the script would be easier to find) or we stick with Environment.CurrentDirectory.
If we go with home then we should be able to specify that on the extension side ${env.HOME} or something like that (assuming we find a xplat env var). OK that's a lot of yammering. Your thoughts?
There was a problem hiding this comment.
Currently I'm not saving the untitled file contents to a file. Since the language server already has the contents of the untitled file, I just execute the entire file buffer as a script without saving to a file first. This is also what the ISE does with untitled files when you press F5. In the future I might want to save to a temp file so that we can set virtual breakpoints in the untitled script, but then we might run into issues for folks with a restrictive ExecutionPolicy set.
Now that I think about it, does it even make sense to try and set the cwd path when running a script in the shared integrated console session? It's basically resetting the user's current file path. Definitely makes sense with unique debug sessions, but we're not back to that point yet.
Sticking with the Environment.CurrentDirectory approach for now could be fine until we have more time to come up with a better solution. I'm planning to ship a patch update tomorrow morning to resolve some issues we found while preparing demos for the PS Summit.
There was a problem hiding this comment.
Sounds good. Then for the unsaved file, the cwd will come through as undefined, then PSES will temporarily set it to untitled:Untitled-N and then set it back to Env.CurrentDir or AppContext.BaseDirectory on CoreCLR. We might want to cleanup the handling of this on the PSES later but should be OK for now. I'll submit my PR soon for you to check out.
This change fixes an issue with untitled file paths being sent in the
cwd parameter to debug adapter when in a window that doesn't have a
workspace loaded. The fix is to change our cwd selection logic to be
more aware of an untitled file being used.
Resolves #655.