What should Process(args, shell: true) do on Windows?

[oprypin]

Let's examine the current implementation of Process.new(command, args, shell: true).

crystal/src/process.cr

Lines 387 to 405 in 4401e90

	if shell
	command = %(#{command} "${@}") unless command.includes?(' ')
	shell_args = ["-c", command, "--"]
	if args
	unless command.includes?(%("${@}"))
	raise ArgumentError.new(%(can't specify arguments in both, command and args without including "${@}" into your command))
	end
	{% if flag?(:freebsd) %}
	shell_args << ""
	{% end %}
	shell_args.concat(args)
	end
	command = "/bin/sh"
	args = shell_args
	end

For context, this allows you to pass a free-form command but also safely pass a list of arguments to it. E.g.

files = ["a.txt", "b.txt"]
Process.run("grep -E 'foo.*bar' -- \"${@}\" another.txt", files, shell: true)

It relies on POSIX shell to expand the arguments for it, which is a nice implementation, however, this kind of puts us in an uncertain situation for also supporting Windows.

The problem is that the literal text "${@}" must be present in the specified command, but

• it doesn't mean anything on Windows
• there's no direct replacement for it, and anyway we shouldn't make users choose the character sequence based on the platform
• if one tries to kludge this with a literal text replacement, other issues arise

So I think the best way forward needs to avoid relying on this specific character sequence. But I don't know how to do it without losing functionality.

And in general, what is Process.run(command, args, shell: true) even supposed to do on Windows?
If we disregard the desire to avoid hacks, sure, I could just do a literal replacement from "${@}" to args, safely quoted and joined. But hm.....

Should this just raise "not implemented"? Maybe.
But the compiler does rely on this feature in this crucial code:

crystal/src/compiler/crystal/compiler.cr

Line 358 in 0bb3fe9

%(#{cc} #{object_name} -o '#{output_filename}' #{link_flags} #{program.lib_flags})

crystal/src/compiler/crystal/compiler.cr

Line 395 in 0bb3fe9

process_wrapper(linker_command, object_names) do |command, args|

---

Side note: how Python does this:

subprocess.run(["grep -E 'foo.*bar' -- \"${@}\" another.txt", "a.txt", "b.txt"], shell=True)

So it has the equivalent functionality but it's never advertised as anything special, the POSIX shell just happens to do this if multiple args are passed.

On POSIX with shell=True, the shell defaults to /bin/sh. If args is a string, the string specifies the command to execute through the shell. This means that the string must be formatted exactly as it would be when typed at the shell prompt. This includes, for example, quoting or backslash escaping filenames with spaces in them. If args is a sequence, the first item specifies the command string, and any additional items will be treated as additional arguments to the shell itself. That is to say, Popen does the equivalent of:

Popen(['/bin/sh', '-c', args[0], args[1], ...])

And on Windows, ... Python just doesn't do anything useful. I guess it runs cmd /C "command" "arg1" "arg2"

---

I also quite dislike that in Crystal the command and args are separate; usually it's more natural to have the command be just the first item of args. Yes, it's another topic, but note that it could make some things make more sense here.

[j8r]

Where the compiler use the shell interpreter, it can be refactored not to use it.
For example by using environment variables (e.g. for link flags) and using an array for arguments.

shell: true should raise on Windows.

[RX14]

shell: true should not raise - cmd.exe is the shell.

I think that passing arguments to shell: true on windows should either ignore the arguments, or raise. I don't mind which.

As @j8r said though, the compiler can be modified to not use the shell and instead construct the args array without ${@}

[oprypin]

The reason that people actually use shell: true is to not have to split their command manually. I.e. `ls "foo bar"` vs ["ls", "foo bar"].

My main plan now is to not support only args and shell at the same time:
raise NotImplementedError.new("Process with args and shell: true is not supported on Windows")

---

And shell: true, I think, should not actually invoke any shell, just pass the command unmodified to CreateProcess, as opposed to the normal mode which has to turn the array into a command string.

Because on Windows the thing you pass to a process is a single string with all args, possibly quoted, unlike on Unix where you pass an array.

If you disagree that there should be no support for implicitly invoking cmd.exe, then note that if this occupies the shell: true spot, then how can we satisfy the need for a mode where one just doesn't want to mess with the input string and also doesn't want any shell?

[oprypin]

I should also note that for running commands with the ` operator it would be really nice if it didn't actually invoke cmd.exe every time. That fits nicely into what I said, as this just uses shell: true.

[RX14]

If you disagree that there should be no support for implicitly invoking cmd.exe, then note that if this occupies the shell: true spot, then how can we satisfy the need for a mode where one just doesn't want to mess with the input string and also doesn't want any shell?

Passing a single argument in args with shell: false should be equivalent to that.

cmd.exe is the shell on windows.

[oprypin]

Passing a single argument in args with shell: false should be equivalent to that.

Oh no no, it shouldn't.
What do you mean, Process.new("my executable.exe") or Process.new(args: ["my file.txt"])? Either of these need to be safely quoted, not kept as is.

[oprypin]

Ah I think you mean Process.new("my executable.exe", args: "\"my file.txt\"")

[j8r]

The topic is likely wider than only just shell - revisiting Process.

On thing is for differentiate commands and arguments is separating them with a -- to prevent potential issues. Here is the on-windows friendly logic:

crystal/src/process.cr

Line 403 in 7e2e840

protected def self.prepare_args(command, args, shell)

[oprypin]

@j8r Sure, the topic is wide, but the behavior of the other aspects is obvious, and already implemented for a future PR.

I don't think -- is relevant to the discussion though.

[j8r]

Maybe, I don't know how if Windows disambiguate command and arguments?
Process is still very UNIX centric - fork, chroot, exec.

[j8r]

We could do something similar as System::User and System::Group - extending the module with Windows/UNIX specific logic depending of the platform - here constructing arguments to pass to the shell interpreter: prepare_args.

It may makes no sense to have both command, args and shell: true on Windows - but it has to be kept for consistency with other platforms which makes the difference.

[oprypin]

@j8r indeed. That is implemented in https://github.com/oprypin/crystal/compare/winapi

[HertzDevil]

Not invoking cmd.exe means that command prompt built-ins like echo and dir cannot be run by the backtick (unless the backtick itself calls cmd.exe). If we have plain shell scripts everywhere, e.g. all those pkg-config calls we have in the standard library, and those backticks call /bin/sh, I don't think Windows should be an exception here.

[oprypin]

POSIX has "the shell" (you just set a text file to be executable and the shell will be called) and Windows doesn't have "the shell".
You arbitrarily picked cmd and not PowerShell, how did you make that choice?

[HertzDevil]

%COMSPEC%

If we anticipate ` to be able to use an arbitrary "non-default" shell then POSIX shouldn't get a preferential treatment here.

[beta-ziliani]

I think it's perfectly sensible to use cmd.exe for now as "the shell" of Windows, in the same vein that we use /bin/bash elsewhere. If we find a reason to change that in the future, we can discuss that then, but it's better to have some sensible behavior to start with.

[lost22git]

• crystal v1.9.0 (Installed by scoop)
• win11
• npm (Installed by scoop)

Not sure if this is an unexpected behavior or it's by design ;-)

error: Unhandled exception: Error executing process: 'npm -v': The system cannot find the file specified. (File::NotFoundError)

from E:\scoop\global\apps\crystal\current\src\crystal\system\win32\process.cr:223 in 'spawn'
from E:\scoop\global\apps\crystal\current\src\process.cr:256 in 'initialize'
from E:\scoop\global\apps\crystal\current\src\process.cr:248 in 'new'

status = Process.run(command: "npm -v", shell: true)
p! status.success?

ok

status = Process.run(command: "cmd", args: ["-c","npm -v"])
p! status.success?

# or

status = Process.run(command: "powershell", args: ["-NoProfile", "-NoLogo", "npm -v"])
p! status.success?

[oprypin]

cmd has no concept of args, it must receive the whole command unquoted or else the quotes go to the command itself.

These two are equivalent and wrong because indeed rather than telling cmd to run npm -v, they tell it to run "npm -v"

Process.run("cmd", args: ["/c","npm -v"])
Process.run("cmd /c \"npm -v\"", shell: true)

The only correct way is

Process.run("cmd /c npm -v", shell: true)

[lost22git]

it seems to work ;-)

[lost22git]

I misunderstood that shell:true will automatically add sh -c or cmd /c to the command on different platforms