rebar3 cmd <comand> : return code fails to indicate command failure status about rebar_cmd HOT 8 OPEN

Thank you for the report @oscarlamasrios , I haven't updated this library in a long time. I will try to look for a solution soon, sorry if it will take some time.

@paulo-ferraz-oliveira perhaps you have some time since you did some of the work?

from rebar_cmd.

I don't have a lot of time "now", but I'll keep this in my GitHub notifications' list to maybe handle in the future (I'll soon probably start using this plugin more). 👍 @oscarlamasrios, do you have any idea, in the code, where the exit status is not handled properly?

from rebar_cmd.

Thank you for your prompt response! I appreciate your willingness to look into this.
After conducting some small-scale tests, I've observed that the issue appears to be related to the usage of os:cmd/1. Specifically, when I run the following commands, the second one consistently returns a return code of 0:

❯ os:cmd(Cmd).
 . . . . 
❯ os:cmd("echo $?").
0

Based on this, I suspect that the problem lies in the use of this library instead of open_port. I noticed that open_port was used previously and it might be worth considering if it's beneficial to switch back.

Thanks a lot @gootik @paulo-ferraz-oliveira 😄

from rebar_cmd.

In case it might be of help, I had started making some modifications using open_port. Specifically, I made adjustments to the functions exec/3 and handle_cmd/3.

exec(CmdName, Cmd, Opts) ->
    Pid = self(),
    Ref = erlang:make_ref(),
    Timeout = get_opt(timeout, Opts, CmdName),
    {MonitorPid, MonitorRef} = erlang:spawn_monitor(
        fun() ->
            not is_list(Cmd) andalso exit("call not expressed as a string"),
            rebar_api:debug("Command ~s executing ~s with options ~p.", [CmdName, Cmd, Opts]),
            {ExitCode, Result} = exec_open_port(Cmd),
            Pid ! {Ref, Result, ExitCode}
        end
    ),
    receive
        {Ref, Result, RetCode} ->
            {ok, {CmdName, Result, RetCode}};
        {'DOWN', MonitorRef, _Type, _Object, Info} ->
            {ok, {{CmdName, Info}, died}}
    after Timeout ->
        exit(MonitorPid, kill),
        TimeOutS = integer_to_list(Timeout),
        {error, "Command " ++ CmdName ++ " (" ++ Cmd ++ ") timed out after " ++ TimeOutS ++ " ms."}
    end.
    
exec_open_port(Command) ->
    Port = open_port({spawn, Command}, [stream, in, eof, hide, exit_status]),
    {ExitCode, Output} = get_data(Port, [], []),
    {ExitCode, Output}.

get_data(Port, Output, ExitCode) ->
    receive
        {Port, {data, Bytes}} ->
            get_data(Port, [Output|Bytes], ExitCode);
        {Port, eof} ->
            Port ! {self(), close},
            receive
                {Port, closed} ->
                    true
            end,
            receive
                {'EXIT',  Port,  _} ->
                    ok
            after 1 ->
                ok
            end,
            NewExitCode = receive
                {Port, {exit_status, Code}} ->
                    Code
            end,
            {NewExitCode, lists:flatten(lists:reverse(Output))}
    end.
    
handle_cmd({error, Error}, _Opts, _State) ->
    {error, Error};
handle_cmd({ok, {{CmdName, Info}, died}}, _Opts, State) ->
    rebar_api:error("Command ~s died unexpectedly with ~p.", [CmdName, Info]),
    {ok, State};
handle_cmd({ok, {CmdName, RawResult, RetCode}}, Opts, State) ->
    Result = string:trim(RawResult),
    case RetCode of
        0 ->
            case get_opt(verbose, Opts, CmdName) of
                true ->
                    rebar_api:debug("Command ~s finished", [CmdName]),
                    rebar_api:console("~ts", [Result]);
                false ->
                    rebar_api:debug("Command ~s finished", [CmdName]),
                    rebar_api:debug("Command ~s result: ~ts", [CmdName, Result])
            end,
            {ok, State};
        _ ->
            rebar_api:error("Command ~s failed with return code ~s.", [CmdName, integer_to_list(RetCode)]),
            {error, State}
    end.

from rebar_cmd.

the problem lies in the use of this library

By this library, do you mean Erlang's os module? It's part of OTP, not external.

from rebar_cmd.

os:cmd was introduced in #23, by me, as a means to (IIRC) solve some issues that Erlang ports presented.

I'd feel more comfortable accepting a change (to go back) if:

1. we identify what os:cmd is solving and write tests for that
2. we make sure ports can "solve" what os:cmd is supposed to
3. we take #22 (comment) into consideration (we've switched once, and this was accepted, so going back might have unintended consequences)

If there's a good identification of the library's use cases, via tests and proper documentation, I don't see why it couldn't use ports instead (mind you, at this moment, I'm only talking about changes conceptually - I'd have to re-read what're the main differences between ports and os:cmd to make a proper informed decision).

from rebar_cmd.

Of course, I agree with what you're saying.
I just wanted to leave what I had been working on in case it could be of any help, that's all!
I haven't conducted an in-depth investigation, so I can't provide a definitive assessment of the challenges associated with reverting to using Erlang ports. It seems quite crucial to me that rebar3 cmd <command> can be used in GitHub checks, so I just wanted to update you on the issue I'm facing. Perhaps there's a way to ensure the correct return code is returned while still using the os:cmd library.

Thanks!

from rebar_cmd.

Perhaps there's a way to ensure the correct return code is returned while still using the os:cmd library.

I can imagine there'd be lots of workarounds (not saying I'm a fan of this approach), from writing to files and checking those to actually calling "echo $?", on the command, and returning that, ...

At this time, rebar_cmd makes no promises on delivering either output or a status code, it's more like a "fire and forget" thing. So maybe it is possible to change it to something else, and e.g. have a correct format on output that can be parseable (?) I can't speak for the original "requirements"; I wasn't here at conception, was just briefly an interested contributor, in the past...

from rebar_cmd.