Unable to run built-in commands using OS.execute

[thebluefish]

Sample Project

Godot version:
3.0.6
8d117b2

OS/device including version:
Windows 10 x64

Issue description:

var output = []
OS.execute('echo', ['somethingyo'], true, output)

Produces the following:

'"echo"' is not recognized as an internal or external command,
operable program or batch file.

It appears that the path argument is being automatically surrounded in quotes, per OS_Windows::execute.

As mentioned in this issue, this can currently be circumvented by calling CMD /K "<command>" - however this is not an elegant solution as it requires launching a second process. I propose one of the following solutions which doesn't require this workaround.

Solution 1:

Remove automatic quotes. This would have the downside of requiring the user to use quotes
when necessary.

Solution 2:

Enclose the path with quotes only when there is whitespace in the path.

Solution 3:

Check if the command is a DOS command, and enclose the path in quotes if it is not.

[bruvzg]

this is not an elegant solution as it requires launching a second process

AFAIK removing quotes won't help with internal commands. cmd is the only way to use them. Unlike Unix tools, Windows/DOS internal commands don't have separate executables and are all bundled into cmd.exe (or command.com in case of DOS) and should not spawn extra processes.

[thebluefish]

It works as long as we remove the quotes. Consider the following script:

func _ready():
    var output = []
    var res = OS.execute('echo somethingyo', [], true, output)
    for o in output:
        print(o)

Currently we are using

argss = "\"\"" + p_path + "\"";

which produces

'"echo"' is not recognized as an internal or external command,
operable program or batch file.

However if I change it to

argss = "\"" + p_path;

then we get

[bruvzg]

Hah, interesting, blocking/pipe version of OS::execute uses _popen call which seems to support external commands, but it have this note in the documentation:

If used in a Windows program, the _popen function returns an invalid file pointer that causes the program to stop responding indefinitely. _popen works properly in a console application.

I wonder if it is working at all in exported app or only in editor (which is console application)?

Non blocking version uses CreateProcess and should not work with internal commands (at least it was not in Windows 7 times).

[thebluefish]

Hm, it does concern me that it wouldn't work with non-blocking option. I suppose we can either:

• Leave the current behavior intact, and allow the user to launch cmd /K for builtins
  • I honestly cannot think of any scenarios where this isn't at least acceptable, though it's unfortunate we're basically dealing with two cmd processes doing so
• Figure out some sort of compatibility with CreateProcess
  • I assume this would involve us wrapping such commands in a cmd /K
• Replace CreateProcess with popen
  • I'm honestly not sure why we use CreateProcess for non-blocking/no-pipe branch - from what I gather _popen is non-blocking and we'd be free to simply open it and move on, though I haven't yet tested this idea.
• Document somewhere that only blocking calls can directly call builtins
  • This just seems like a bad idea

[thebluefish]

I would assume it should work in an exported app, though I will test it tomorrow. If it doesn't, that's likely a larger concern since it would affect existing expected behavior.

[neikeq]

The quotes should only be added if they are needed. Would that solve the issue?

[bruvzg]

Some tests.

Non blocking var pid = OS.execute('echo', ['somethingyo'], false) fails with:

ERROR: execute: Condition ' ret == 0 ' is true. returned: ERR_CANT_FORK
At: platform/windows/os_windows.cpp:2494

Blocking version works in exported app, but it's opening new console window each time, even when it's starting GUI app.

Also cmd /k is not auto-closing console after command execution and will hang in case you are using editor executable to run project from existing command line session, it's better to use cmd /c instead.

[KoBeWi]

Tested in 3.2.4 beta3 and the provided code sample seems to do nothing, not even error appears.