Allow :nouse_stdio for System.shell

[saveman71]

I'd like to fire a command from Mix.shell that is interactive (it calls System.shell). I've seen https://elixirforum.com/t/launch-a-system-command-with-stdin-stdout-access/30201/7?u=saveman71, but wondered how much work would there be to bring this functionality.

I see arguments are handled here:

elixir/lib/elixir/lib/system.ex

Lines 1091 to 1113 in 81d6007

	defp cmd_opts([{:into, any} | t], opts, _into),
	do: cmd_opts(t, opts, any)
	defp cmd_opts([{:cd, bin} | t], opts, into) when is_binary(bin),
	do: cmd_opts(t, [{:cd, bin} | opts], into)
	defp cmd_opts([{:arg0, bin} | t], opts, into) when is_binary(bin),
	do: cmd_opts(t, [{:arg0, bin} | opts], into)
	defp cmd_opts([{:stderr_to_stdout, true} | t], opts, into),
	do: cmd_opts(t, [:stderr_to_stdout | opts], into)
	defp cmd_opts([{:stderr_to_stdout, false} | t], opts, into),
	do: cmd_opts(t, opts, into)
	defp cmd_opts([{:parallelism, bool} | t], opts, into) when is_boolean(bool),
	do: cmd_opts(t, [{:parallelism, bool} | opts], into)
	defp cmd_opts([{:env, enum} | t], opts, into),
	do: cmd_opts(t, [{:env, validate_env(enum)} | opts], into)
	defp cmd_opts([{key, val} | _], _opts, _into),
	do: raise(ArgumentError, "invalid option #{inspect(key)} with value #{inspect(val)}")

I see that here we basically forward a subset of options to Port (with some validation).

Would adding an nouse_stdio case be reasonable? use_stdio is a default, so we'd need to pop it from the default options.

Since that would almost cover every option in https://www.erlang.org/doc/man/erlang.html#open_port-2, maybe we should instead forward unknown options?

Thanks!

[josevalim]

I am not sure if the option would actually work since System.shell is invoking sh and that may not be happy with it. Please give it a try though and check if the behaviour makes sense.

[josevalim]

Ping!

[saveman71]

I had a try locally, and it works (at least for my use case). Since the introduced behavior only changes when the option is set, I'd say it's not a breaking change. Do you have other use cases in mind? Stuff that would fail?

From 9b9fbc91f6972251ac28dc4192d98d4c7bc61fd8 Mon Sep 17 00:00:00 2001
From: Antoine Bolvy <antoine.bolvy@gmail.com>
Date: Tue, 21 May 2024 10:33:29 +0200
Subject: [PATCH] Add support for no_usestdio option to System.shell and
 Mix.Shell.cmd

---
 lib/elixir/lib/system.ex |  8 ++++++++
 lib/mix/lib/mix/shell.ex | 10 +++++++---
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/lib/elixir/lib/system.ex b/lib/elixir/lib/system.ex
index 565be451b..2ba6fa1f0 100644
--- a/lib/elixir/lib/system.ex
+++ b/lib/elixir/lib/system.ex
@@ -1055,6 +1055,9 @@ defp do_shell(command, opts) do
 
     * `:stderr_to_stdout` - redirects stderr to stdout when `true`
 
+    * `:no_usestdio` - (since v1.????) disables stdio handling, leaving it to
+      the shell. Incompatible with `stderr_to_stdout`
+
     * `:parallelism` - when `true`, the VM will schedule port tasks to improve
       parallelism in the system. If set to `false`, the VM will try to perform
       commands immediately, improving latency at the expense of parallelism.
@@ -1179,6 +1182,11 @@ defp cmd_opts([{:stderr_to_stdout, true} | t], opts, into, line),
   defp cmd_opts([{:stderr_to_stdout, false} | t], opts, into, line),
     do: cmd_opts(t, opts, into, line)
 
+  defp cmd_opts([{:nouse_stdio, true} | t], opts, into, line) do
+    opts = opts -- [:use_stdio, :stderr_to_stdout]
+    cmd_opts(t, [:nouse_stdio | opts], into, line)
+  end
+
   defp cmd_opts([{:parallelism, bool} | t], opts, into, line) when is_boolean(bool),
     do: cmd_opts(t, [{:parallelism, bool} | opts], into, line)
 
diff --git a/lib/mix/lib/mix/shell.ex b/lib/mix/lib/mix/shell.ex
index 743d29d36..4cb2dda8c 100644
--- a/lib/mix/lib/mix/shell.ex
+++ b/lib/mix/lib/mix/shell.ex
@@ -103,7 +103,9 @@ def printable_app_name do
 
     * `:cd` *(since v1.11.0)* - the directory to run the command in
 
-    * `:stderr_to_stdout` - redirects stderr to stdout, defaults to true
+    * `:stderr_to_stdout` - redirects stderr to stdout, defaults to true, unless nouse_stdio is set
+
+    * `:nouse_stdio` - Do not use stdio for the command, allowing terminal input, defaults to false
 
     * `:env` - a list of environment variables, defaults to `[]`
 
@@ -119,11 +121,13 @@ def cmd(command, options \\ [], callback) when is_function(callback, 1) do
         callback
       end
 
+    use_stdio = !Keyword.get(options, :nouse_stdio, false)
+
     options =
       options
-      |> Keyword.take([:cd, :stderr_to_stdout, :env])
+      |> Keyword.take([:cd, :stderr_to_stdout, :env, :nouse_stdio])
       |> Keyword.put(:into, %Mix.Shell{callback: callback})
-      |> Keyword.put_new(:stderr_to_stdout, true)
+      |> Keyword.put_new(:stderr_to_stdout, use_stdio)
 
     {_, status} = System.shell(command, options)
     status
-- 
2.45.0

Let me know the next steps!

[saveman71]

A side note, I noted that even with hitting ^C twice, the background process wasn't killed / continued its happy life. I don't think this is something introduced by the patch since it does rigorously the same thing as the solution from @jayjun over at the elixirforums above (thought I'd CC you if you don't mind!)

If we go ahead, we might want to at least disclose that caveat.

[josevalim]

Can you please send a PR that adds use_stdio as a boolean flag? Thank you!

[saveman71]

So it would be true by default, setting it to false would toggle between use_stdio/nouse_stdio at the erlang options. Right?

[josevalim]

Closing in favor of PR.