Unify executable invocation

Add flags to rebar_utils:sh to control output and error handling.
Replace calls to os:cmd with calls to rebar_utils:sh.

src/rebar_ct.erl

@@ -71,7 +71,7 @@ run_test(TestDir, Config, _File) ->
             Output = " 2>&1 | tee -a " ++ RawLog
     end,
 
-    rebar_utils:sh(Cmd ++ Output, [{"TESTDIR", TestDir}]),
+    rebar_utils:sh(Cmd ++ Output, [{env,[{"TESTDIR", TestDir}]}]),
     check_log(RawLog).
 
 
@@ -89,7 +89,9 @@ clear_log(RawLog) ->
 %% calling ct with erl does not return non-zero on failure - have to check
 %% log results
 check_log(RawLog) ->
-    Msg = os:cmd("grep -e 'TEST COMPLETE' -e '{error,make_failed}' " ++ RawLog),
+    {ok, Msg} =
+        rebar_utils:sh("grep -e 'TEST COMPLETE' -e '{error,make_failed}' "
+                       ++ RawLog, [{use_stdout, false}]),
     MakeFailed = string:str(Msg, "{error,make_failed}") =/= 0,
     RunFailed = string:str(Msg, ", 0 failed") =:= 0,
     if

src/rebar_deps.erl

@@ -292,28 +292,31 @@ use_source(Dep, Count) ->
 
 download_source(AppDir, {hg, Url, Rev}) ->
     ok = filelib:ensure_dir(AppDir),
-    rebar_utils:sh(?FMT("hg clone -U ~s ~s", [Url, filename:basename(AppDir)]), [], filename:dirname(AppDir)),
-    rebar_utils:sh(?FMT("hg update ~s", [Rev]), [], AppDir);
+    rebar_utils:sh(?FMT("hg clone -U ~s ~s", [Url, filename:basename(AppDir)]),
+                   [{cd, filename:dirname(AppDir)}]),
+    rebar_utils:sh(?FMT("hg update ~s", [Rev]), [{cd, AppDir}]);
 download_source(AppDir, {git, Url, {branch, Branch}}) ->
     ok = filelib:ensure_dir(AppDir),
-    rebar_utils:sh(?FMT("git clone -n ~s ~s", [Url, filename:basename(AppDir)]), [], filename:dirname(AppDir)),
-    rebar_utils:sh(?FMT("git checkout -q origin/~s", [Branch]), [], AppDir);
+    rebar_utils:sh(?FMT("git clone -n ~s ~s", [Url, filename:basename(AppDir)]),
+                   [{cd, filename:dirname(AppDir)}]),
+    rebar_utils:sh(?FMT("git checkout -q origin/~s", [Branch]), [{cd, AppDir}]);
 download_source(AppDir, {git, Url, {tag, Tag}}) ->
     ok = filelib:ensure_dir(AppDir),
-    rebar_utils:sh(?FMT("git clone -n ~s ~s", [Url, filename:basename(AppDir)]), [], filename:dirname(AppDir)),
-    rebar_utils:sh(?FMT("git checkout -q ~s", [Tag]), [], AppDir);
+    rebar_utils:sh(?FMT("git clone -n ~s ~s", [Url, filename:basename(AppDir)]),
+                   [{cd, filename:dirname(AppDir)}]),
+    rebar_utils:sh(?FMT("git checkout -q ~s", [Tag]), [{cd, AppDir}]);
 download_source(AppDir, {git, Url, Rev}) ->
     download_source(AppDir, {git, Url, {branch, Rev}});
 download_source(AppDir, {bzr, Url, Rev}) ->
     ok = filelib:ensure_dir(AppDir),
     rebar_utils:sh(?FMT("bzr branch -r ~s ~s ~s",
-                        [Rev, Url, filename:basename(AppDir)]), [],
-                   filename:dirname(AppDir));
+                        [Rev, Url, filename:basename(AppDir)]),
+                   [{cd, filename:dirname(AppDir)}]);
 download_source(AppDir, {svn, Url, Rev}) ->
     ok = filelib:ensure_dir(AppDir),
     rebar_utils:sh(?FMT("svn checkout -r ~s ~s ~s",
-                        [Rev, Url, filename:basename(AppDir)]), [],
-                   filename:dirname(AppDir)).
+                        [Rev, Url, filename:basename(AppDir)]),
+                   [{cd, filename:dirname(AppDir)}]).
 
 update_source(Dep) ->
     %% It's possible when updating a source, that a given dep does not have a
@@ -333,19 +336,19 @@ update_source(Dep) ->
     end.
 
 update_source(AppDir, {git, _Url, {branch, Branch}}) ->
-    rebar_utils:sh(?FMT("git fetch origin", []), [], AppDir),
-    rebar_utils:sh(?FMT("git checkout -q origin/~s", [Branch]), [], AppDir);
+    rebar_utils:sh("git fetch origin", [{cd, AppDir}]),
+    rebar_utils:sh(?FMT("git checkout -q origin/~s", [Branch]), [{cd, AppDir}]);
 update_source(AppDir, {git, _Url, {tag, Tag}}) ->
-    rebar_utils:sh(?FMT("git fetch --tags origin", []), [], AppDir),
-    rebar_utils:sh(?FMT("git checkout -q ~s", [Tag]), [], AppDir);
+    rebar_utils:sh("git fetch --tags origin", [{cd, AppDir}]),
+    rebar_utils:sh(?FMT("git checkout -q ~s", [Tag]), [{cd, AppDir}]);
 update_source(AppDir, {git, Url, Refspec}) ->
     update_source(AppDir, {git, Url, {branch, Refspec}});
 update_source(AppDir, {svn, _Url, Rev}) ->
-    rebar_utils:sh(?FMT("svn up -r ~s", [Rev]), [], AppDir);
+    rebar_utils:sh(?FMT("svn up -r ~s", [Rev]), [{cd, AppDir}]);
 update_source(AppDir, {hg, _Url, Rev}) ->
-    rebar_utils:sh(?FMT("hg pull -u -r ~s", [Rev]), [], AppDir);
+    rebar_utils:sh(?FMT("hg pull -u -r ~s", [Rev]), [{cd, AppDir}]);
 update_source(AppDir, {bzr, _Url, Rev}) ->
-    rebar_utils:sh(?FMT("bzr update -r ~s", [Rev]), [], AppDir).
+    rebar_utils:sh(?FMT("bzr update -r ~s", [Rev]), [{cd, AppDir}]).
 
 
 
@@ -366,7 +369,8 @@ source_engine_avail({Name, _, _}=Source)
 scm_client_vsn(false, _VsnArg, _VsnRegex) ->
     false;
 scm_client_vsn(Path, VsnArg, VsnRegex) ->
-    Info = os:cmd("LANG=C " ++ Path ++ VsnArg),
+    {ok, Info} = rebar_utils:sh(Path ++ VsnArg, [{env, [{"LANG", "C"}]},
+                                                 {use_stdout, false}]),
     case re:run(Info, VsnRegex, [{capture, all_but_first, list}]) of
         {match, Match} ->
             list_to_tuple([list_to_integer(S) || S <- Match]);

src/rebar_erlydtl_compiler.erl

@@ -162,11 +162,16 @@ referenced_dtls1(Step, Config, Seen) ->
     DtlOpts = erlydtl_opts(Config),
     ExtMatch = re:replace(option(source_ext, DtlOpts), "\.", "\\\\\\\\.",
                           [{return, list}]),
-    AllRefs = lists:append(
-                [string:tokens(
-                   os:cmd(["grep -o [^\\\"]*",ExtMatch," ",F]),
-                   "\n")
-                 || F <- Step]),
+    AllRefs =
+        lists:append(
+          lists:map(
+            fun(F) ->
+                    {ok, Res} = rebar_utils:sh(
+                                  lists:flatten(["grep -o [^\\\"]*",
+                                                 ExtMatch," ",F]),
+                                  [{use_stdout, false}]),
+                    string:tokens(Res, "\n")
+            end, Step)),
     DocRoot = option(doc_root, DtlOpts),
     WithPaths = [ filename:join([DocRoot, F]) || F <- AllRefs ],
     Existing = [F || F <- WithPaths, filelib:is_file(F)],
@@ -176,4 +181,3 @@ referenced_dtls1(Step, Config, Seen) ->
         _ -> referenced_dtls1(sets:to_list(New), Config,
                               sets:union(New, Seen))
     end.
-

src/rebar_file_utils.erl

@@ -28,6 +28,7 @@
 
 -export([rm_rf/1,
          cp_r/2,
+         mv/2,
          delete_each/1]).
 
 -include("rebar.hrl").
@@ -42,25 +43,54 @@
 -spec rm_rf(Target::string()) -> ok.
 rm_rf(Target) ->
     case os:type() of
-        {unix,_} ->
-            [] = os:cmd(?FMT("rm -rf ~s", [Target])),
+        {unix, _} ->
+            {ok, []} = rebar_utils:sh(?FMT("rm -rf ~s", [Target]),
+                                      [{use_stdout, false}, return_on_error]),
             ok;
-        {win32,_} ->
-            ok = rm_rf_win32(Target)
+        {win32, _} ->
+            Filelist = filelib:wildcard(Target),
+            Dirs = lists:filter(fun filelib:is_dir/1,Filelist),
+            Files = lists:subtract(Filelist,Dirs),
+            ok = delete_each(Files),
+            ok = delete_each_dir_win32(Dirs),
+            ok
     end.
 
 -spec cp_r(Sources::list(string()), Dest::string()) -> ok.
 cp_r(Sources, Dest) ->
     case os:type() of
-        {unix,_} ->
+        {unix, _} ->
             SourceStr = string:join(Sources, " "),
-            [] = os:cmd(?FMT("cp -R ~s ~s", [SourceStr, Dest])),
+            {ok, []} = rebar_utils:sh(?FMT("cp -R ~s ~s", [SourceStr, Dest]),
+                                      [{use_stdout, false}, return_on_error]),
             ok;
-        {win32,_} ->
+        {win32, _} ->
             lists:foreach(fun(Src) -> ok = cp_r_win32(Src,Dest) end, Sources),
             ok
     end.
 
+-spec mv(Source::string(), Dest::string()) -> ok.
+mv(Source, Dest) ->
+    case os:type() of
+        {unix, _} ->
+            {ok, []} = rebar_utils:sh(?FMT("mv ~s ~s", [Source, Dest]),
+                                      [{use_stdout, false}, return_on_error]),
+            ok;
+        {win32, _} ->
+            {ok, R} = rebar_utils:sh(
+                        ?FMT("cmd " "/c move /y ~s ~s 1> nul",
+                             [filename:nativename(Source),
+                              filename:nativename(Dest)]),
+                        [{use_stdout, false}, return_on_error]),
+            case length(R) == 0 of
+                true -> ok;
+                false ->
+                    {error, lists:flatten(
+                              io_lib:format("Failed to move ~s to ~s~n",
+                                            [Source, Dest]))}
+            end
+    end.
+
 delete_each([]) ->
     ok;
 delete_each([File | Rest]) ->
@@ -78,30 +108,26 @@ delete_each([File | Rest]) ->
 %% Internal functions
 %% ===================================================================
 
-rm_rf_win32(Target) ->
-    Filelist = filelib:wildcard(Target),
-    Dirs = lists:filter(fun filelib:is_dir/1,Filelist),
-    Files = lists:subtract(Filelist,Dirs),
-    ok = delete_each(Files),
-    ok = delete_each_dir_win32(Dirs),
-    ok.
-
 delete_each_dir_win32([]) -> ok;
 delete_each_dir_win32([Dir | Rest]) ->
-    [] = os:cmd(?FMT("rd /q /s ~s", [filename:nativename(Dir)])),
+    {ok, []} = rebar_utils:sh(?FMT("cmd /c rd /q /s ~s",
+                                   [filename:nativename(Dir)]),
+                              [{use_stdout, false}, return_on_error]),
     delete_each_dir_win32(Rest).
 
 xcopy_win32(Source,Dest)->
-    R = os:cmd(?FMT("xcopy ~s ~s /q /y /e 2> nul",
-                    [filename:nativename(Source), filename:nativename(Dest)])),
-    case string:str(R,"\r\n") > 0 of
+    {ok, R} = rebar_utils:sh(
+                ?FMT("cmd /c xcopy ~s ~s /q /y /e 2> nul",
+                     [filename:nativename(Source), filename:nativename(Dest)]),
+                [{use_stdout, false}, return_on_error]),
+    case length(R) > 0 of
         %% when xcopy fails, stdout is empty and and error message is printed
         %% to stderr (which is redirected to nul)
         true -> ok;
         false ->
             {error, lists:flatten(
-                      io_lib:format("Failed to xcopy from ~s to ~s\n",
-                                     [Source, Dest]))}
+                      io_lib:format("Failed to xcopy from ~s to ~s~n",
+                                    [Source, Dest]))}
     end.
 
 cp_r_win32({true,SourceDir},{true,DestDir}) ->

src/rebar_neotoma_compiler.erl

@@ -121,11 +121,16 @@ referenced_pegs1(Step, Config, Seen) ->
     NeoOpts = neotoma_opts(Config),
     ExtMatch = re:replace(option(source_ext, NeoOpts), "\.", "\\\\\\\\.",
                           [{return, list}]),
-    AllRefs = lists:append(
-                [string:tokens(
-                   os:cmd(["grep -o [^\\\"]*",ExtMatch," ",F]),
-                   "\n")
-                 || F <- Step]),
+    AllRefs =
+        lists:append(
+          lists:map(
+            fun(F) ->
+                    {ok, Res} = rebar_utils:sh(
+                                  lists:flatten(["grep -o [^\\\"]*",
+                                                 ExtMatch," ",F]),
+                                  [{use_stdout, false}]),
+                    string:tokens(Res, "\n")
+            end, Step)),
     DocRoot = option(doc_root, NeoOpts),
     WithPaths = [ filename:join([DocRoot, F]) || F <- AllRefs ],
     Existing = [F || F <- WithPaths, filelib:is_file(F)],

src/rebar_port_compiler.erl

@@ -89,7 +89,7 @@ compile(Config, AppFile) ->
 
             %% One or more files are available for building. Run the pre-compile hook, if
             %% necessary.
-            run_precompile_hook(Config, Env),
+            ok = run_precompile_hook(Config, Env),
 
             %% Compile each of the sources
             {NewBins, ExistingBins} = compile_each(Sources, Config, Env, [], []),
@@ -103,8 +103,9 @@ compile(Config, AppFile) ->
             lists:foreach(fun({SoName,Bins}) ->
                 case needs_link(SoName, sets:to_list(sets:intersection([sets:from_list(Bins),sets:from_list(NewBins)]))) of
                   true ->
-                    rebar_utils:sh_failfast(?FMT("$CC ~s $LDFLAGS $DRV_LDFLAGS -o ~s",
-                                                  [string:join(Bins, " "), SoName]), Env);
+                    rebar_utils:sh(?FMT("$CC ~s $LDFLAGS $DRV_LDFLAGS -o ~s",
+                                        [string:join(Bins, " "), SoName]),
+                                   [{env, Env}]);
                   false ->
                     ?INFO("Skipping relink of ~s\n", [SoName]),
                     ok
@@ -148,7 +149,8 @@ run_precompile_hook(Config, Env) ->
             case filelib:is_regular(BypassFileName) of
                 false ->
                     ?CONSOLE("Running ~s\n", [Script]),
-                    rebar_utils:sh_failfast(Script, Env);
+                    {ok, _} = rebar_utils:sh(Script, [{env, Env}]),
+                    ok;
                 true ->
                     ?INFO("~s exists; not running ~s\n", [BypassFileName, Script])
             end
@@ -160,7 +162,8 @@ run_cleanup_hook(Config) ->
             ok;
         Script ->
             ?CONSOLE("Running ~s\n", [Script]),
-            rebar_utils:sh_failfast(Script, [])
+            {ok, _} = rebar_utils:sh(Script, []),
+            ok
     end.
 
 
@@ -174,11 +177,12 @@ compile_each([Source | Rest], Config, Env, NewBins, ExistingBins) ->
             ?CONSOLE("Compiling ~s\n", [Source]),
             case compiler(Ext) of
                 "$CC" ->
-                    rebar_utils:sh_failfast(?FMT("$CC -c $CFLAGS $DRV_CFLAGS ~s -o ~s",
-                                                 [Source, Bin]), Env);
+                    rebar_utils:sh(?FMT("$CC -c $CFLAGS $DRV_CFLAGS ~s -o ~s",
+                                        [Source, Bin]), [{env, Env}]);
                 "$CXX" ->
-                    rebar_utils:sh_failfast(?FMT("$CXX -c $CXXFLAGS $DRV_CFLAGS ~s -o ~s",
-                                                 [Source, Bin]), Env)
+                    rebar_utils:sh(
+                      ?FMT("$CXX -c $CXXFLAGS $DRV_CFLAGS ~s -o ~s",
+                           [Source, Bin]), [{env, Env}])
             end,
             compile_each(Rest, Config, Env, [Bin | NewBins], ExistingBins);
 

src/rebar_post_script.erl

@@ -51,5 +51,6 @@ execute_post_script(Config, Key) ->
         undefined ->
             ok;
         Script ->
-            rebar_utils:sh(Script, [])
+            {ok, _} = rebar_utils:sh(Script, []),
+            ok
     end.

src/rebar_pre_script.erl

@@ -51,5 +51,6 @@ execute_pre_script(Config, Key) ->
         undefined ->
             ok;
         Script ->
-            rebar_utils:sh(Script, [])
+            {ok, _} = rebar_utils:sh(Script, []),
+            ok
     end.

src/rebar_protobuffs_compiler.erl

@@ -104,9 +104,9 @@ compile_each([{Proto, Beam, Hrl} | Rest]) ->
                     %% into the ebin/ and include/ directories respectively
                     %% TODO: Protobuffs really needs to be better about this...sigh.
                     ok = filelib:ensure_dir(filename:join("ebin","dummy")),
-                    [] = os:cmd(?FMT("mv ~s ebin", [Beam])),
+                    ok = rebar_file_utils:mv(Beam, "ebin"),
                     ok = filelib:ensure_dir(filename:join("include", Hrl)),
-                    [] = os:cmd(?FMT("mv ~s include", [Hrl])),
+                    ok = rebar_file_utils:mv(Hrl, "include"),
                     ok;
                 Other ->
                     ?ERROR("Protobuff compile of ~s failed: ~p\n", [Proto, Other]),

src/rebar_utils.erl

@@ -30,8 +30,7 @@
          is_arch/1,
          get_arch/0,
          get_os/0,
-         sh/2, sh/3,
-         sh_failfast/2,
+         sh/2,
          find_files/2,
          now_str/0,
          ensure_dir/1,
@@ -73,36 +72,49 @@ get_os() ->
             ArchAtom
     end.
 
+%%
+%% Options = [Option] -- defaults to [use_stdout, abort_on_error]
+%% Option = ErrorOption | OutputOption | {cd, string()} | {env, Env}
+%% ErrorOption = return_on_error | abort_on_error | {abort_on_error, string()}
+%% OutputOption = use_stdout | {use_stdout, bool()}
+%% Env = [{string(), Val}]
+%% Val = string() | false
+%%
+sh(Command0, Options0) ->
+    ?INFO("sh: ~s\n~p\n", [Command0, Options0]),
 
-sh(Command, Env) ->
-    sh(Command, Env, get_cwd()).
+    DefaultOptions = [use_stdout, abort_on_error],
+    Options = lists:map(fun expand_sh_flag/1,
+                         proplists:compact(Options0 ++ DefaultOptions)),
 
-sh(Command0, Env, Dir) ->
-    ?INFO("sh: ~s\n~p\n", [Command0, Env]),
-    Command = patch_on_windows(Command0, os:type()),
-    Port = open_port({spawn, Command}, [{cd, Dir}, {env, Env}, exit_status, {line, 16384},
-                                        use_stdio, stderr_to_stdout]),
-    case sh_loop(Port) of
-        ok ->
-            ok;
+    ErrorHandler = proplists:get_value(error_handler, Options),
+    OutputHandler = proplists:get_value(output_handler, Options),
+
+    Command = patch_on_windows(Command0),
+    PortSettings = proplists:get_all_values(port_settings, Options) ++
+        [exit_status, {line, 16384}, use_stdio, stderr_to_stdout, hide],
+    Port = open_port({spawn, Command}, PortSettings),
+
+    case sh_loop(Port, OutputHandler, []) of
+        {ok, Output} ->
+            {ok, Output};
         {error, Rc} ->
-            ?ABORT("~s failed with error: ~w\n", [Command, Rc])
+            ErrorHandler(Command, Rc)
     end.
 
-
 %% We need a bash shell to execute on windows
 %% also the port doesn't seem to close from time to time (mingw)
-patch_on_windows(Cmd, {win32,nt}) ->
-    case find_executable("bash") of
-        false -> Cmd;
-        Bash -> 
-            Bash ++ " -c \"" ++ Cmd ++ "; echo _port_cmd_status_ $?\" ";
-    end;
-patch_on_windows(Command, _) ->;
-    Command.
-
-sh_failfast(Command, Env) ->
-    sh(Command, Env).
+patch_on_windows(Cmd) ->
+    case os:type() of
+        {win32,nt} ->;
+            case find_executable("bash") of
+                false -> Cmd;
+                Bash ->;
+                    Bash ++ " -c \"" ++ Cmd ++ "; echo _port_cmd_status_ $?\" ";
+            end;
+        _ ->
+            Cmd
+    end.
 
 find_files(Dir, Regex) ->
     filelib:fold_files(Dir, Regex, true, fun(F, Acc) -> [F | Acc] end, []).
@@ -162,19 +174,51 @@ match_first([{Regex, MatchValue} | Rest], Val) ->
            match_first(Rest, Val)
     end.
 
-sh_loop(Port) ->
+expand_sh_flag(return_on_error) ->
+    {error_handler,
+     fun(_Command, Rc) ->
+             {error, Rc}
+     end};
+expand_sh_flag({abort_on_error, Message}) ->
+    {error_handler,
+     fun(_Command, _Rc) ->
+             ?ABORT(Message, [])
+     end};
+expand_sh_flag(abort_on_error) ->
+    {error_handler,
+     fun(Command, Rc) ->
+             ?ABORT("~s failed with error: ~w\n", [Command, Rc])
+     end};
+expand_sh_flag(use_stdout) ->
+    {output_handler,
+     fun(Line, Acc) ->
+             ?CONSOLE("~s", [Line]),
+             [Acc | Line]
+     end};
+expand_sh_flag({use_stdout, false}) ->
+    {output_handler,
+     fun(Line, Acc) ->
+             [Acc | Line]
+     end};
+expand_sh_flag({cd, Dir}) ->
+    {port_settings, {cd, Dir}};
+expand_sh_flag({env, Env}) ->
+    {port_settings, {env, Env}}.
+
+sh_loop(Port, Fun, Acc) ->
     receive
         {Port, {data, {_, "_port_cmd_status_ " ++ Status}}} ->
             (catch erlang:port_close(Port)), % sigh () for indentation
             case list_to_integer(Status) of
-                0  -> ok;
+                0  -> {ok, lists:flatten(Acc)};
                 Rc -> {error, Rc}
             end;
-        {Port, {data, {_, Line}}} ->
-            ?CONSOLE("~s\n", [Line]),
-            sh_loop(Port);
+        {Port, {data, {eol, Line}}} ->
+            sh_loop(Port, Fun, Fun(Line ++ "\n", Acc));
+        {Port, {data, {noeol, Line}}} ->
+            sh_loop(Port, Fun, Fun(Line, Acc));
         {Port, {exit_status, 0}} ->
-            ok;
+            {ok, lists:flatten(Acc)};
         {Port, {exit_status, Rc}} ->
             {error, Rc}
     end.

test/rebar_file_utils_tests.erl

@@ -215,6 +215,18 @@ cp_r_overwrite_dir_fail_test_() ->
                       [filename:join([?TMP_DIR,"source"])],
                       filename:join([?TMP_DIR,"dest"])))]}.
 
+mv_file_test_() ->
+    {"move a file to folder",
+     setup,
+     fun() ->
+             setup(),
+             rebar_file_utils:mv(filename:join([?TMP_DIR,"source","file1"]),
+                                 filename:join([?TMP_DIR,"dest"]))
+     end,
+     fun teardown/1,
+     [?_assert(filelib:is_file(filename:join([?TMP_DIR,"dest","file1"]))),
+      ?_assertNot(filelib:is_file(filename:join([?TMP_DIR,"source","file1"])))]}.
+
 %% ====================================================================
 %% Utilities
 %% ====================================================================