Skip to content

Interactive variable replacment also supported for string commands #1315

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.

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

Closed
isidorn opened this issue May 8, 2018 · 28 comments · Fixed by #1317
Closed

Interactive variable replacment also supported for string commands #1315

isidorn opened this issue May 8, 2018 · 28 comments · Fixed by #1317
Assignees
Labels

Comments

@isidorn
Copy link
Contributor

isidorn commented May 8, 2018

Hello friends, I am from the vscode team and some users are reporting an issue with debugging powershell. More datils can be found here microsoft/vscode#49390

After investigating I noticed that your command SpecifyScriptArgs which is used to substitue values before debugging is returning an array of strings instead of a string. This was never officialy supported, and with the refactoring on the vscode side we are more strict and only allow strings.
Bottom line you should change a cast to array to simply return a string and not an array here

return new Array(text);

Make sure that all your commands which are used for launch.json configs are returning strings and not array.

@rkeithhill
Copy link
Contributor

Thanks for the heads up. I'll make this change.

@rkeithhill rkeithhill self-assigned this May 8, 2018
@rkeithhill rkeithhill added this to the 1.7.1 milestone May 8, 2018
@isidorn
Copy link
Contributor Author

isidorn commented May 8, 2018

Great, thanks

@eknraw
Copy link

eknraw commented May 8, 2018

@isidorn @rkeithhill Thank you.

@rjmholt rjmholt added Issue-Bug A bug to squash. Area-Debugging labels May 8, 2018
@rkeithhill
Copy link
Contributor

rkeithhill commented May 9, 2018

@isidorn @weinand Small problem. We allow the user to delete all the text in the "specify args" dialog box:

image

The problem is that if the user leaves the input box empty (intending to pass no args), then the debugger never bothers to launch this config. AFAICT, I have no control over this behavior. By this point, our resolveDebugConfiguration has already been called and the debug adapter is never sent the launch request. In our case, empty input is valid input.

@isidorn
Copy link
Contributor Author

isidorn commented May 9, 2018

@rkeithhill in that case your SpecifyScriptArgs command should return an empty string '' and I believe everything should work fine. Did you try that out?
This will work since type of empty string is string and this check should correctly pass.

@rkeithhill
Copy link
Contributor

rkeithhill commented May 9, 2018

But if the input box is left blank, an empty string is what I see in the debugger and is what is returned to vscode?? I'll double check that tonight. I suspect that is why I had the return new Array(text) code there in the first place.

@isidorn
Copy link
Contributor Author

isidorn commented May 9, 2018

@rkeithhill ok please double check and let us know what is unexpected exactly.

@avelis26
Copy link

avelis26 commented May 9, 2018

Not trying to be that guy but, I downloaded and installed the latest "stable" build of Visual Studio Code, loaded the PowerShell extension, and tried to execute a PowerShell script with args and it's broken...

Not exactly a "stable" build so I'm wondering if any changes should be made to the deployment pipeline so obvious bugs like this don't find their way into "production". I'm happy to help as I've always wanted to assist in the open source world.

@weinand
Copy link

weinand commented May 9, 2018

@avelis26 VS Code did not change any specified and documented functionality here. We just closed some undocumented loophole and broke the Powershell extension.
We are sorry about that but there are thousand of extension out there and we cannot test them all against using unspecfied functionality.

@rkeithhill
Copy link
Contributor

@avelis26 Given that this is a UI feature (inputbox) that is used as part of starting up the debugger, it might be difficult to test. But if you can find a way to automate such a test, that would be great! Hopefully, we'll have this fixed and can get out a 1.7.1 maintenance release shortly. We have a few other issues that cropped up in 1.7.0 that need to be fixed as well.

@avelis26
Copy link

avelis26 commented May 9, 2018 via email

@avelis26
Copy link

avelis26 commented May 9, 2018

@weinand
I understand not all extensions can be tested, but I feel Visual Studio Code is a Microsoft product and thus, major Microsoft extensions like PowerShell should probably have a few smoke tests prior to release.

I also understand that dream may never come true but like I said, I'm happy help in whatever way I can.

@weinand
Copy link

weinand commented May 9, 2018

@avelis26 we expect extension authors to verify their extensions against the daily Insiders release (in fact that's exactly the main purpose of the Insiders release).
We do smoke test some non built-in extension, but we cannot track and test what undocumented features they rely on deliberately or by accident.

@rkeithhill
Copy link
Contributor

@isidorn @weinand OK we are returning an empty string - the empty string that inputbox returns to us. The debug session is not initiated.

image

It appears to me that VSCode is treating an empty string result the same as undefined. In either case, the debug session is not initiated.

@isidorn
Copy link
Contributor Author

isidorn commented May 10, 2018

@rkeithhill thanks, there was an issue on the vscode side which I tackled via microsoft/vscode@11a1ce9

Please try tomorrows insiders and let me know if this fixes the issue for you.
I created a dummy extension which returns an empty string for commands and it worked great for me.

Since this is a corner case (returning empty string) I choose not to include this fix in the recovery release. It will be present in the next stable build.

@rkeithhill For the future it would be great if you self hosted on vscode insiders so we find issues like this before the release.

Thanks a lot

rkeithhill added a commit to rkeithhill/vscode-powershell that referenced this issue May 12, 2018
Fix PowerShell#1315 but folks won't see this fix until they are on VSCode 1.24
or insiders build >= 5/11.
This is because a corresponding fix is required in VSCode.
rkeithhill added a commit that referenced this issue May 12, 2018
…#1317)

Fix #1315 but folks won't see this fix until they are on VSCode 1.24
or insiders build >= 5/11.
This is because a corresponding fix is required in VSCode.
@rkeithhill
Copy link
Contributor

This bug has been fixed but you will need VSCode 1.24 or an insiders daily build of 1.24 >= 5/11.

@rkeithhill rkeithhill changed the title Intercative variable replacment also supported for string commands Interactive variable replacment also supported for string commands May 13, 2018
@beniwa
Copy link

beniwa commented Jun 12, 2018

@rkeithhill Is this fix in VSCode 1.24 included or does the extension need a new release as well?

@rkeithhill
Copy link
Contributor

Fix is on master but we haven't released since I committed the fix. We really need to push a release but there is one major fix for finding the correct version of PSScriptAnalyzer that needs to go in before we release. That PR is done, just waiting for a final approval by @tylerl0706 ... nudge, nudge.

@TylerLeonhardt
Copy link
Member

Almost to a new release! Just a few internal processes that need to be taken care of this one time. Thank you for you patience 😊

@bluearcher-bc
Copy link

Yes, please. This bug is killing us at work :)

@jasoncopseynielseniq
Copy link

Any ETA on this please?

@mpearon
Copy link

mpearon commented Jul 10, 2018

As of vsCode v1.25.0 this is still misbehaving.
@tylerl0706 - Are we thinking that 1.26 will contain the fix?

@bluearcher-bc
Copy link

bluearcher-bc commented Jul 10, 2018

@mpearon Some component of this fix was on VS Code, it was shipped in 1.24. The remaining fix has to come with vscode-powershell 1.7.1. I am not sure what the hold up is, it has been 2.5 months since 1.7.0, and very little movement is showing in the issues for the 1.7.1 milestone (EDIT: wording)

@rkeithhill
Copy link
Contributor

My understanding is the change to use named pipes instead of TCP ports, introduced a delay due to both effort to implement and subsequent internal review. There's also been ongoing work to improve the whole release process to make it simpler/quicker to roll releases in the future.

If you were to take a look at merged PR's, you'd see there's been a lot of work done since 1.7.0. I'm as anxious as everyone else to see an update released but that has to be done by @tylerl0706 (or @rjmholt). @tylerl0706 is out this week on a cruise (lucky dog).

@TylerLeonhardt
Copy link
Member

The PowerShell extension has been updated (now at 1.8.1). Can you give this a try now?

@jasoncopseynielseniq
Copy link

I received notice of an update to the extension and after installation - I can confirm that I am once again able to run powershell with args in debugging.

Thanks for sorting this out.

@123dev
Copy link

123dev commented Jul 16, 2018

Working fine in 1.8.1.

Thanks

@mpearon
Copy link

mpearon commented Jul 20, 2018

I'm still having issues with this - but only when the vsLiveShare extension is enabled. I mentioned this thread when reporting the issue there.

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

Successfully merging a pull request may close this issue.