Env for providers #1706
Env for providers #1706
Conversation
|
oops, messed up the tests, gotta fix that. |
cjkjellander
force-pushed
the
env-for-providers
branch
from
d1e7721 to
dbc9372
Compare
|
@cjkjellander sorry for the long delay. Do you think you could rebase this PR off master so the review is simpler to track through? |
|
of course! |
cjkjellander
force-pushed
the
env-for-providers
branch
from
dbc9372 to
e81daeb
Compare
| @@ -453,6 +453,24 @@ reread_config(ConfigList) -> | |||
| "and will be ignored.", []) | |||
| end. | |||
|
|
|||
| %% @doc Given env. variable `FOO' we want to expand all references to | |||
There was a problem hiding this comment.
Has this function just been moved from internal to non-internal code sections without having been exported?
There was a problem hiding this comment.
Nevermind, just saw it was already exported without having previously been moved.
There was a problem hiding this comment.
Yeah, that confused me as well, that was the reason for the failing tests above. I exported it but it already was exported it turns out.
src/rebar_hooks.erl
Outdated
| Env = create_env(State, Opts), | ||
| State2 = rebar_state:env(State1, Env), | ||
| run_provider_hooks_(Dir, Type, Command, Providers, | ||
| TypeHooks, State2) |
There was a problem hiding this comment.
I'm not sure what this provides here. So we create the ENV and store it in the provider? The bug as illustrated at blt/port_compiler#30 seems to be that https://github.com/blt/port_compiler/pull/38/files#diff-190510d6d34e2aff57877104e5ec0246R152 uses the ENV values to just expand the ENV themselves.
Is there a reason why we couldn't just expose create_env as a function instead (maybe stick it in a rebar_env module or something like that) and let the plugin call it and expand it however it wants based on the current opts and config? It sounds like it would be far less risky than having to track and update the env value in the state record along with everything else if we make it so whoever needs it generates it on-demand.
cjkjellander
force-pushed
the
env-for-providers
branch
from
e81daeb to
6d29233
Compare
|
ok, fixed it according to review. |
|
I'm not so sure about this solution. Does the providers have access to Opts? I'm trying to reimplement the fix blt/port_compiler#30 and the Opts in there isn't even a dict. Gotta read some code.... The two ENVs that need Opts is |
|
Opts can be two possible structures depending on who calls it (legacy leftovers!) but usually rebar_opts module allows transparent usage there. |
|
I'm doing some cleaning up. I'll be done in 10 minutes. |
|
There. I've completed the port_compile part to use this new interface and it seems to work both as a top level app and a dependency so now providers should be able to get the env and use them in specs and when using external programs. |
|
btw, this is the original repo that we were having trouble converting to rebar3: https://github.com/campanja/csv/tree/rebar3-build |
|
Yeah that sounds like a much more manageable change from the rebar3 perspective. Merging. |
This adds and exports the necessary parts to use env variables like REBAR_DEPS_DIR inside provider hooks. It addresses this issue in port_compiler: blt/port_compiler#30
There will be a separate pull request in that repo as well.
This branch is based off of #1698 and should not be merged before that one. But in order to use env variables in port_specs of deps, you actually have to run the port_hooks for the deps. So this depends on the mentioned pull request.