Command array is not passes arg by arg

[AJIOB]

This issue is continuation of #83.

launch.json config:

{
            "id": "load-path-value",
            "type": "command",
            "command": "shellCommand.execute",
            "args": {
                "useSingleResult": true,
                "description": "Load PATH value",
                "command": [
                    "${config:cmake.cmakePath}",
                    "-E",
                    "cat",
                    "test-environment.txt"
                ],
                "taskId": "load-path-value-task",
                "cwd": "${command:cmake.buildDirectory}",
                "defaultOptions": [
                    "<Please, reconfigure CMake>",
                ],
            }
        }

If I use this command, I see the error of the screenshot above:

I think that every arg from the array will be a single command/arg for the program: I pass the 4 elements => element 0 is an executable and elements 1-3 are the 3 args, independently of the any number of quotes and spaces after the variables expansion.

IMO, they should be escaped if need (with another shell-specific symbols too).

[MarcelRobitaille]

It seems like you are right. VSCode does handle commands as arrays like this. Unfortunately, they don't document support for commands as arrays at all: https://code.visualstudio.com/docs/editor/tasks-appendix

command: string;

I did a little test: "command": [ "python", "${workspaceFolder}/test.py", "foo", "bar" ], with test.py as print(sys.argv) and it indeed prints out ['test.py', 'foo', 'bar']

However, the current implementation is consistent with what was requested in #70 and changing this would be a breaking change.

[AJIOB]

@MarcelRobitaille, hi.

As I can see, VS Code support array of arguments.

You can use BaseTaskConfiguration:

• command will be arg0 (executable).
• args should contain another arguments (arg1...argN).

For the current case, I also think that type should be process, not a shell. But for the classical variant, when the whole string is passed, it should be shell for parsing & executing real shell script.

Do you agree?

[MarcelRobitaille]

Hi @AJIOB.

I am not really sure what you are proposing. This extension provides inputs, not BaseTaskConfiguration. Unfortunately, it appears that the published schema is out of date, so I can't find a schema for inputs.

If we change it so command is arg0 and args is arg1...argN, it's still a breaking change for people who expected command to be interpreted like in a shell.

We could check if args is present and only then treat command like arg0, but this adds a lot of complexity.

This discussion about shells seems similar to #59.

[AJIOB]

Ok, it may be a shell, no difference.

I still think that every element of the command, if it is an array, should be escaped for having 1:1 matching: one array element is equal to one argument/program name.

Another way, array syntax is really useless & will be incorrectly understandable.

[augustocdias]

Maybe we could add an option to escape some chars from the commands, like space.

From my understanding that's the issue with your specific case here

[MarcelRobitaille]

Hi @augustocdias. What do you think about this proposal?

We could check if args is present and only then treat command like arg0

I would be happy to implement it. I also think I have an idea for some simple tests.

[augustocdias]

I didn't get this comment. We don't have an args attribute... And treating the first entry in the array as arg0 wouldn't solve the issue, would it? The problem here is that the space is not being escaped...

I think @AJIOB is a bit confused about how this works. Our extension has control only over what is inside the args object. Everything else is from VSCode and we can't change it. That "command": "shellCommand.execute" is the reason of the existence of this extension. When I first tried to interact with this I tried to set a shell command in this attribute and to my surprise it would accept only vscode commands. I opened an issue in VSCode asking to support shell commands as well and they rejected. Then I created an extension to run shell commands. And what our extension receives from VSCode is only what's inside the args attribute. The outer command is a fixed string that our extension registers in VSCode, so it knows it has to call us when invoked.

[AJIOB]

Hi @augustocdias

You are partially right.

If I pass the command as a string, it should be executed as a usual shell string, without any escapes.

If I pass the command as an array or strings, how they should be executed? As a single command (merge the elements to a single string with required elements escaping to receive a command with x arguments) or as an array of sequential commands?

If the second case will be selected, please, think about failure checking: sometimes we need to check only the last command exit code, sometimes - for every command.

P.S. IMO, selected variant for the command array mode should be defined more cleaner inside the documentation.

[AJIOB]

P.P.S. Also, if the second variant will be selected as a main, it will be awesome to have the first one as an addition (maybe with the internal arrays or something else)

[augustocdias]

I don't think making an array of commands is a good feature. The intended behavior is always the first is the command the others args. I agree the docs should reflect that. I'm not sure when this was changed (as it was originally implemented as only a string) and whoever did it forgot to update the doc.

My opinion is that we could just add an option with characters to escape from the first item of the array or the whole string if a string is passed. There might be people not expecting things to be escaped so I wouldn't set this as a default behavior.

[AJIOB]

One more note: we must escape all the arguments if we escape it.

Try to pass to the test python script from this comment path with spaces. You will have arguments splitting by the space, but it's not nice to have it.

[augustocdias]

you're right. If we escape, everything should be escaped.

What I propose is that we create a new option in the extension to inform which chars should be escaped.

Something like this:

"escapeChars": [" ", "$"]

What do you think @AJIOB, @MarcelRobitaille ?

[AJIOB]

For the sh/bash/*sh we need to escape at least characters from this one.

For the CMD this one may be used as a base.

[augustocdias]

The idea is to let users configure themselves what and what not.

[AJIOB]

It will be the good start. Maybe we add the predefined value for the future (or just a macro that will be expanded)

[MarcelRobitaille]

To be honest, I don't like the escaping solution very much. I would rather pass the array of arguments to execFileSync.

I think there was some confusion because of the two things called "args". My proposal was effectively this (I renamed "args" to "commandArgs" to avoid confusion).

{
            "command": "shellCommand.execute",
            "args": {
                "command": "${config:cmake.cmakePath}",
                "commandArgs": [
                    "-E",
                    "cat",
                    "test-environment.txt"
                ],
            }
        }

So if "commandArgs" is present, we use execFile(command, commandArgs) instead of execSync(command.join(' ')).

We could also change how "command" as an array is treated. If it's an array, instead of simply joining the elements, use execFile(command[0], command[1...]). However, this is a breaking change for everyone who relies on the execSync(command.join(' ')). This does not affect anyone still using the old way of just command as a simple string.

[augustocdias]

I think we could go your route @MarcelRobitaille

It would be way more elegant and flexible. We could release a 2.0 and find a way to display a notification about breaking changes.

I'm not familiar with the execFileSync, how would it handle piped commands (find . -name "*.ts" | grep "main"), command substitution (find $(pwd) -name "*.ts") and setting env vars to the executable (MY_ENV=test ./my_executable)? I think those use cases must work with any solution we provide.

[MarcelRobitaille]

To be clear, my proposal is:

Variant A:

• "command": "string" does not change, still uses execSync().
• "command": ["string", "string"] does not change, still execSync(command.join(" "))
• (new) "command": "string", "commandArgs": []: execFileSync(command, commandArgs) ^ no breaking change, 1 variant added

Variant B:

• "command": "string" still does not change, still uses execSync().
• "command": ["string", "string"] execFileSync(command[0], [command[1], command[2], ...]) ^ breaking change, but only for people who use command as an array, which was only introduced recently

All of the command in your comment with | and $() do not change

[AJIOB]

All of the command in your comment with | and $() do not change

Will the ${...} expressions be evaluated for every variant items? IMO, they must.

[MarcelRobitaille]

Yes I agree.

[augustocdias]

Variant A is more flexible and doesn't introduce any breaking change.