Feature/fix crash on relup warning #559

lrascao · 2017-01-04T19:26:06Z

Addresses #554

sirihansen · 2017-01-10T10:51:58Z

src/rlx_prv_relup.erl

@@ -145,7 +155,14 @@ make_upfrom_script(State, Release, UpFrom) ->
                            "relup successfully created!"),
            {ok, State};
        {ok,_, Module,Warnings} ->
-            ?RLX_ERROR({relup_script_generation_warn, Module, Warnings});
+            case WarningsAsErrors of


I would suggest sending the warnings_as_errors option to systools:make_relup/4 instead of handling it here. Else, the relup will be generated anyway, even if this option is set.

i did as you suggested, yet i wasn't expecting for systools:make_relup to return simply error when i give it the warnings_as_errors option instead of {error,Module,Errors}, intended behaviour?

oh... that's unexpected indeed... this might be a bug. I will discuss it in the team if we should do anything about it. I guess you will have to decide for yourself what to do in the meantime, but a possible solution would be to do as your first implementation, but backup any old relup that might exists and restore it if it has be erroneously overwritten (or just delete the faulty generated relup if there was no file there already). Sorry for the hassle!!

i've tried this and apparently no relup is generated in this case so i guess aside from an unclear message there are no major side effects

Do you mean that systools does not write the relup if there are warnings, even if warnings_as_errors is not set?

warnings_as_errors is not set: relup is generated, systools:make_relup returns {ok,_, Module,Warnings} warnings are emitted

warnings_as_errors is set: relup is not generated, systools:make_relup returns error atom, in this case i would expect to receive {error,Module,Errors}, since no relup is generated there is no danger of overwriting a previous one

Ok, thanks - then we're talking about the same :) I need to look into the fact that systools:make_relup returns the error atom when warnings_as_errors is set - if it is a bug or not. Haven't had the time to do so yet. What I meant above was that maybe you need to do a workaround in the meantime - i.e. do as your original solution (when you didn't give the warnings_as_error option to systools, but handled it all in relx), but then add the cleanup if the relup is erroneously written. Unless you're happy with the error atom until further notice, that is...

FYI: I have written ticket for this issue (the error atom) to be considered at our next sprint planning in the beginning of February.

ah sorry, i see what you mean, that's a good idea, i've updated the pr accordingly, so now relx deletes the relup file if the `warnings_as_errors' option is set and there are warnings.

sirihansen · 2017-01-10T10:52:48Z

src/rlx_prv_relup.erl

+    "this case";
+format_error({relup_script_generation_warn, _Module, Warnings}) ->
+    ["Warnings generating relup \n",
+     rlx_util:indent(2), io_lib:format("~p", [Warnings])];


Could use Module:format_warning/1 here instead of io_lib:format

sirihansen · 2017-01-16T08:58:15Z

src/rlx_prv_relup.erl

+    "this case";
+format_error({relup_script_generation_warn, Module, Warnings}) ->
+    ["Warnings generating relup \n",
+     rlx_util:indent(2), Module:format_warning("~p", [Warnings])];


Module:format_warning takes only one argument - so it should be Module:format_warning(Warnings).

Obtained from command line and saved in the state to be used on situations where we want to error out on warnings explicitly.

sirihansen · 2017-01-30T12:30:21Z

src/rlx_prv_relup.erl

        {error,Module,Errors} ->
+            ec_cmd_log:warn(rlx_state:log(State),
+                            "error with warnings"),


What do you mean by "error with warnings"?

just forgotten debug messages, i've dropped them

sirihansen · 2017-01-31T12:10:30Z

I think this correction looks good now!

Two somehow related comments though:

since the options are hardcoded, I don't think that the two first clauses for the return value from systools are relevant. I.e. if you use option silent, you should never get just an ok or error (unless using warnings_as_errors, of course, but that's case is taken care of now).
you seem to write the relup file once more after receiving {ok,Relup,_,_} (write_relup_file/3). I see that it has been like this since the relup support was first added, and I guess it comes from the assumption that the relup file is not written when the Relup term i returned. This is the case when using option noexec, but not when using option silent.

Anyway, that was just some thoughts - no harm done, just some superfluous code...

Make use of warnings_on_errors option to exit with error or proceed with just a warning.

lrascao · 2017-02-01T23:40:19Z

yeah, my guess is that the silent option was added after all that logic was already in place

lrascao · 2017-02-01T23:40:33Z

@tsloughter this is ready for review

tsloughter · 2017-02-02T01:50:35Z

Looks good, but I'm a bit confused about the systools warnings_as_errors. Is there a change in the works that we know of? And it seems to be saying that if you pass it that it simply will return the atom error if there is a warning, and not the actual warnings/errors to display? Am I reading that comment correctly?

lrascao · 2017-02-02T10:28:16Z

Looks good, but I'm a bit confused about the systools warnings_as_errors. Is there a change in the works that we know of?

This is the first step, the next one would be extracting the warnings_as_errors option from rebar.config's erl_opts and passing it to relx

And it seems to be saying that if you pass it that it simply will return the atom error if there is a warning, and not the actual warnings/errors to display? Am I reading that comment correctly?

Yes, this is an issue we found in systools:make_relup, the least worst solution we found was handling warnings_as_errors in relxand deleting the relup that gets generated with the option is active

sirihansen · 2017-02-07T14:24:00Z

And it seems to be saying that if you pass it that it simply will return the atom error if there is a warning, and not the actual warnings/errors to display? Am I reading that comment correctly?

@lrascao @tsloughter A correction for this will be added in OTP-19.3, mid March.

lrascao mentioned this pull request Jan 4, 2017

Warning from systools:make_relup/4 crashes relx #554

Closed

sirihansen reviewed Jan 10, 2017

View reviewed changes

lrascao force-pushed the feature/fix_crash_on_relup_warning branch from af22f06 to 90d9030 Compare January 12, 2017 23:36

sirihansen reviewed Jan 16, 2017

View reviewed changes

lrascao force-pushed the feature/fix_crash_on_relup_warning branch from 90d9030 to db5e4f1 Compare January 22, 2017 00:32

Expose warnings as errors option

4314d77

Obtained from command line and saved in the state to be used on situations where we want to error out on warnings explicitly.

lrascao force-pushed the feature/fix_crash_on_relup_warning branch from db5e4f1 to 6ddc7e0 Compare January 28, 2017 00:44

sirihansen reviewed Jan 30, 2017

View reviewed changes

Don't crash on warnings emitted during relup

59ace4d

Make use of warnings_on_errors option to exit with error or proceed with just a warning.

lrascao force-pushed the feature/fix_crash_on_relup_warning branch from 6ddc7e0 to 59ace4d Compare February 1, 2017 23:19

tsloughter merged commit d3d9bc7 into erlware:master Feb 2, 2017

lrascao deleted the feature/fix_crash_on_relup_warning branch March 11, 2017 10:34

lrascao mentioned this pull request Mar 23, 2017

Let relx know about application erl opts erlang/rebar3#1520

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/fix crash on relup warning #559

Feature/fix crash on relup warning #559

lrascao commented Jan 4, 2017

sirihansen Jan 10, 2017

lrascao Jan 12, 2017

sirihansen Jan 16, 2017

lrascao Jan 22, 2017

sirihansen Jan 26, 2017

lrascao Jan 26, 2017 •

edited

Loading

sirihansen Jan 27, 2017

sirihansen Jan 27, 2017

lrascao Jan 28, 2017

sirihansen Jan 10, 2017

sirihansen Jan 16, 2017

sirihansen Jan 30, 2017

lrascao Feb 1, 2017

sirihansen commented Jan 31, 2017

lrascao commented Feb 1, 2017

lrascao commented Feb 1, 2017

tsloughter commented Feb 2, 2017

lrascao commented Feb 2, 2017

sirihansen commented Feb 7, 2017

Feature/fix crash on relup warning #559

Feature/fix crash on relup warning #559

Conversation

lrascao commented Jan 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrascao Jan 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirihansen commented Jan 31, 2017

lrascao commented Feb 1, 2017

lrascao commented Feb 1, 2017

tsloughter commented Feb 2, 2017

lrascao commented Feb 2, 2017

sirihansen commented Feb 7, 2017

lrascao Jan 26, 2017 •

edited

Loading