search

erlware / relx

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

Relx call to xref should take into account ignore_xref #872

Closed acw224 closed 3 years ago

acw224 commented 3 years ago

relx calls xref in rlx_assemble.erl

This one should honour the -xref_ignores from rebar3, and the ones defined at the module level (-ignore_xref(..)).

One way to do it is to have move the code: https://github.com/erlang/rebar3/blob/master/src/rebar_prv_xref.erl#L147 from rebar to relx.

Another way is to prevent relx from printing xref warning: https://github.com/erlware/relx/blob/master/src/rlx_assemble.erl#L737 and instead pass them to the rebar3 plugin, who can then use the code in rebar_prv_xref.erl as a dependency.

What do you think ?

acw224 commented 3 years ago

Some update: https://github.com/erlang/rebar3/pull/2580 https://github.com/erlware/relx/pull/874

ferd commented 3 years ago

We've communicated by e-mail before on this. In short, we think this is a good idea and something that does indeed need fixing, and greatly appreciate the PRs.

The one bit we're not sure of is the division of responsibility between Relx and Rebar3 in terms of policies and components. The one bit we think needs changing is that we're passing the warning through the Relx state, which sort of piggy-backs on implementation details.

What we think should happen instead is this:

  1. extend relx state to accept in a filter function for xref. Make it default to fun(_) -> true end if unspecified
  2. keep the relx state weaving the way you build it here, but apply the filter before printing the way it was already printing (within relx). We can at this point merge the Relx PR fine independently, which speaks to the isolation we get
  3. adjust the rebar3 PR to take the adjusted filtering function you already wrote, but wrap it in fun(Warn) -> YourCode end and set it in the relx state. You should use a closure to carry whatever rebar3 state you need without relx knowing a thing about the details
  4. The xref filtering is now configured by rebar3 knowing its config state but handled by Relx as if nothing special took place. We're essentially using this to tunnel rebar3's config into the relx state to apply it to the xref warnings, rather than tunneling the xref warnings back to rebar3.

Do you think this make sense? It sounds like it shouldn't be too much of a rewrite since many of the components can be kept and shifted around instead.

acw224 commented 3 years ago

Thanks for the feedback.

I was able to carry the required modifications pretty easily, except for one little detail. The method filter_xref_results in rebar3 https://github.com/erlang/rebar3/blob/3169392c7d73e6b849fb3117fb76177478c85f38/src/rebar_prv_xref.erl#L183 is a bit more involved than being just a predicate on list.

Therefore, I found it a bit easier to pass the whole filter method as a fun(list()) -> list(), which default to the identity.

acw224 commented 3 years ago

On Relx side https://github.com/erlware/relx/pull/882 On rebar3 side https://github.com/erlang/rebar3/pull/2611

tsloughter commented 3 years ago

Resolved on the relx side in #882