Skip to content
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

terminal.integrated.shellArgs.windows user setting ignores quotes, and escape sequences #57452

Closed
AArnott opened this issue Aug 28, 2018 · 29 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug terminal General terminal issues that don't fall under another label verified Verification succeeded windows VS Code on Windows issues
Milestone

Comments

@AArnott
Copy link
Member

AArnott commented Aug 28, 2018

Issue Type: Bug

As described in this blog post, add these settings to the User Settings json file (adjusting for your actual VS 2017 install dir as necessary):

    "terminal.integrated.shell.windows": "cmd.exe",
    "terminal.integrated.shellArgs.windows": [
        "/k",
        "C:\\Program Files (x86)\\Microsoft Visual Studio\\2017\\release\\Common7\\Tools\\VsDevCmd.bat"
    ],

As (somewhat) expected, this gets the VS Code Terminal running with the VsDevCmd.bat script started first. But this doesn't include quotes around the VsDevCmd.bat script. While this works in the simple Terminal window case, it causes any VS Code "tasks" to fail with this bizarre error:

> Executing task: d:\git\Nerdbank.Streams\src\nerdbank-streams\node_modules\.bin\tsc.cmd -p d:\git\Nerdbank.Streams\src\nerdbank-streams\tsconfig.json <

'C:\Program' is not recognized as an internal or external command,
operable program or batch file.

So we need to add quotes, apparently. But here's where the VS Code bug comes in. Given this is JSON, I should be able to add quotes by escaping them:

    "terminal.integrated.shellArgs.windows": [
        "/k",
        "\"C:\\Program Files (x86)\\Microsoft Visual Studio\\2017\\release\\Common7\\Tools\\VsDevCmd.bat\""
    ],

But VS Code doesn't interpret the escape sequences and instead keeps the backslashes in the string, producing this error when I open a terminal window:

'\"C:\Program Files (x86)\Microsoft Visual Studio\2017\release\Common7\Tools\VsDevCmd.bat\"' is not recognized as an internal or external command,
operable program or batch file.

Very oddly, this syntax is accepted:

    "terminal.integrated.shellArgs.windows": [
        "/k "C:\\Program Files (x86)\\Microsoft Visual Studio\\2017\\release\\Common7\\Tools\\VsDevCmd.bat\""
    ],

Note the above isn't even valid JSON since the inside quotes aren't properly escaped. Yet VS Code happily runs with this and spawns the process... with the inner quotes dropped, which means it doesn't actually solve my problem because once again, the VS Code tasks fail because of the missing quotes around that script.

Can we please get this JSON file properly interpreted, including escape sequences, so that we can use quotes in these shell args?

This seems closely related to #44152 but as that is focused on tasks inheriting a common environment, I think a separate issue tracking just supporting using quotes inside shell arguments is worthwhile and would solve my issue.

VS Code version: Code 1.26.1 (493869e, 2018-08-16T18:38:57.434Z)
OS version: Windows_NT x64 10.0.17134

System Info
Item Value
CPUs Intel(R) Core(TM) i7-8650U CPU @ 1.90GHz (8 x 2112)
GPU Status 2d_canvas: enabled
checker_imaging: disabled_off
flash_3d: enabled
flash_stage3d: enabled
flash_stage3d_baseline: enabled
gpu_compositing: enabled
multiple_raster_threads: enabled_on
native_gpu_memory_buffers: disabled_software
rasterization: enabled
video_decode: enabled
video_encode: enabled
webgl: enabled
webgl2: enabled
Memory (System) 15.93GB (2.75GB free)
Process Argv C:\Users\andarno\AppData\Local\Programs\Microsoft VS Code\Code.exe ..
Screen Reader no
VM 0%
Extensions (29)
Extension Author (truncated) Version
npm-intellisense chr 1.3.0
path-intellisense chr 1.4.2
vscode-markdownlint Dav 0.20.0
jshint dba 0.10.19
vscode-eslint dba 1.5.0
githistory don 0.4.2
xml Dot 2.3.2
gitlens eam 8.5.6
remotehub eam 0.2.0
EditorConfig Edi 0.12.4
tslint eg2 1.0.38
vscode-npm-script eg2 0.3.5
git-project-manager fel 1.6.1
docker-linter hen 0.5.0
beautify Hoo 1.4.2
docomment k-- 0.1.2
azure-account ms- 0.4.3
cpptools ms- 0.18.1
csharp ms- 1.15.2
liveshare-logviewer ms- 0.2.1
PowerShell ms- 1.8.3
team ms- 1.136.0
debugger-for-chrome msj 4.8.2
vscode-docker Pet 0.1.0
vscode-versionlens pfl 0.21.1
java red 0.29.0
vscode-icons rob 7.25.0
gitconfig sid 2.0.0
vsc-docker Zim 0.34.0

(1 theme extensions excluded)

@vscodebot vscodebot bot added the terminal General terminal issues that don't fall under another label label Aug 28, 2018
@Tyriar
Copy link
Member

Tyriar commented Aug 29, 2018

This is related to the way that we're computing the "Command Line" string in node-pty, here's the code pointer:

https://github.com/Microsoft/node-pty/blob/cafed13d456fa37df46a9dc26f9a5ebdfe88f3f6/src/windowsPtyAgent.ts#L113-L161

The fix here may be to allow passing in a raw string like is allowed in tasks https://github.com/Microsoft/node-pty/blob/cafed13d456fa37df46a9dc26f9a5ebdfe88f3f6/src/windowsPtyAgent.ts#L121

Interested in your thoughts on this @dbaeumer; should terminal.integrated.shellArgs.windows be a string[] | string?

@Tyriar Tyriar added bug Issue identified by VS Code Team member as probable bug windows VS Code on Windows issues labels Aug 29, 2018
@dbaeumer
Copy link
Member

Since this is already OS & Shell specific I would stick with one string which is exactly pass this way to the shell. Then it is clear how it is for example quoted. I see no benefit for a []

@Tyriar
Copy link
Member

Tyriar commented Aug 30, 2018

@dbaeumer you mean allow a string only for Windows? This would break people if we didn't support string[] anymore.

@AArnott
Copy link
Member Author

AArnott commented Aug 30, 2018

Changing the way you're interpreting the string(s) might also break people (since quotes that you're dropping would suddenly not be dropped and cause a JSON parsing failure instead). But I think it's a good change regardless. You might have a helpful error message that links to a doc describing the change though.

@dbaeumer
Copy link
Member

@Tyriar my misunderstanding. Then it should be string[] | string.

@Tyriar
Copy link
Member

Tyriar commented Aug 30, 2018

@sandy081 is it possible to have VS Code settings be union types. I want to have a string[] | string in a single setting but still have the type checking/warnings when it's wrong.

@sandy081
Copy link
Member

This has to be supported by JSON Schema.. @aeschli ?

@aeschli
Copy link
Contributor

aeschli commented Aug 31, 2018

Try something like:

{
	"properties": {
		"terminal.integrated.shellArgs.windows": {
			"anyOf": [
				{
					"type": "array",
					"items": {
						"type": "string",
						"description": "%arg.desc%"
					},
				},
				{
					"type": "string",
					"description": "%arg.desc%"
				}
			],
			"default": []
		}
	}
}

@Tyriar Tyriar added help wanted Issues identified as good community contribution opportunities good first issue Issues identified as good for first-time contributors labels Aug 31, 2018
@alexr00 alexr00 self-assigned this Sep 4, 2018
@alexr00
Copy link
Member

alexr00 commented Sep 4, 2018

I tried this out, and it looks like even using string[] | string doesn't solve this. The args string is passed into winpty spawn as expected with the escaped characters converted. The bug in winpty needs to be addressed to solve this.

@AArnott
Copy link
Member Author

AArnott commented Sep 4, 2018

If you're using child_process.spawn (or similar), the removal of the quotes may be by design unless you set windowsVerbatimArguments: true as documented here.

@alexr00
Copy link
Member

alexr00 commented Sep 5, 2018

Looks like I was hitting a separate issue. string[] | string does work around this.

alexr00 added a commit that referenced this issue Sep 5, 2018
@alexr00 alexr00 added this to the September 2018 milestone Sep 5, 2018
alexr00 added a commit that referenced this issue Sep 6, 2018
Work around #57452 by allowing a raw string for windows shell args
@alexr00 alexr00 closed this as completed Sep 7, 2018
@alexr00
Copy link
Member

alexr00 commented Sep 7, 2018

With the work around in #57971 you can put all the arguments in one string instead of an array. With microsoft/node-pty#222 , the actual escaping issue is fixed.

@chrmarti
Copy link
Collaborator

@dbaeumer The task does not run, maybe that's expected because the task is passed to the shell as a parameter?

My settings:

	"terminal.integrated.shell.windows": "cmd.exe",
    "terminal.integrated.shellArgs.windows": [
        "/k",
        "\"C:\\devel\\fresh\\sp ace\\echo.bat\""
	]

My tasks.json:

{
    "version": "2.0.0",
    "tasks": [
        {
            "taskName": "echosomething",
            "type": "shell",
            "command": "echo",
            "args": [
                "HERE"
            ]
        }
    ]
}

echo.bat:

echo TEST!

The output is:

> Executing task: echo HERE <


C:\devel\fresh>echo TEST!
TEST!

C:\devel\fresh>

When I remove the shell arguments, the task runs as expected.

@chrmarti
Copy link
Collaborator

Also: With the quotes the terminal (without task) now opens with an error: '\"C:\devel\fresh\sp ace\echo.bat\"' is not recognized as an internal or external command, operable program or batch file.

That seems unexpected. Reopening.

@chrmarti chrmarti reopened this Sep 28, 2018
@chrmarti chrmarti added the verification-found Issue verification failed label Sep 28, 2018
@Tyriar
Copy link
Member

Tyriar commented Sep 28, 2018

@chrmarti this is not meant to work, the quoting should happen for you:

    "terminal.integrated.shell.windows": "cmd.exe",
    "terminal.integrated.shellArgs.windows": [
        "/k",
        "\"C:\\devel\\fresh\\sp ace\\echo.bat\""
    ]

Verification steps:

    "terminal.integrated.shell.windows": "cmd.exe",
    "terminal.integrated.shellArgs.windows": [
        "/k",
        "C:\\devel\\fresh\\sp ace\\echo.bat"
    ]

@Tyriar Tyriar removed the verification-found Issue verification failed label Oct 1, 2018
@Tyriar Tyriar modified the milestones: September 2018, October 2018 Oct 1, 2018
@alexr00
Copy link
Member

alexr00 commented Oct 1, 2018

There is an issue with terminalTaskSystem which is not doing any escaping. However I think there's also some contention between "terminal.integrated.shellArgs.windows" and tasks. @chrmarti, your expectation is that your terminal starts with your specified args and then runs your task, correct?

As far as I understand, the way it actually works is the task command + parameters are passed as additional parameters when starting your terminal with shell args. So, the whole line would look something like(if we were escaping it correctly):

cmd.exe /k "c:\users\alros\documents\sp ace\echo.bat" "echo HERE"

I assume that instead you wanted to run the shell args and then the task sequentially. It seems like for tasks we should start the terminal with the shellArgs then execute the task instead of putting them all in one command line. @dbaeumer and @Tyriar, what do you think?

@Tyriar
Copy link
Member

Tyriar commented Oct 1, 2018

It seems like for tasks we should start the terminal with the shellArgs then execute the task instead of putting them all in one command line

@alexr00 tasks need to finish though and they won't if you just launch the shell and run some stuff. Also I think that would be a pretty radical departure from the current design.

@alexr00
Copy link
Member

alexr00 commented Oct 1, 2018

In that case, the task and shellArgs that @chrmarti tested with shouldn't work.

@Tyriar Tyriar removed good first issue Issues identified as good for first-time contributors help wanted Issues identified as good community contribution opportunities labels Oct 1, 2018
@alexr00
Copy link
Member

alexr00 commented Oct 1, 2018

The original issue where the terminal was incorrectly escaping has been fixed. Opened a new issue to figure out how to reconcile shellArgs and tasks. #59789

@alexr00 alexr00 closed this as completed Oct 1, 2018
@roblourens
Copy link
Member

What is the correct format of that setting now? I tried both (with escaped double quotes and without) and neither seems to work. I either get 'C:\Program' is not recognized as an internal or external command'C:\Program' is not recognized as an internal or external command or with escaped quotes, opening a normal terminal complains that '\"C:\Program Files (x86)\Microsoft Visual Studio 14.0\Common7\Tools\VsDevCmd.bat\"' is not recognized as an internal or external command, operable program or batch file.

@roblourens roblourens reopened this Nov 2, 2018
@roblourens roblourens added the verification-found Issue verification failed label Nov 2, 2018
@alexr00
Copy link
Member

alexr00 commented Nov 2, 2018

Looks like this has completely regressed. It only ever worked in insiders, so moving this to November.

@joaomoreno
Copy link
Member

After discussing this with @alexr00, it is my understanding that this has been an issue in stable for long. Given that we're at the end of endgame and it's not a regression nor severe, I'll push this out to November.

@alexr00 alexr00 modified the milestones: October 2018, November 2018 Nov 2, 2018
@alexr00 alexr00 removed the verification-found Issue verification failed label Nov 2, 2018
@alexr00
Copy link
Member

alexr00 commented Nov 7, 2018

@roblourens, can you confirm whether C:\Program Files (x86)\Microsoft Visual Studio 14.0\Common7\Tools\VsDevCmd.bat actually exists? I realized I don't have that file and tried again with a file I do have and it works.

@alexr00 alexr00 modified the milestones: November 2018, October 2018 Nov 7, 2018
@alexr00 alexr00 added the verified Verification succeeded label Nov 7, 2018
@alexr00 alexr00 closed this as completed Nov 7, 2018
@roblourens
Copy link
Member

Yeah it does exist

@damianog
Copy link

EUREKA!

I managed to launch the dev cmd by adding an env variable

"terminal.integrated.env.windows": {
        "VsDevCmd": "\"C:\\Program Files (x86)\\Microsoft Visual Studio\\2017\\Community\\Common7\\Tools\\VsDevCmd.bat\""
    },
    "terminal.integrated.shellArgs.windows": [
        "/k",
        "%VsDevCmd%"
]

image

@AArnott
Copy link
Member Author

AArnott commented Nov 22, 2018

Very nice, @damianog. I made a couple changes so I could use the VsDevCmd script and still launch powershell:

    "terminal.integrated.env.windows": {
        "VsDevCmd": "\"C:\\Program Files (x86)\\Microsoft Visual Studio\\IntPreview\\Common7\\Tools\\VsDevCmd.bat\""
    },
    "terminal.integrated.shell.windows": "C:\\WINDOWS\\system32\\cmd.exe",
    "terminal.integrated.shellArgs.windows": [
        "/k",
        "%VsDevCmd%",
        "&&",
        "%windir%\\system32\\windowspowershell\\v1.0\\powershell.exe"
    ],

@alexr00
Copy link
Member

alexr00 commented Dec 6, 2018

I still can't repro this. I'm using this:

"terminal.integrated.shell.windows": "c:\\Windows\\System32\\cmd.exe",
"terminal.integrated.shellArgs.windows": [
    "/k",
    "C:\\Program Files (x86)\\Microsoft\\path with more spaces\\test.bat"
],

Can one of the folks who is still seeing space escape issues try this out?

@AArnott
Copy link
Member Author

AArnott commented Dec 6, 2018

@alex00 Are you trying my original repro steps? Remember that this alone will allow the terminal to launch, but it's the VS Code "tasks" to fail.

@vscodebot vscodebot bot locked and limited conversation to collaborators Dec 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug terminal General terminal issues that don't fall under another label verified Verification succeeded windows VS Code on Windows issues
Projects
None yet
Development

No branches or pull requests

10 participants