-
Notifications
You must be signed in to change notification settings - Fork 518
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.
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you show the expected difference between the old output and the new one? I'm having a bit of a hard time just tracing in my mind what the difference in output is expected to look like.
@@ -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"}, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Right, I forgot! Before:
After:
|
@@ -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"}, |
There was a problem hiding this comment.
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
This PR fixes a bug.
Description
When running
rebar3 compile
on a strange environment (Windows...), acmd
command in avsn
attribute in an.app.src
file failed.Expected behaviour
An error is prominently displayed.
Actual behaviour
The error was only available with
DEBUG=1
and localizing it to thevsn
attribute took quite some further digging.Additional info
While at it, make it so that
{abort_on_error, Msg}
ALSO shows the error that generated it, indiscriminately. This is a little breaking, but should be a good thing.I think that this PR is good enough as-is. I might have time to also add a test, if someone kindly points out specifically where.