Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd attach to local runspace. #875
Conversation
|
Not seeing this test failure locally. I am seeing other failures that aren't listed in AppVeyor that seem unrelated to my PR. |
| @@ -21,5 +21,7 @@ public class AttachRequestArguments | |||
| public string ProcessId { get; set; } | |||
|
|
|||
| public int RunspaceId { get; set; } | |||
|
|
|||
| public string LocalRunspaceId { get; set; } | |||
rkeithhill
Mar 9, 2019
Collaborator
Just asking but ... do we really need two different fields for runspace id?
Just asking but ... do we really need two different fields for runspace id?
rkeithhill
Mar 9, 2019
Collaborator
BTW if it would be more convenient if RunspaceId was a string, we change the type I think. We'd just update the VSCode debug attach code to stringify the runspace id number.
BTW if it would be more convenient if RunspaceId was a string, we change the type I think. We'd just update the VSCode debug attach code to stringify the runspace id number.
adamdriscoll
Mar 9, 2019
Author
Contributor
OK. I can make that change. That's pretty much the reason I added another field was that I didn't wanna mess with RunspaceId's type but I'm happy to adjust that to eliminate the new property.
OK. I can make that change. That's pretty much the reason I added another field was that I didn't wanna mess with RunspaceId's type but I'm happy to adjust that to eliminate the new property.
adamdriscoll
Mar 9, 2019
Author
Contributor
I guess I was curious how this affects existing launch.json files. Is there something we need to do to ensure that they get upgraded in some way or will a number just work?
I guess I was curious how this affects existing launch.json files. Is there something we need to do to ensure that they get upgraded in some way or will a number just work?
adamdriscoll
Mar 9, 2019
Author
Contributor
I just tried this. Seems to still work. It complains in the launch.json that the number should be a string but the attaching still works.
I just tried this. Seems to still work. It complains in the launch.json that the number should be a string but the attaching still works.
| { | ||
| runspaceId = attachParams.RunspaceId > 0 ? attachParams.RunspaceId : 1; | ||
| } | ||
| else if (int.TryParse(attachParams.LocalRunspaceId, out int localRunspaceId)) |
rkeithhill
Mar 9, 2019
Collaborator
Seems like we could int.TryParse() on a string typed RunspaceId. In the normal "attach" case we'd just set it to "1". Your new debug config would set it to whatever the user picks from the runspace id pick list.
Seems like we could int.TryParse() on a string typed RunspaceId. In the normal "attach" case we'd just set it to "1". Your new debug config would set it to whatever the user picks from the runspace id pick list.
adamdriscoll
Mar 9, 2019
Author
Contributor
Sweet. Works for me. I will update these PRs sometime this weekend.
Sweet. Works for me. I will update these PRs sometime this weekend.
| @@ -417,16 +417,18 @@ protected void Stop() | |||
| return; | |||
| } | |||
|
|
|||
| await _editorSession.PowerShellContext.ExecuteScriptStringAsync( | |||
| if (attachParams.ProcessId != "current") { | |||
TylerLeonhardt
Mar 11, 2019
Member
Shouldn't need this if because the outer if tries to parse the ProcessId as an integer (L411):
if (processIdIsSet && int.TryParse(attachParams.ProcessId, out int processId) && (processId > 0))
Shouldn't need this if because the outer if tries to parse the ProcessId as an integer (L411):
if (processIdIsSet && int.TryParse(attachParams.ProcessId, out int processId) && (processId > 0))
adamdriscoll
Mar 11, 2019
Author
Contributor
Good catch. Updated the PR.
Good catch. Updated the PR.
| int runspaceId = 1; | ||
| if (int.TryParse(attachParams.RunspaceId, out runspaceId)) | ||
| { | ||
| runspaceId = runspaceId > 0 ? runspaceId : 1; |
TylerLeonhardt
Mar 11, 2019
Member
I would maybe factor this into the if
if (!int.TryParse(attachParams.RunspaceId, out runspaceId) || runspaceId <= 0)
{
Logger.Write(
LogLevel.Error,
$"Attach request failed, '{attachParams.RunspaceId}' is an invalid value for the processId.");
await requestContext.SendErrorAsync(
"A positive integer must be specified for the RunspaceId field.");
return;
}
I would maybe factor this into the if
if (!int.TryParse(attachParams.RunspaceId, out runspaceId) || runspaceId <= 0)
{
Logger.Write(
LogLevel.Error,
$"Attach request failed, '{attachParams.RunspaceId}' is an invalid value for the processId.");
await requestContext.SendErrorAsync(
"A positive integer must be specified for the RunspaceId field.");
return;
}
adamdriscoll
Mar 11, 2019
Author
Contributor
👍
|
Looking good! Just some general comments and questions. |
| var psCommand = new PSCommand(); | ||
| psCommand.AddCommand("Get-Runspace"); | ||
|
|
||
| var runspaces = await editorSession.PowerShellContext.ExecuteCommandAsync<PSObject>(psCommand); |
TylerLeonhardt
Mar 11, 2019
Member
can you replace var with the type since this is a tad more complicated than new Blah()
can you replace var with the type since this is a tad more complicated than new Blah()
TylerLeonhardt
Mar 11, 2019
Member
Also, instead of PSObject you should be able to do System.Management.Automation.Runspaces.Runspace and then you don't have to use dynamic below.
Also, instead of PSObject you should be able to do System.Management.Automation.Runspaces.Runspace and then you don't have to use dynamic below.
adamdriscoll
Mar 11, 2019
Author
Contributor
👍
| { | ||
| var runspaceResponses = new List<GetRunspaceResponse>(); | ||
|
|
||
| if (this.editorSession.PowerShellContext.LocalPowerShellVersion.Version.Major >= 5) |
TylerLeonhardt
Mar 11, 2019
Member
Was Get-Runspace added in 5?
Was Get-Runspace added in 5?
adamdriscoll
Mar 11, 2019
Author
Contributor
That's a really good question. I just took that check from the process picker but I'm not sure when Debug-Runspace\Get-Runspace was added. I can try and find out.
That's a really good question. I just took that check from the process picker but I'm not sure when Debug-Runspace\Get-Runspace was added. I can try and find out.
adamdriscoll
Mar 11, 2019
Author
Contributor
TylerLeonhardt
Mar 11, 2019
Member
Well looks like that answers our question :)
Well looks like that answers our question :)
It's neat that we can just commit a suggestion. Co-Authored-By: adamdriscoll <adamdriscoll@users.noreply.github.com>
|
LGTM! I'm excited for this code path to see more usage. It's a very nice feature of the extension. |
|
@adamdriscoll if you couldn't tell already, that test is flaky... I've been meaning to fix it, but I haven't had the time. I will address it as soon as I can. restarted build. |
|
just a couple nits really. Basically looks good. |
| @@ -1222,17 +1222,27 @@ public static string GetDecoratedSymbolName(SymbolReference symbolReference) | |||
| } | |||
|
|
|||
| protected async Task HandleGetRunspaceRequestAsync( | |||
| object noParams, | |||
| object processId, | |||
TylerLeonhardt
Mar 12, 2019
Member
Does this have to be object? Or can we turn it into a string?
Does this have to be object? Or can we turn it into a string?
adamdriscoll
Mar 13, 2019
Author
Contributor
You're right. Changed to a string.
You're right. Changed to a string.
| @@ -1246,6 +1256,12 @@ public static string GetDecoratedSymbolName(SymbolReference symbolReference) | |||
| }); | |||
| } | |||
| } | |||
|
|
|||
| if (processId != null && processId.ToString() != "current") { | |||
TylerLeonhardt
Mar 12, 2019
Member
Since you do this comparison a couple times, maybe do a:
bool isCurrentProcess = processId != null && processId.ToString() != "current";
and use that.
Since you do this comparison a couple times, maybe do a:
bool isCurrentProcess = processId != null && processId.ToString() != "current";and use that.
adamdriscoll
Mar 13, 2019
Author
Contributor
Works for me. Done.
Works for me. Done.
|
LGTM! Excited to get this in! |
* Add attach to local runspace. * Remove comment. * Use single runspaceId property. * Remove unnecessary processId check. * Factored if. * Update src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs It's neat that we can just commit a suggestion. Co-Authored-By: adamdriscoll <adamdriscoll@users.noreply.github.com> * Use runspace rather than dynamic. * Accept process ID for GetRunspaceRequest. * Clean up.
* Add attach to local runspace. * Remove comment. * Use single runspaceId property. * Remove unnecessary processId check. * Factored if. * Update src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs It's neat that we can just commit a suggestion. Co-Authored-By: adamdriscoll <adamdriscoll@users.noreply.github.com> * Use runspace rather than dynamic. * Accept process ID for GetRunspaceRequest. * Clean up.
Adds support for attaching to a local runspace. This extends the existing support for attaching to a runspace in a remote process. This enables debugging for modules that create additional runspaces such as ThreadJob, PoshRSJob and UnivesalDashboard.