Skip to content

Fix issue with cwd being set to untitled file path #656

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 6, 2017
Merged

Fix issue with cwd being set to untitled file path #656

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 7 additions & 1 deletion src/features/DebugSession.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export class DebugSessionFeature implements IFeature {

if (config.request === 'launch') {
// Make sure there's a usable working directory if possible
config.cwd = config.cwd || vscode.workspace.rootPath || config.script;
config.cwd = config.cwd || vscode.workspace.rootPath;

// For launch of "current script", don't start the debugger if the current file
// is not a file that can be debugged by PowerShell
Expand Down Expand Up @@ -80,6 +80,12 @@ export class DebugSessionFeature implements IFeature {
}
}
}
else if (config.script) {
// 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;
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the config.script is set to Invoke-Pester? Will we try to set that to the CWD?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...

Copy link
Contributor

@rkeithhill rkeithhill Apr 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me take a crack at a PR, would be easier to visualize anyway. :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

When I let undefined slip through as cwd, the debug session seems to hang. Wasn't that the original issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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 618

That 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}
}

// Prevent the Debug Console from opening
Expand Down