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.

Sign up for GitHub

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

allow use deps on ERL_LIBS in escriptize #2552

Merged
merged 1 commit into from
May 17, 2021
Merged

allow use deps on ERL_LIBS in escriptize #2552

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion src/rebar_prv_escriptize.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,10 @@ find_deps_of_deps([Name|Names], Apps, Acc) ->
?DIAGNOSTIC("processing ~p", [Name]),
App = case rebar_app_utils:find(Name, Apps) of
{ok, Found} -> Found;
error -> throw(?PRV_ERROR({bad_app, binary_to_atom(Name, utf8)}))
error -> case find_external_app(Name) of
error -> throw(?PRV_ERROR({bad_app, binary_to_atom(Name, utf8)}));
App0 -> App0
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was about to merge this and I had to do a second take. The apps that are being resolved here are apps that have been declared as part of your project, as defined by rebar_state:all_deps(State)++rebar_state:project_apps(State) (line 120). These should actually never contain anything in ERL_LIBS because they're built dynamically from what fits within rebar.config and is resolved recursively.

You can actually see the resolving of code:lib_dir/1 dependencies below on line 276 (279 in your patch). Those are dependencies that should be declared in the .app or .app.src files, but not within rebar.config, since rebar.config assumes it's fetching all the deps declared there.

I believe you see the issue you're having here because there's a disconnection between how you're using the dependencies in rebar3's config (for them to be fetched) and where they're supposed to be used for runtime dependencies (in .app or .app.src files).

I believe this PR is the wrong fix for your problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ferd, the code on line 278 (281 here) skips dependencies which are in code:root_dir(), ie. part of OTP, but not apps which are in other directories but still on ERL_LIBS. These are included in BinDepNames but the next call to find_deps_of_deps fails since these deps are not in rebar_state:all_deps(State)++rebar_state:project_apps(State) (I haven't added them to rebar.config).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm yeah, the recursive call is re-adding them there, and it makes sense at that point. Yeah the fix would make sense then since the input set is no longer the rebar.config apps anymore. My bad for the bad code reading.

end,
DepNames = proplists:get_value(applications, rebar_app_info:app_details(App), []),
BinDepNames = [rebar_utils:to_binary(Dep) || Dep <- DepNames,
Expand All @@ -280,6 +283,16 @@ find_deps_of_deps([Name|Names], Apps, Acc) ->
?DIAGNOSTIC("new deps of ~p found to be ~p", [Name, BinDepNames]),
find_deps_of_deps(BinDepNames ++ Names, Apps, BinDepNames ++ Acc).

%% This is for apps which are on the code path (ERL_LIBS) but not part of OTP
find_external_app(Name) ->
case code:lib_dir(binary_to_atom(Name, utf8)) of
{error, bad_name} -> error;
Dir -> case rebar_app_discover:find_app(Dir, valid) of
{true, App} -> App;
false -> error
end
end.

def(Rm, State, Key, Default) ->
Value0 = rebar_state:get(State, Key, Default),
case Rm of
Expand Down