Skip to content

Add support for use_stdio option to System.shell and Mix.Shell.cmd #13580

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
May 24, 2024
Merged

Add support for use_stdio option to System.shell and Mix.Shell.cmd #13580

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
262d641
Add support for use_stdio option to System.shell and Mix.Shell.cmd
saveman71 May 21, 2024
cf01d21
Handle `use_stdio: true` and add tests
saveman71 May 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion lib/elixir/lib/system.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1053,7 +1053,11 @@ defmodule System do

* `:arg0` - sets the command arg0

* `:stderr_to_stdout` - redirects stderr to stdout when `true`
* `:stderr_to_stdout` - redirects stderr to stdout when `true`, no effect
if `use_stdio` is `false``.

* `:use_stdio` - `true` by default, setting it to false allows direct
interaction with the terminal from the callee

* `: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
Expand Down Expand Up @@ -1179,6 +1183,11 @@ defmodule System do
defp cmd_opts([{:stderr_to_stdout, false} | t], opts, into, line),
do: cmd_opts(t, opts, into, line)

defp cmd_opts([{:use_stdio, false} | t], opts, into, line) do
opts = opts -- [:use_stdio, :stderr_to_stdout]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot remove stderr_to_stdout after because there is no guarantee it was set before. For example, someone can do use_stdio: false, stderr_to_stdout: true and now it won't be removed. We should also support use_stdio: true and we can add a test for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've come up with something. However not very happy with the tests - they're on unix only for one - but obviously detaching the IO means we don't capture it anymore so it logs a bit on the console.

Copy link
Contributor Author

@saveman71 saveman71 May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(note about the use_stdio: false, stderr_to_stdout: true case, we could just forward that to erlang and it'll raise, even though I added an extra check at the end for that to log our own message)

     expected:
       ~r/cannot use `stderr_to_stdout: true` and `use_stdio: false`/
     actual:
       "errors were found at the given arguments:\n\n  * 2nd argument: invalid option in list\n"

arguably it's less clear though

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)

Expand Down
10 changes: 7 additions & 3 deletions lib/mix/lib/mix/shell.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,9 @@ defmodule Mix.Shell 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 use_stdio is set to false

* `:use_stdio` - controls whether the command should use stdin / stdout / stdrr, defaults to true

* `:env` - a list of environment variables, defaults to `[]`

Expand All @@ -119,11 +121,13 @@ defmodule Mix.Shell do
callback
end

use_stdio = Keyword.get(options, :use_stdio, true)

options =
options
|> Keyword.take([:cd, :stderr_to_stdout, :env])
|> Keyword.take([:cd, :stderr_to_stdout, :env, :use_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
Expand Down