Skip to content
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

Fail loudly on error when executing a cmd in an app.src vsn #2543

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fail loudly on error when executing a cmd in an app.src vsn #2543

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cf1c49d
Also show error on {abort_on_error, Msg}
aronisstav Apr 22, 2021
3944acc
Fail loudly when app src vsn cmd fails
aronisstav Apr 22, 2021
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
21 changes: 14 additions & 7 deletions src/rebar_utils.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,10 +654,18 @@ expand_sh_flag({env, _EnvArg} = Env) ->
-type err_handler() :: fun((string(), {integer(), string()}) -> no_return()).
-spec log_msg_and_abort(string()) -> err_handler().
log_msg_and_abort(Message) ->
fun(_Command, {_Rc, _Output}) ->
?ABORT(Message, [])
fun(Command, Arg) ->
Msg = io_lib:format("~s~n", [Message]),
log_msg_and_abort(Msg, Command, Arg)
end.

-spec log_msg_and_abort(string(), string(), {integer(), string()}) -> no_return().
log_msg_and_abort(Message, Command, {Rc, Output}) ->
?ABORT("~s"
"sh(~ts)~n"
"failed with return code ~w and the following output:~n"
"~ts", [Message, Command, Rc, Output]).

-spec debug_log_msg_and_abort(string()) -> err_handler().
debug_log_msg_and_abort(Message) ->
fun(Command, {Rc, Output}) ->
Expand All @@ -668,10 +676,8 @@ debug_log_msg_and_abort(Message) ->
end.

-spec log_and_abort(string(), {integer(), string()}) -> no_return().
log_and_abort(Command, {Rc, Output}) ->
?ABORT("sh(~ts)~n"
"failed with return code ~w and the following output:~n"
"~ts", [Command, Rc, Output]).
log_and_abort(Command, Arg) ->
log_msg_and_abort("", Command, Arg).

-spec debug_and_abort(string(), {integer(), string()}) -> no_return().
debug_and_abort(Command, {Rc, Output}) ->
Expand Down Expand Up @@ -792,7 +798,8 @@ vcs_vsn_cmd(_, _, _) ->
unknown.

cmd_vsn_invoke(Cmd, Dir) ->
{ok, VsnString} = rebar_utils:sh(Cmd, [{cd, Dir}, {use_stdout, false}]),
ErrorOpt = {abort_on_error, "vsn cmd in .app.src failed"},
Copy link
Collaborator

Choose a reason for hiding this comment

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

if the error message takes place this deep, then it would make sense not to make assumptions on what the caller was doing ("was it really called in a .app.src file?") and simply report the error at its own level (something like "Command x failed") to prevent misguiding error messages when the callsites are expanded.

If we want to provide contextual errors with high-level concerns, then we would need to put in a conditional here and return {ok, VsnString} | {error, Term} and move the reporting to the parent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you clarify a little bit? I am guessing you mean that this code could be reached in different ways, but my understanding, without really checking, is that the only way to reach cmd_vsn_invoke/2 is when really evaluating a cmd in a vsn property, in an .app.src file, which is what my error message reports.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah this is more of future-proofing. I.e. the function cmd_vsn_invoke knows it is about a version, but has no idea it actually comes from a .app.src file.

Copy link

Choose a reason for hiding this comment

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

It would be good to add the actual filename or path here as well. Otherwise, if you have many apps its up to the user to find the one with the bad vsn command

{ok, VsnString} = rebar_utils:sh(Cmd, [ErrorOpt, {cd, Dir}, {use_stdout, false}]),
rebar_string:trim(VsnString, trailing, "\n").

%% @doc ident to the level specified
Expand Down