Use System.cmd rather than System.shell for Rebar3 to avoid escaping (#13751)

Closes #13750.

Co-authored-by: Michael Goodyear <[email protected]>

Author: josevalim

lib/mix/lib/mix/dep/loader.ex

@@ -332,7 +332,7 @@ defmodule Mix.Dep.Loader do
   defp rebar_dep(dep, children, manager, locked?) do
     %Mix.Dep{app: app, opts: opts, extra: overrides} = dep
 
-    if locked? and is_nil(Mix.Rebar.rebar_cmd(manager)) do
+    if locked? and is_nil(Mix.Rebar.rebar_path(manager)) do
       Mix.Tasks.Local.Rebar.run(["--force"])
     end
 

lib/mix/lib/mix/rebar.ex

@@ -1,49 +1,56 @@
 defmodule Mix.Rebar do
   @moduledoc false
 
-  @doc """
-  Returns the path supposed to host the local copy of `rebar`.
+  # TODO: Remove on Elixir v1.20 because phx_new and other installers rely on it.
+  @deprecated "Use global_rebar_path/1 instead"
+  def global_rebar_cmd(manager) do
+    global_rebar_path(manager)
+  end
 
-  The rebar3 installation is specific to the Elixir version,
-  in order to force updates when new Elixir versions come out.
-  """
-  def local_rebar_path(:rebar3) do
-    [major, minor | _] = String.split(System.version(), ".")
-    Path.join([Mix.Utils.mix_home(), "elixir", "#{major}-#{minor}", "rebar3"])
+  @deprecated "Use local_rebar_path/1 instead"
+  def local_rebar_cmd(manager) do
+    local_rebar_path(manager)
+  end
+
+  @deprecated "Use rebar_path/1 instead"
+  def rebar_cmd(manager) do
+    global_rebar_cmd(manager) || local_rebar_cmd(manager)
   end
 
   @doc """
-  Returns the path to the global copy of `rebar` defined by the
-  environment variable `MIX_REBAR3`.
+  Receives a Rebar executable and returns how it must be invoked.
   """
-  def global_rebar_cmd(:rebar3) do
-    if cmd = System.get_env("MIX_REBAR3") do
-      wrap_cmd(cmd)
+  def rebar_args(rebar, args) do
+    if match?({:win32, _}, :os.type()) and not String.ends_with?(rebar, ".cmd") do
+      {"escript.exe", [rebar | args]}
+    else
+      {rebar, args}
     end
   end
 
   @doc """
-  Returns the path to the local copy of `rebar`, if one exists.
+  Returns either the global or local rebar path.
   """
-  def local_rebar_cmd(manager) do
-    cmd = local_rebar_path(manager)
-
-    if File.regular?(cmd) do
-      wrap_cmd(cmd)
-    end
+  def rebar_path(:rebar3) do
+    global_rebar_path(:rebar3) || local_rebar_path(:rebar3)
   end
 
   @doc """
-  Returns the path to the available `rebar` command.
+  Finds the global rebar path.
   """
-  # TODO: Remove on Elixir v1.20 because phx_new and other installers rely on it.
-  def rebar_cmd(:rebar) do
-    Mix.shell().error("[warning] :rebar is no longer supported in Mix, falling back to :rebar3")
-    rebar_cmd(:rebar3)
+  def global_rebar_path(:rebar3) do
+    System.get_env("MIX_REBAR3")
   end
 
-  def rebar_cmd(manager) do
-    global_rebar_cmd(manager) || local_rebar_cmd(manager)
+  @doc """
+  Returns the path supposed to host the local copy of `rebar`.
+
+  The rebar3 installation is specific to the Elixir version,
+  in order to force updates when new Elixir versions come out.
+  """
+  def local_rebar_path(:rebar3) do
+    [major, minor | _] = String.split(System.version(), ".")
+    Path.join([Mix.Utils.mix_home(), "elixir", "#{major}-#{minor}", "rebar3"])
   end
 
   @doc """
@@ -215,19 +222,6 @@ defmodule Mix.Rebar do
     end)
   end
 
-  defp wrap_cmd(rebar) do
-    cond do
-      not match?({:win32, _}, :os.type()) ->
-        String.replace(rebar, " ", "\\ ")
-
-      String.ends_with?(rebar, ".cmd") ->
-        "\"#{String.replace(rebar, "/", "\\")}\""
-
-      true ->
-        "escript.exe \"#{rebar}\""
-    end
-  end
-
   @doc """
   Applies the given overrides for app config.
   """

lib/mix/lib/mix/shell.ex

@@ -90,14 +90,15 @@ defmodule Mix.Shell do
   end
 
   @doc """
-  Executes the given `command` as a shell command and
-  invokes the `callback` for the streamed response.
+  Executes the given `command` and invokes the `callback` for the streamed response.
 
-  This is most commonly used by shell implementations
-  but can also be invoked directly.
+  `command` is either a string, to be executed as a `System.shell/2` command,
+  or a `{executable, args}` to be executed via `System.cmd/3`.
 
-  `callback` takes the output data of the command. Its
-  return value is ignored.
+  `callback` takes the output data of the command. Its return value is ignored.
+
+  This function is most commonly used by `Mix.Shell` implementations but can
+  also be invoked directly.
 
   ## Options
 
@@ -112,7 +113,8 @@ defmodule Mix.Shell do
     * `:quiet` - overrides the callback to no-op
 
   """
-  @spec cmd(String.t(), keyword, (binary -> term)) :: exit_status :: non_neg_integer
+  @spec cmd(String.t() | {String.t(), [String.t()]}, keyword, (binary -> term)) ::
+          exit_status :: non_neg_integer
   def cmd(command, options \\ [], callback) when is_function(callback, 1) do
     callback =
       if options[:quiet] do
@@ -129,7 +131,12 @@ defmodule Mix.Shell do
       |> Keyword.put(:into, %Mix.Shell{callback: callback})
       |> Keyword.put_new(:stderr_to_stdout, use_stdio)
 
-    {_, status} = System.shell(command, options)
+    {_, status} =
+      case command do
+        {command, args} -> System.cmd(command, args, options)
+        command when is_binary(command) -> System.shell(command, options)
+      end
+
     status
   end
 

lib/mix/lib/mix/tasks/deps.compile.ex

@@ -207,8 +207,15 @@ defmodule Mix.Tasks.Deps.Compile do
       {"TERM", "dumb"}
     ]
 
-    cmd = "#{rebar_cmd(dep)} bare compile --paths #{escape_path(lib_path)}"
-    do_command(dep, config, cmd, false, env)
+    rebar = Mix.Rebar.local_rebar_path(:rebar3) || handle_rebar_not_found(dep)
+    {exec, args} = Mix.Rebar.rebar_args(rebar, ["bare", "compile", "--paths", lib_path])
+
+    if Mix.shell().cmd({exec, args}, opts_for_cmd(dep, config, env)) != 0 do
+      Mix.raise(
+        "Could not compile dependency #{inspect(dep.app)}, \"#{Enum.join([exec | args], " ")}\" command failed. " <>
+          deps_compile_feedback(dep.app)
+      )
+    end
 
     # Check if we have any new symlinks after compilation
     for dir <- ~w(include priv src),
@@ -220,21 +227,12 @@ defmodule Mix.Tasks.Deps.Compile do
     true
   end
 
-  defp escape_path(path) do
-    escape = if match?({:win32, _}, :os.type()), do: "^ ", else: "\\ "
-    String.replace(path, " ", escape)
-  end
-
   defp rebar_config(dep) do
     dep.extra
     |> Mix.Rebar.dependency_config()
     |> Mix.Rebar.serialize_config()
   end
 
-  defp rebar_cmd(%Mix.Dep{manager: manager} = dep) do
-    Mix.Rebar.rebar_cmd(manager) || handle_rebar_not_found(dep)
-  end
-
   defp handle_rebar_not_found(%Mix.Dep{app: app, manager: manager}) do
     shell = Mix.shell()
 
@@ -255,12 +253,12 @@ defmodule Mix.Tasks.Deps.Compile do
     end
 
     Mix.Tasks.Local.Rebar.run(["--force"])
-    Mix.Rebar.local_rebar_cmd(manager) || Mix.raise("\"#{manager}\" installation failed")
+    Mix.Rebar.local_rebar_path(manager) || Mix.raise("\"#{manager}\" installation failed")
   end
 
   defp do_make(dep, config) do
     command = make_command(dep)
-    do_command(dep, config, command, true, [{"IS_DEP", "1"}])
+    shell_cmd!(dep, config, command, [{"IS_DEP", "1"}])
     build_structure(dep, config)
     true
   end
@@ -289,27 +287,29 @@ defmodule Mix.Tasks.Deps.Compile do
 
   defp do_compile(%Mix.Dep{opts: opts} = dep, config) do
     if command = opts[:compile] do
-      do_command(dep, config, command, true)
+      shell_cmd!(dep, config, command)
       build_structure(dep, config)
       true
     else
       false
     end
   end
 
-  defp do_command(dep, config, command, print_app?, env \\ []) do
-    %Mix.Dep{app: app, system_env: system_env, opts: opts} = dep
-
-    env = [{"ERL_LIBS", Path.join(config[:deps_build_path], "lib")} | system_env] ++ env
-
-    if Mix.shell().cmd(command, env: env, print_app: print_app?, cd: opts[:dest]) != 0 do
+  defp shell_cmd!(%Mix.Dep{app: app} = dep, config, command, env \\ []) do
+    if Mix.shell().cmd(command, [print_app: true] ++ opts_for_cmd(dep, config, env)) != 0 do
       Mix.raise(
         "Could not compile dependency #{inspect(app)}, \"#{command}\" command failed. " <>
           deps_compile_feedback(app)
       )
     end
 
-    true
+    :ok
+  end
+
+  defp opts_for_cmd(dep, config, env) do
+    %Mix.Dep{system_env: system_env, opts: opts} = dep
+    env = [{"ERL_LIBS", Path.join(config[:deps_build_path], "lib")} | system_env] ++ env
+    [env: env, cd: opts[:dest]]
   end
 
   defp build_structure(%Mix.Dep{opts: opts}, config) do

lib/mix/test/mix/rebar_test.exs

@@ -240,7 +240,7 @@ defmodule Mix.RebarTest do
       in_tmp("rebar3 env with spaces", fn ->
         File.cp!(Mix.Rebar.local_rebar_path(:rebar3), "rebar3")
         System.put_env("MIX_REBAR3", Path.absname("rebar3"))
-        assert Mix.Rebar.rebar_cmd(:rebar3) =~ " "
+        assert Mix.Rebar.rebar_path(:rebar3) =~ " "
 
         Mix.Project.push(RebarAsDep)
         Mix.Tasks.Deps.Get.run([])