From 9d788d893620f868b9c0ee00ddec8ae4d5d8fea7 Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Fri, 5 Oct 2018 07:57:20 -0400 Subject: Abstracted path management Move path management out of rebar_utils manual code path function handling (which we leave there for backwards compat), and centralize them to allow easier coordination of paths between plugins and deps. On top of path handling, do a check of loaded modules to only purge and reload those that actually need it done in order to prevent all kinds of weird interaction and accidental purge kills. It also allows the possible cohabitation of both at once, with a "in case of conflict pick X" as a policy Changing path handling in providers also highlighted a bunch of bugs in some tests and appears to fix some in other providers, specifically around plugins. --- src/rebar_paths.erl | 177 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 177 insertions(+) create mode 100644 src/rebar_paths.erl (limited to 'src/rebar_paths.erl') diff --git a/src/rebar_paths.erl b/src/rebar_paths.erl new file mode 100644 index 0000000..bb43897 --- /dev/null +++ b/src/rebar_paths.erl @@ -0,0 +1,177 @@ +-module(rebar_paths). +-include("rebar.hrl"). + +-type target() :: deps | plugins. +-type targets() :: [target(), ...]. +-export_type([target/0, targets/0]). +-export([set_paths/2, unset_paths/2]). + +-spec set_paths(targets(), rebar_state:t()) -> ok. +set_paths(UserTargets, State) -> + Targets = normalize_targets(UserTargets), + GroupPaths = path_groups(Targets, State), + Paths = lists:append([P || {_, P} <- GroupPaths]), + [code:del_path(P) || P <- Paths], + code:add_pathsa(lists:reverse(Paths)), + % set path breaks with escripts + %true = code:set_path(lists:append([P || {_, P} <- GroupPaths])), + AppGroups = app_groups(Targets, State), + purge_and_load(AppGroups, code:all_loaded(), sets:new()), + ok. + +-spec unset_paths(targets(), rebar_state:t()) -> ok. +unset_paths(UserTargets, State) -> + Targets = normalize_targets(UserTargets), + GroupPaths = path_groups(Targets, State), + Paths = lists:append([P || {_, P} <- GroupPaths]), + [code:del_path(P) || P <- Paths], + purge(Paths, code:all_loaded()), + ok. + + +%% The paths are to be set in the reverse order; i.e. the default +%% path is always last when possible (minimize cases where a build +%% tool version clashes with an app's), and put the highest priorities +%% first. +-spec normalize_targets(targets()) -> targets(). +normalize_targets(List) -> + %% Plan for the eventuality of getting values piped in + %% from future versions of rebar3, possibly from plugins and so on, + %% which means we'd risk failing kind of violently. We only support + %% deps and plugins + TmpList = lists:foldl( + fun(deps, [deps | _] = Acc) -> Acc; + (plugins, [plugins | _] = Acc) -> Acc; + (deps, Acc) -> [deps | Acc -- [deps]]; + (plugins, Acc) -> [plugins | Acc -- [plugins]]; + (_, Acc) -> Acc + end, + [], + List + ), + lists:reverse(TmpList). + +purge_and_load([], _, _) -> + ok; +purge_and_load([{_Group, Apps}|Rest], ModPaths, Seen) -> + %% We have: a list of all applications in the current priority group, + %% a list of all loaded modules with their active path, and a list of + %% seen applications. + %% + %% We do the following: + %% 1. identify the apps that have not been solved yet + %% 2. find the paths for all apps in the current group + %% 3. unload and reload apps that may have changed paths in order + %% to get updated module lists and specs + %% (we ignore started apps and apps that have not run for this) + %% 4. create a list of modules to check from that app list + %% 5. check the modules to match their currently loaded paths with + %% the path set from the apps in the current group; modules + %% that differ must be purged; others can stay + + %% 1) + AppNames = [AppName || App <- Apps, + AppName <- [rebar_app_info:name(App)], + not sets:is_element(AppName, Seen)], + GoodApps = [App || AppName <- AppNames, + App <- Apps, + rebar_app_info:name(App) =:= AppName], + %% 2) + %% TODO: add extra dirs (and test), and possibly the stdlib + GoodAppPaths = [rebar_app_info:ebin_dir(App) || App <- GoodApps], + %% ++ [code:lib_dir()], + %% 3) + [begin + AtomApp = binary_to_atom(AppName, utf8), + %% blind load/unload won't interrupt an already-running app, + %% preventing odd errors, maybe! + case application:unload(AtomApp) of + ok -> application:load(AtomApp); + _ -> ok + end + end || AppName <- AppNames, + %% Shouldn't unload ourselves; rebar runs without ever + %% being started and unloading breaks logging! + AppName =/= <<"rebar">>], + + %% 4) + CandidateMods = lists:append( + %% Start by asking the currently loaded app (if loaded) + %% since it would be the primary source of conflicting modules + [case application:get_key(AppName, modules) of + {ok, Mods} -> + Mods; + undefined -> + %% if not found, parse the app file on disk, in case + %% the app's modules are used without it being loaded + case rebar_app_info:app_details(App) of + [] -> []; + Details -> proplists:get_value(modules, Details, []) + end + end || App <- GoodApps, + AppName <- [binary_to_atom(rebar_app_info:name(App), utf8)]] + ), + + %% 5) + Mods = misloaded_modules(CandidateMods, GoodAppPaths, ModPaths), + [purge_mod(Mod) || Mod <- Mods], + purge_and_load(Rest, ModPaths, + sets:union(Seen, sets:from_list(AppNames))). + + +purge(Paths, ModPaths) -> + lists:map(fun purge_mod/1, lists:usort( + [Mod || {Mod, Path} <- ModPaths, + is_list(Path), % not 'preloaded' or mocked + any_prefix(Path, Paths)] + )). + +misloaded_modules(Mods, GoodAppPaths, ModPaths) -> + %% Identify paths that are invalid; i.e. app paths that cover an + %% app in the desired group, but are not in the desired group. + lists:usort( + [purge_mod(Mod) + || Mod <- Mods, + {_, Path} <- [lists:keyfind(Mod, 1, ModPaths)], + is_list(Path), % not 'preloaded' or mocked + not any_prefix(Path, GoodAppPaths)] + ). + +any_prefix(Path, Paths) -> + lists:any(fun(P) -> lists:prefix(P, Path) end, Paths). + +%% assume paths currently set are good; only unload a module so next call +%% uses the correctly set paths +purge_mod(Mod) -> + case erlang:check_process_code(self(), Mod) of + false -> + code:purge(Mod), + code:delete(Mod); + _ -> + %% cannot purge safely without killing ourselves + code:soft_purge(Mod) andalso + code:delete(Mod) + end. + +path_groups(Targets, State) -> + [{Target, get_paths(Target, State)} || Target <- Targets]. + +app_groups(Targets, State) -> + [{Target, get_apps(Target, State)} || Target <- Targets]. + +get_paths(deps, State) -> + rebar_state:code_paths(State, all_deps); +get_paths(plugins, State) -> + rebar_state:code_paths(State, all_plugin_deps). + +get_apps(deps, State) -> + %% The code paths for deps also include the top level apps + %% and the extras, which we don't have here; we have to + %% add the apps by hand + rebar_state:all_deps(State) ++ + case rebar_state:project_apps(State) of + undefined -> []; + List -> List + end; +get_apps(plugins, State) -> + rebar_state:all_plugin_deps(State). -- cgit v1.1 From 311ee6b1371c3eea3611dc5d7945b1b5667c75bd Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Sat, 6 Oct 2018 11:38:33 -0400 Subject: Fix a bug in compiler path handling Also handle some formatting --- src/rebar_paths.erl | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) (limited to 'src/rebar_paths.erl') diff --git a/src/rebar_paths.erl b/src/rebar_paths.erl index bb43897..71f0016 100644 --- a/src/rebar_paths.erl +++ b/src/rebar_paths.erl @@ -130,11 +130,10 @@ misloaded_modules(Mods, GoodAppPaths, ModPaths) -> %% Identify paths that are invalid; i.e. app paths that cover an %% app in the desired group, but are not in the desired group. lists:usort( - [purge_mod(Mod) - || Mod <- Mods, - {_, Path} <- [lists:keyfind(Mod, 1, ModPaths)], - is_list(Path), % not 'preloaded' or mocked - not any_prefix(Path, GoodAppPaths)] + [Mod || Mod <- Mods, + {_, Path} <- [lists:keyfind(Mod, 1, ModPaths)], + is_list(Path), % not 'preloaded' or mocked + not any_prefix(Path, GoodAppPaths)] ). any_prefix(Path, Paths) -> @@ -168,10 +167,10 @@ get_apps(deps, State) -> %% The code paths for deps also include the top level apps %% and the extras, which we don't have here; we have to %% add the apps by hand - rebar_state:all_deps(State) ++ case rebar_state:project_apps(State) of undefined -> []; List -> List - end; + end ++ + rebar_state:all_deps(State); get_apps(plugins, State) -> rebar_state:all_plugin_deps(State). -- cgit v1.1 From af5cecd8eec9692f43d04ad53c8f28734012b873 Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Mon, 8 Oct 2018 12:41:30 -0400 Subject: Clean path code, add tests, add clash detection Some finishing touch to that code --- src/rebar_paths.erl | 47 +++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 41 insertions(+), 6 deletions(-) (limited to 'src/rebar_paths.erl') diff --git a/src/rebar_paths.erl b/src/rebar_paths.erl index 71f0016..23fc755 100644 --- a/src/rebar_paths.erl +++ b/src/rebar_paths.erl @@ -5,6 +5,11 @@ -type targets() :: [target(), ...]. -export_type([target/0, targets/0]). -export([set_paths/2, unset_paths/2]). +-export([clashing_apps/2]). + +-ifdef(TEST). +-export([misloaded_modules/3]). +-endif. -spec set_paths(targets(), rebar_state:t()) -> ok. set_paths(UserTargets, State) -> @@ -13,8 +18,8 @@ set_paths(UserTargets, State) -> Paths = lists:append([P || {_, P} <- GroupPaths]), [code:del_path(P) || P <- Paths], code:add_pathsa(lists:reverse(Paths)), - % set path breaks with escripts - %true = code:set_path(lists:append([P || {_, P} <- GroupPaths])), + % set path breaks with escripts; we gotta do it by hand + % true = code:set_path(lists:append([P || {_, P} <- GroupPaths])), AppGroups = app_groups(Targets, State), purge_and_load(AppGroups, code:all_loaded(), sets:new()), ok. @@ -28,6 +33,16 @@ unset_paths(UserTargets, State) -> purge(Paths, code:all_loaded()), ok. +clashing_apps(Targets, State) -> + AppGroups = app_groups(Targets, State), + AppNames = [{G, sets:from_list( + [rebar_app_info:name(App) || App <- Apps] + )} || {G, Apps} <- AppGroups], + clashing_app_names(sets:new(), AppNames, []). + +%%%%%%%%%%%%%%% +%%% PRIVATE %%% +%%%%%%%%%%%%%%% %% The paths are to be set in the reverse order; i.e. the default %% path is always last when possible (minimize cases where a build @@ -77,9 +92,9 @@ purge_and_load([{_Group, Apps}|Rest], ModPaths, Seen) -> App <- Apps, rebar_app_info:name(App) =:= AppName], %% 2) - %% TODO: add extra dirs (and test), and possibly the stdlib + %% (no need for extra_src_dirs since those get put into ebin; + %% also no need for OTP libs; we want to allow overtaking them) GoodAppPaths = [rebar_app_info:ebin_dir(App) || App <- GoodApps], - %% ++ [code:lib_dir()], %% 3) [begin AtomApp = binary_to_atom(AppName, utf8), @@ -118,12 +133,12 @@ purge_and_load([{_Group, Apps}|Rest], ModPaths, Seen) -> purge_and_load(Rest, ModPaths, sets:union(Seen, sets:from_list(AppNames))). - purge(Paths, ModPaths) -> + SortedPaths = lists:sort(Paths), lists:map(fun purge_mod/1, lists:usort( [Mod || {Mod, Path} <- ModPaths, is_list(Path), % not 'preloaded' or mocked - any_prefix(Path, Paths)] + any_prefix(Path, SortedPaths)] )). misloaded_modules(Mods, GoodAppPaths, ModPaths) -> @@ -152,6 +167,26 @@ purge_mod(Mod) -> code:delete(Mod) end. + +%% This is a tricky O(n²) check since we want to +%% know whether an app clashes with any of the top priority groups. +%% +%% For example, let's say we have `[deps, plugins]', then we want +%% to find the plugins that clash with deps: +%% +%% `[{deps, [ClashingPlugins]}, {plugins, []}]' +%% +%% In case we'd ever have alternative or additional types, we can +%% find all clashes from other 'groups'. +clashing_app_names(_, [], Acc) -> + lists:reverse(Acc); +clashing_app_names(PrevNames, [{G,AppNames} | Rest], Acc) -> + CurrentNames = sets:subtract(AppNames, PrevNames), + NextNames = sets:subtract(sets:union([A || {_, A} <- Rest]), PrevNames), + Clashes = sets:intersection(CurrentNames, NextNames), + NewAcc = [{G, sets:to_list(Clashes)} | Acc], + clashing_app_names(sets:union(PrevNames, CurrentNames), Rest, NewAcc). + path_groups(Targets, State) -> [{Target, get_paths(Target, State)} || Target <- Targets]. -- cgit v1.1 From dada4e36e6d9a5c4b41bbe1f68389520e7c59ace Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Thu, 11 Oct 2018 08:38:37 -0400 Subject: Optimize path handling - Only set paths that need to be put as a priority - Clean up paths before leaving API mode The first point accounted for some performance cost, but the latter one explains the 40% overhead in test runs: since rebar3 calls rebar3 a lot with a bunch of fake apps, and that the new mechanism for path handling by default does not _remove_ paths, it just _orders_ them, we would end up in a situation where as the tests ran, more and more fake paths would get added to the VM. By the time the run was over, all path handling would take longer since more paths needed filtering every time. By resetting paths at the end of an API run, we prevent a given 'project' from polluting another one's runtime and performance once the API successfully returns. --- src/rebar_paths.erl | 59 ++++++++++++++++++++++++++--------------------------- 1 file changed, 29 insertions(+), 30 deletions(-) (limited to 'src/rebar_paths.erl') diff --git a/src/rebar_paths.erl b/src/rebar_paths.erl index 23fc755..900443d 100644 --- a/src/rebar_paths.erl +++ b/src/rebar_paths.erl @@ -1,3 +1,5 @@ +%% BEFORE THIS FIX: rebar3 ct 266.78s user 144.06s system 144% cpu 4:33.70 total +%% CURRENT TIME: rebar3 ct 419.30s user 301.00s system 152% cpu 7:51.98 total -module(rebar_paths). -include("rebar.hrl"). @@ -8,20 +10,17 @@ -export([clashing_apps/2]). -ifdef(TEST). --export([misloaded_modules/3]). +-export([misloaded_modules/2]). -endif. -spec set_paths(targets(), rebar_state:t()) -> ok. set_paths(UserTargets, State) -> Targets = normalize_targets(UserTargets), GroupPaths = path_groups(Targets, State), - Paths = lists:append([P || {_, P} <- GroupPaths]), - [code:del_path(P) || P <- Paths], - code:add_pathsa(lists:reverse(Paths)), - % set path breaks with escripts; we gotta do it by hand - % true = code:set_path(lists:append([P || {_, P} <- GroupPaths])), + Paths = lists:append(lists:reverse([P || {_, P} <- GroupPaths])), + code:add_pathsa(Paths), AppGroups = app_groups(Targets, State), - purge_and_load(AppGroups, code:all_loaded(), sets:new()), + purge_and_load(AppGroups, sets:new()), ok. -spec unset_paths(targets(), rebar_state:t()) -> ok. @@ -33,6 +32,7 @@ unset_paths(UserTargets, State) -> purge(Paths, code:all_loaded()), ok. +-spec clashing_apps(targets(), rebar_state:t()) -> [{target(), [binary()]}]. clashing_apps(Targets, State) -> AppGroups = app_groups(Targets, State), AppNames = [{G, sets:from_list( @@ -66,9 +66,9 @@ normalize_targets(List) -> ), lists:reverse(TmpList). -purge_and_load([], _, _) -> +purge_and_load([], _) -> ok; -purge_and_load([{_Group, Apps}|Rest], ModPaths, Seen) -> +purge_and_load([{_Group, Apps}|Rest], Seen) -> %% We have: a list of all applications in the current priority group, %% a list of all loaded modules with their active path, and a list of %% seen applications. @@ -79,7 +79,12 @@ purge_and_load([{_Group, Apps}|Rest], ModPaths, Seen) -> %% 3. unload and reload apps that may have changed paths in order %% to get updated module lists and specs %% (we ignore started apps and apps that have not run for this) - %% 4. create a list of modules to check from that app list + %% This part turns out to be the bottleneck of this module, so + %% to speed it up, using clash detection proves useful: + %% only reload apps that clashed since others are unlikely to + %% conflict in significant ways + %% 4. create a list of modules to check from that app list—only loaded + %% modules make sense to check. %% 5. check the modules to match their currently loaded paths with %% the path set from the apps in the current group; modules %% that differ must be purged; others can stay @@ -126,27 +131,29 @@ purge_and_load([{_Group, Apps}|Rest], ModPaths, Seen) -> end || App <- GoodApps, AppName <- [binary_to_atom(rebar_app_info:name(App), utf8)]] ), + ModPaths = [{Mod,Path} || Mod <- CandidateMods, + erlang:function_exported(Mod, module_info, 0), + {file, Path} <- [code:is_loaded(Mod)]], %% 5) - Mods = misloaded_modules(CandidateMods, GoodAppPaths, ModPaths), + Mods = misloaded_modules(GoodAppPaths, ModPaths), [purge_mod(Mod) || Mod <- Mods], - purge_and_load(Rest, ModPaths, - sets:union(Seen, sets:from_list(AppNames))). + + purge_and_load(Rest, sets:union(Seen, sets:from_list(AppNames))). purge(Paths, ModPaths) -> SortedPaths = lists:sort(Paths), - lists:map(fun purge_mod/1, lists:usort( - [Mod || {Mod, Path} <- ModPaths, - is_list(Path), % not 'preloaded' or mocked - any_prefix(Path, SortedPaths)] - )). + lists:map(fun purge_mod/1, + [Mod || {Mod, Path} <- ModPaths, + is_list(Path), % not 'preloaded' or mocked + any_prefix(Path, SortedPaths)] + ). -misloaded_modules(Mods, GoodAppPaths, ModPaths) -> +misloaded_modules(GoodAppPaths, ModPaths) -> %% Identify paths that are invalid; i.e. app paths that cover an %% app in the desired group, but are not in the desired group. lists:usort( - [Mod || Mod <- Mods, - {_, Path} <- [lists:keyfind(Mod, 1, ModPaths)], + [Mod || {Mod, Path} <- ModPaths, is_list(Path), % not 'preloaded' or mocked not any_prefix(Path, GoodAppPaths)] ). @@ -157,15 +164,7 @@ any_prefix(Path, Paths) -> %% assume paths currently set are good; only unload a module so next call %% uses the correctly set paths purge_mod(Mod) -> - case erlang:check_process_code(self(), Mod) of - false -> - code:purge(Mod), - code:delete(Mod); - _ -> - %% cannot purge safely without killing ourselves - code:soft_purge(Mod) andalso - code:delete(Mod) - end. + code:soft_purge(Mod) andalso code:delete(Mod). %% This is a tricky O(n²) check since we want to -- cgit v1.1 From fb6de6e0e5dc0da1c4e64c166bcb69327420cb60 Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Sun, 14 Oct 2018 12:43:28 -0400 Subject: Drop outdated comments --- src/rebar_paths.erl | 2 -- 1 file changed, 2 deletions(-) (limited to 'src/rebar_paths.erl') diff --git a/src/rebar_paths.erl b/src/rebar_paths.erl index 900443d..82c0218 100644 --- a/src/rebar_paths.erl +++ b/src/rebar_paths.erl @@ -1,5 +1,3 @@ -%% BEFORE THIS FIX: rebar3 ct 266.78s user 144.06s system 144% cpu 4:33.70 total -%% CURRENT TIME: rebar3 ct 419.30s user 301.00s system 152% cpu 7:51.98 total -module(rebar_paths). -include("rebar.hrl"). -- cgit v1.1 From fc9b11afcadc1090f613eaef54873f17fc9020c5 Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Thu, 29 Nov 2018 23:00:23 -0500 Subject: Clear app details cache before checking in paths The app data, when set by `rebar3_app_discover` basically does not include the modules if it was derived from the .app.src file, even if at a later point the modules were compiled. Since there is currently no clear way to add that information reliably across all compiler versions in all types of compilers, we simply clear the cache and re-derive the information when required. This might have a small performance cost, but is required for correctness whenever an application's first build is run. Specifically, this bug was detected when a plugin included lager's parse transform, and the application itself also required it, but two distinct versions were needed. This patch finalizes fixing this issue. --- src/rebar_paths.erl | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'src/rebar_paths.erl') diff --git a/src/rebar_paths.erl b/src/rebar_paths.erl index 82c0218..c49ed36 100644 --- a/src/rebar_paths.erl +++ b/src/rebar_paths.erl @@ -111,7 +111,6 @@ purge_and_load([{_Group, Apps}|Rest], Seen) -> %% Shouldn't unload ourselves; rebar runs without ever %% being started and unloading breaks logging! AppName =/= <<"rebar">>], - %% 4) CandidateMods = lists:append( %% Start by asking the currently loaded app (if loaded) @@ -121,8 +120,10 @@ purge_and_load([{_Group, Apps}|Rest], Seen) -> Mods; undefined -> %% if not found, parse the app file on disk, in case - %% the app's modules are used without it being loaded - case rebar_app_info:app_details(App) of + %% the app's modules are used without it being loaded; + %% invalidate the cache in case we're proceeding during + %% compilation steps by setting the app details to `[]' + case rebar_app_info:app_details(rebar_app_info:app_details(App, [])) of [] -> []; Details -> proplists:get_value(modules, Details, []) end -- cgit v1.1 From c0957db49bdd0af80eb72a5a3c2c03796d959044 Mon Sep 17 00:00:00 2001 From: Fred Hebert Date: Sat, 1 Dec 2018 13:04:48 -0500 Subject: Review comments addressed --- src/rebar_paths.erl | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'src/rebar_paths.erl') diff --git a/src/rebar_paths.erl b/src/rebar_paths.erl index c49ed36..160f9fa 100644 --- a/src/rebar_paths.erl +++ b/src/rebar_paths.erl @@ -122,7 +122,9 @@ purge_and_load([{_Group, Apps}|Rest], Seen) -> %% if not found, parse the app file on disk, in case %% the app's modules are used without it being loaded; %% invalidate the cache in case we're proceeding during - %% compilation steps by setting the app details to `[]' + %% compilation steps by setting the app details to `[]', which + %% is its empty value; the details will then be reloaded + %% from disk when found case rebar_app_info:app_details(rebar_app_info:app_details(App, [])) of [] -> []; Details -> proplists:get_value(modules, Details, []) -- cgit v1.1