search

erlware / relx

Sane, simple release creation for Erlang
http://erlware.github.io/relx
Apache License 2.0
697 stars 232 forks source link

Feature/fix crash on relup warning #559

Closed lrascao closed 7 years ago

lrascao commented 7 years ago

Addresses #554

sirihansen commented 7 years ago

I think this correction looks good now!

Two somehow related comments though:

  1. 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).

  2. 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...

lrascao commented 7 years ago

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

lrascao commented 7 years ago

@tsloughter this is ready for review

tsloughter commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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.