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
[windows] Use quotes to allow spaces in path #381
[windows] Use quotes to allow spaces in path #381
Conversation
const vswhere_res | ||
= await proc.execute(`${sys32_path}\\cmd.exe`, vswhere_args, null, {silent: true, encoding: 'utf8'}).result; | ||
= await proc.execute(`${sys32_path}\\cmd.exe`, vswhere_args, null, {silent: true, encoding: 'utf8', shell: true}).result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is shell mode necessary here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried without setting it. It reported something like:
'\"C:\Users\Tushar Maheshwari\[...]\vswhere.exe\"' is not recognized as an internal or external command, operable program or batch file.
It escaped the quotes because of the missing windowsVerbatimArguments
(default: false).
The existing proc.execute
does not pass that option to child_process.spawn
, like it does shell
.
According to the documentation, the former "is set to true
automatically when shell
is specified."
Therefore, although the shell
mode might not exactly be necessary, it helps in the fix here.
The alternative is to modify proc.ts:execute
to pass windowVerbatimArguments
also.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the slow response. Does setting windowsVerbatimArguments
work? I'm disinclined to ever use shell
, but we'll have to if nothing else works.
I'm hoping to push 0.11.1 today and want to get this in the release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I attempted this in 7374792.
I got:
$ npm run compile
src/proc.ts(109,5): error TS2322: Type '{ env: EnvironmentVariables; shell: boolean; windowsVerbatimArguments: boolean; }' is not assignable to type 'SpawnOptions'.
Object literal may only specify known properties, and 'windowsVerbatimArguments' does not exist in type 'SpawnOptions'.
I checked @type/node v8.0.30 index.d.ts
(automatically installed by npm install
), SpawnOptions
doesn't declare windowVerbatimArguments
as a member.
However, the generated out/src/proc.js
resembles the expected output.
Should I pull that commit to this branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, just use shell
. The NodeJS version shipping with VSCode is a little outdated so the docs might not match what's available in the VSCode version. Tell me when you're ready to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I tested that change in a different branch. This one should be alright to merge if shell
is acceptable.
@Randshot could you follow up on your review? I can override and do the merge without your review, but I'd rather not. |
The following changes are proposed:
vswhere_exe
path.The purpose of this change
For the case where
vswhere_exe
path has a whitespace character.Other Notes/Information
On Windows, my username has a whitespace ("Tushar Maheshwari" in my case).
VS Code stores the installed extensions, and their resources in the home directory, which has the username in the path.
The extension uses
vswhere
without quotes, which fails with:Tried locally with the changes, the extension succeeds in executing
vswhere
and continues execution.