Skip to content
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

State changes from provider hooks are ignored #2914

Open
filmor opened this issue Sep 16, 2024 · 7 comments · May be fixed by #2915
Open

State changes from provider hooks are ignored #2914

filmor opened this issue Sep 16, 2024 · 7 comments · May be fixed by #2915

Comments

@filmor
Copy link
Contributor

filmor commented Sep 16, 2024

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
Copy link
Collaborator

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

@filmor
Copy link
Contributor Author

filmor commented Sep 16, 2024

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
Copy link
Collaborator

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

@tsloughter
Copy link
Collaborator

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

@filmor filmor linked a pull request Sep 16, 2024 that will close this issue
@ferd
Copy link
Collaborator

ferd commented Sep 17, 2024

Digging into the history of it, #695 seems to be the main issue. #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
Copy link
Collaborator

ferd commented Sep 17, 2024

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
Copy link
Contributor Author

filmor commented Sep 17, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants