State changes from provider hooks are ignored

[filmor]

The functions to call provider hooks don't return the state.

My use-case is my work on protocol consolidation in https://github.com/filmor/exerl/blob/main/src/exerl_prv_consolidate.erl. The process is essentially:

• Discover all protocol implementations in the code path of all project apps and deps
• Consolidate and place in some app's ebin (currently just the first project app)

The original protocol module and the consolidated "dispatch" module have the same name.

This works, so far. However, now I would like to add "seamless" release support. relx checks for duplicates (which errors on the protocol modules) but can be convinced to exclude modules. This requires an explicit configuration in the relx section of the rebar.config or a separate relx.config.

While I can modify the State in my provider, this has no effect if it's used as a hook: https://github.com/erlang/rebar3/blob/main/apps/rebar/src/rebar_hooks.erl#L20

Would you be open to a PR that adjusts this behaviour or is there a better way to achieve what I'm trying to do?

[tsloughter]

You can't use a provider hook? The other hooks are from rebar2 and shell out so can't update state anyway?

[filmor]

I do use a provider hook, but the result of its do/1 call is not passed through because the run_all_hooks/6 function doesn't return it. Thus, as far as I can tell, I can't modify the State to inject the exclude_modules configuration from within the provider hook.

[tsloughter]

oh, hm, maybe this is a bug. I'm not sure what the intention was.

[tsloughter]

This is definitely a good use case so I'd like to support it, we just need to consider the ramifications of modifying this :(

[ferd]

Digging into the history of it, https://github.com/erlang/rebar3/pull/695 seems to be the main issue. https://github.com/erlang/rebar3/pull/780 allowed to carry state in hooks if we wanted to.

Despite having that state already set, we never set any hook anywhere by default. The only place we started doing so was in some specific case-by-case basis, such as escripts.

One thing to be careful about though is that because generally hooks are not carrying state, there have been changes done in the past that specifically drop the input command from hooks (see source). As such, there's a chance that starting to carrying state in commands that tend to take arguments (such as releases or tarballs, but was put in to fix issues with test commands) could cause breakage in edge cases.

If we start passing state, we're probably going to need to store the command that was unset and start tracking it again, and risk breakage in setups where not passing state was load bearing somehow because the hook played with state a lot.

[ferd]

I'm noticing that there's also a distinction between hooks that run for each app in an umbrella, and hooks that are applied project-wide. Only the former currently have the ability to track their state. I don't necessarily have a good understanding of what would happen with project-wide hooks tracking and modifying state.

[filmor]

Yeah, and the per-app one is the one that is relevant here (and the far more complicated one :)).

I have already done some wprk in this direction, will push it this week to the draft PR.