From 9d788d893620f868b9c0ee00ddec8ae4d5d8fea7 Mon Sep 17 00:00:00 2001
From: Fred Hebert <mononcqc@ferd.ca>
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_api.erl             |  17 ++++
 src/rebar_hooks.erl           |   8 +-
 src/rebar_packages.erl        |   2 +-
 src/rebar_paths.erl           | 177 +++++++++++++++++++++++++++++++++++
 src/rebar_plugins.erl         |   4 +-
 src/rebar_prv_common_test.erl |  12 +--
 src/rebar_prv_compile.erl     |  36 ++++---
 src/rebar_prv_dialyzer.erl    |   5 +-
 src/rebar_prv_edoc.erl        |   4 +-
 src/rebar_prv_eunit.erl       |   8 +-
 src/rebar_prv_xref.erl        |   5 +-
 test/rebar_compile_SUITE.erl  |   5 +-
 test/rebar_paths_SUITE.erl    | 211 ++++++++++++++++++++++++++++++++++++++++++
 test/rebar_plugins_SUITE.erl  |   2 +-
 14 files changed, 448 insertions(+), 48 deletions(-)
 create mode 100644 src/rebar_paths.erl
 create mode 100644 test/rebar_paths_SUITE.erl

diff --git a/src/rebar_api.erl b/src/rebar_api.erl
index 4dabe8a..00eb054 100644
--- a/src/rebar_api.erl
+++ b/src/rebar_api.erl
@@ -9,6 +9,8 @@
          expand_env_variable/3,
          get_arch/0,
          wordsize/0,
+         set_paths/2,
+         unset_paths/2,
          add_deps_to_path/1,
          restore_code_path/1,
          processing_base_dir/1,
@@ -67,6 +69,21 @@ get_arch() ->
 wordsize() ->
     rebar_utils:wordsize().
 
+%% @doc Set code paths. Takes arguments of the form
+%% `[plugins, deps]' or `[deps, plugins]' and ensures the
+%% project's app and dependencies are set in the right order
+%% for the next bit of execution
+-spec set_paths(rebar_paths:targets(), rebar_state:t()) -> ok.
+set_paths(List, State) ->
+    rebar_paths:set_paths(List, State).
+
+%% @doc Unsets code paths. Takes arguments of the form
+%% `[plugins, deps]' or `[deps, plugins]' and ensures the
+%% paths are no longer active.
+-spec unset_paths(rebar_paths:targets(), rebar_state:t()) -> ok.
+unset_paths(List, State) ->
+    rebar_paths:unset_paths(List, State).
+
 %% @doc Add deps to the code path
 -spec add_deps_to_path(rebar_state:t()) -> ok.
 add_deps_to_path(State) ->
diff --git a/src/rebar_hooks.erl b/src/rebar_hooks.erl
index ec6fe31..f2ef8a3 100644
--- a/src/rebar_hooks.erl
+++ b/src/rebar_hooks.erl
@@ -42,8 +42,9 @@ run_provider_hooks_(Dir, Type, Command, Providers, TypeHooks, State) ->
         [] ->
             State;
         HookProviders ->
-            PluginDepsPaths = lists:usort(rebar_state:code_paths(State, all_plugin_deps)),
-            code:add_pathsa(PluginDepsPaths),
+            %PluginDepsPaths = lists:usort(rebar_state:code_paths(State, all_plugin_deps)),
+            %code:add_pathsa(PluginDepsPaths),
+            rebar_paths:set_paths([plugins, deps], State),
             Providers1 = rebar_state:providers(State),
             State1 = rebar_state:providers(rebar_state:dir(State, Dir), Providers++Providers1),
             case rebar_core:do(HookProviders, State1) of
@@ -51,7 +52,8 @@ run_provider_hooks_(Dir, Type, Command, Providers, TypeHooks, State) ->
                     ?DEBUG(format_error({bad_provider, Type, Command, ProviderName}), []),
                     throw(?PRV_ERROR({bad_provider, Type, Command, ProviderName}));
                 {ok, State2} ->
-                    rebar_utils:remove_from_code_path(PluginDepsPaths),
+                    %rebar_utils:remove_from_code_path(PluginDepsPaths),
+                    rebar_paths:set_paths([deps, plugins], State2),
                     State2
             end
     end.
diff --git a/src/rebar_packages.erl b/src/rebar_packages.erl
index 7676213..096948c 100644
--- a/src/rebar_packages.erl
+++ b/src/rebar_packages.erl
@@ -82,7 +82,7 @@ get_package(Dep, Vsn, Hash, Repos, Table, State) ->
             not_found
     end.
 
-new_package_table() ->    
+new_package_table() ->
     ?PACKAGE_TABLE = ets:new(?PACKAGE_TABLE, [named_table, public, ordered_set, {keypos, 2}]),
     ets:insert(?PACKAGE_TABLE, {?PACKAGE_INDEX_VERSION, package_index_version}).
 
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).
diff --git a/src/rebar_plugins.erl b/src/rebar_plugins.erl
index d58675d..a9c7e00 100644
--- a/src/rebar_plugins.erl
+++ b/src/rebar_plugins.erl
@@ -122,12 +122,10 @@ handle_plugin(Profile, Plugin, State, Upgrade) ->
         %% Add newly built deps and plugin to code path
         State3 = rebar_state:update_all_plugin_deps(State2, Apps),
         NewCodePaths = [rebar_app_info:ebin_dir(A) || A <- ToBuild],
-        AllPluginEbins = filelib:wildcard(filename:join([rebar_dir:plugins_dir(State), "*", "ebin"])),
-        CodePaths = PreBuiltPaths++(AllPluginEbins--ToBuild),
-        code:add_pathsa(NewCodePaths++CodePaths),
 
         %% Store plugin code paths so we can remove them when compiling project apps
         State4 = rebar_state:update_code_paths(State3, all_plugin_deps, PreBuiltPaths++NewCodePaths),
+        rebar_paths:set_paths([plugins, deps], State4),
 
         {plugin_providers(Plugin), State4}
     catch
diff --git a/src/rebar_prv_common_test.erl b/src/rebar_prv_common_test.erl
index 9e71ee7..3d3bd8a 100644
--- a/src/rebar_prv_common_test.erl
+++ b/src/rebar_prv_common_test.erl
@@ -58,7 +58,7 @@ do(State) ->
 
 do(State, Tests) ->
     ?INFO("Running Common Test suites...", []),
-    rebar_utils:update_code(rebar_state:code_paths(State, all_deps), [soft_purge]),
+    rebar_paths:set_paths([deps, plugins], State),
 
     %% Run ct provider prehooks
     Providers = rebar_state:providers(State),
@@ -73,14 +73,14 @@ do(State, Tests) ->
                 ok    ->
                     %% Run ct provider post hooks for all project apps and top level project hooks
                     rebar_hooks:run_project_and_app_hooks(Cwd, post, ?PROVIDER, Providers, State),
-                    rebar_utils:cleanup_code_path(rebar_state:code_paths(State, default)),
+                    rebar_paths:set_paths([plugins, deps], State),
                     {ok, State};
                 Error ->
-                    rebar_utils:cleanup_code_path(rebar_state:code_paths(State, default)),
+                    rebar_paths:set_paths([plugins, deps], State),
                     Error
             end;
         Error ->
-            rebar_utils:cleanup_code_path(rebar_state:code_paths(State, default)),
+            rebar_paths:set_paths([plugins, deps], State),
             Error
     end.
 
@@ -250,11 +250,9 @@ select_tests(State, ProjectApps, CmdOpts, CfgOpts) ->
                             end, SysConfigs),
     %% NB: load the applications (from user directories too) to support OTP < 17
     %% to our best ability.
-    OldPath = code:get_path(),
-    code:add_pathsa(rebar_state:code_paths(State, all_deps)),
+    rebar_paths:set_paths([deps, plugins], State),
     [application:load(Application) || Config <- Configs, {Application, _} <- Config],
     rebar_utils:reread_config(Configs),
-    code:set_path(OldPath),
 
     Opts = merge_opts(CmdOpts,CfgOpts),
     discover_tests(State, ProjectApps, Opts).
diff --git a/src/rebar_prv_compile.erl b/src/rebar_prv_compile.erl
index a509704..405f73a 100644
--- a/src/rebar_prv_compile.erl
+++ b/src/rebar_prv_compile.erl
@@ -37,10 +37,7 @@ init(State) ->
 -spec do(rebar_state:t()) -> {ok, rebar_state:t()} | {error, string()}.
 do(State) ->
     IsDepsOnly = is_deps_only(State),
-    DepsPaths = rebar_state:code_paths(State, all_deps),
-    PluginDepsPaths = rebar_state:code_paths(State, all_plugin_deps),
-    rebar_utils:remove_from_code_path(PluginDepsPaths),
-    code:add_pathsa(DepsPaths),
+    rebar_paths:set_paths([deps, plugins], State),
 
     Providers = rebar_state:providers(State),
     Deps = rebar_state:deps_to_build(State),
@@ -50,11 +47,10 @@ do(State) ->
                  true ->
                      State;
                  false ->
-                     handle_project_apps(DepsPaths, Providers, State)
+                     handle_project_apps(Providers, State)
              end,
 
-    rebar_utils:cleanup_code_path(rebar_state:code_paths(State1, default)
-                                  ++ rebar_state:code_paths(State, all_plugin_deps)),
+    rebar_paths:set_paths([plugins, deps], State1),
 
     {ok, State1}.
 
@@ -62,7 +58,7 @@ is_deps_only(State) ->
     {Args, _} = rebar_state:command_parsed_args(State),
     proplists:get_value(deps_only, Args, false).
 
-handle_project_apps(DepsPaths, Providers, State) ->
+handle_project_apps(Providers, State) ->
     Cwd = rebar_state:dir(State),
     ProjectApps = rebar_state:project_apps(State),
     {ok, ProjectApps1} = rebar_digraph:compile_order(ProjectApps),
@@ -76,7 +72,7 @@ handle_project_apps(DepsPaths, Providers, State) ->
     %% projects with structures like /apps/foo,/apps/bar,/test
     build_extra_dirs(State, ProjectApps2),
 
-    State3 = update_code_paths(State2, ProjectApps2, DepsPaths),
+    State3 = update_code_paths(State2, ProjectApps2),
 
     rebar_hooks:run_all_hooks(Cwd, post, ?PROVIDER, Providers, State2),
     case rebar_state:has_all_artifacts(State3) of
@@ -176,11 +172,10 @@ compile(State, Providers, AppInfo) ->
     %% The rebar_otp_app compilation step is safe regarding the
     %% overall path management, so we can just load all plugins back
     %% in memory.
-    PluginDepsPaths = rebar_state:code_paths(State, all_plugin_deps),
-    code:add_pathsa(PluginDepsPaths),
+    rebar_paths:set_paths([plugins, deps], State),
     AppFileCompileResult = rebar_otp_app:compile(State, AppInfo4),
     %% Clean up after ourselves, leave things as they were.
-    rebar_utils:remove_from_code_path(PluginDepsPaths),
+    rebar_paths:set_paths([deps, plugins], State),
 
     case AppFileCompileResult of
         {ok, AppInfo5} ->
@@ -206,21 +201,22 @@ build_app(AppInfo, State) ->
             case lists:keyfind(Type, 1, ProjectBuilders) of
                 {_, Module} ->
                     %% load plugins since thats where project builders would be
-                    PluginDepsPaths = rebar_state:code_paths(State, all_plugin_deps),
-                    code:add_pathsa(PluginDepsPaths),
-                    case Module:build(AppInfo) of
-                        ok ->
-                            rebar_utils:remove_from_code_path(PluginDepsPaths);
-                        {error, Reason} ->
-                            throw({error, {Module, Reason}})
+                    rebar_paths:set_paths([plugins, deps], State),
+                    Res = Module:build(AppInfo),
+                    rebar_paths:set_paths([deps, plugins], State),
+                    case Res of
+                        ok -> ok;
+                        {error, Reason} -> throw({error, {Module, Reason}})
                     end;
                 _ ->
                     throw(?PRV_ERROR({unknown_project_type, rebar_app_info:name(AppInfo), Type}))
             end
     end.
-update_code_paths(State, ProjectApps, DepsPaths) ->
+
+update_code_paths(State, ProjectApps) ->
     ProjAppsPaths = paths_for_apps(ProjectApps),
     ExtrasPaths = paths_for_extras(State, ProjectApps),
+    DepsPaths = rebar_state:code_paths(State, all_deps),
     rebar_state:code_paths(State, all_deps, DepsPaths ++ ProjAppsPaths ++ ExtrasPaths).
 
 paths_for_apps(Apps) -> paths_for_apps(Apps, []).
diff --git a/src/rebar_prv_dialyzer.erl b/src/rebar_prv_dialyzer.erl
index 99a7698..585051c 100644
--- a/src/rebar_prv_dialyzer.erl
+++ b/src/rebar_prv_dialyzer.erl
@@ -85,7 +85,8 @@ short_desc() ->
 do(State) ->
     maybe_fix_env(),
     ?INFO("Dialyzer starting, this may take a while...", []),
-    code:add_pathsa(rebar_state:code_paths(State, all_deps)),
+    rebar_paths:unset_paths([plugins], State), % no plugins in analysis
+    rebar_paths:set_paths([deps], State),
     Plt = get_plt(State),
 
     try
@@ -104,7 +105,7 @@ do(State) ->
         throw:{output_file_error, _, _} = Error ->
             ?PRV_ERROR(Error)
     after
-        rebar_utils:cleanup_code_path(rebar_state:code_paths(State, default))
+        rebar_paths:set_paths([plugins,deps], State)
     end.
 
 %% This is used to workaround dialyzer quirk discussed here
diff --git a/src/rebar_prv_edoc.erl b/src/rebar_prv_edoc.erl
index 9517335..c78296a 100644
--- a/src/rebar_prv_edoc.erl
+++ b/src/rebar_prv_edoc.erl
@@ -32,7 +32,7 @@ init(State) ->
 -spec do(rebar_state:t()) ->
     {ok, rebar_state:t()} | {error, string()} | {error, {module(), any()}}.
 do(State) ->
-    code:add_pathsa(rebar_state:code_paths(State, all_deps)),
+    rebar_paths:set_paths([deps, plugins], State),
     ProjectApps = rebar_state:project_apps(State),
     Providers = rebar_state:providers(State),
     EdocOpts = rebar_state:get(State, edoc_opts, []),
@@ -64,7 +64,7 @@ do(State) ->
             {app_failed, AppName}
     end,
     rebar_hooks:run_all_hooks(Cwd, post, ?PROVIDER, Providers, State),
-    rebar_utils:cleanup_code_path(rebar_state:code_paths(State, default)),
+    rebar_paths:set_paths([plugins, deps], State),
     case Res of
         {app_failed, App} ->
             ?PRV_ERROR({app_failed, App});
diff --git a/src/rebar_prv_eunit.erl b/src/rebar_prv_eunit.erl
index 4b71416..f120926 100644
--- a/src/rebar_prv_eunit.erl
+++ b/src/rebar_prv_eunit.erl
@@ -54,7 +54,7 @@ do(State, Tests) ->
     ?INFO("Performing EUnit tests...", []),
 
     setup_name(State),
-    rebar_utils:update_code(rebar_state:code_paths(State, all_deps), [soft_purge]),
+    rebar_paths:set_paths([deps, plugins], State),
 
     %% Run eunit provider prehooks
     Providers = rebar_state:providers(State),
@@ -67,14 +67,14 @@ do(State, Tests) ->
                 {ok, State1} ->
                     %% Run eunit provider posthooks
                     rebar_hooks:run_project_and_app_hooks(Cwd, post, ?PROVIDER, Providers, State1),
-                    rebar_utils:cleanup_code_path(rebar_state:code_paths(State, default)),
+                    rebar_paths:set_paths([plugins, deps], State),
                     {ok, State1};
                 Error ->
-                    rebar_utils:cleanup_code_path(rebar_state:code_paths(State, default)),
+                    rebar_paths:set_paths([plugins, deps], State),
                     Error
             end;
         Error ->
-            rebar_utils:cleanup_code_path(rebar_state:code_paths(State, default)),
+            rebar_paths:set_paths([plugins, deps], State),
             Error
     end.
 
diff --git a/src/rebar_prv_xref.erl b/src/rebar_prv_xref.erl
index 2405ebb..a6b1f73 100644
--- a/src/rebar_prv_xref.erl
+++ b/src/rebar_prv_xref.erl
@@ -36,8 +36,7 @@ init(State) ->
 
 -spec do(rebar_state:t()) -> {ok, rebar_state:t()} | {error, string()}.
 do(State) ->
-    OldPath = code:get_path(),
-    code:add_pathsa(rebar_state:code_paths(State, all_deps)),
+    rebar_paths:set_paths([deps, plugins], State),
     XrefChecks = prepare(State),
     XrefIgnores = rebar_state:get(State, xref_ignores, []),
     %% Run xref checks
@@ -48,7 +47,7 @@ do(State) ->
     QueryChecks = rebar_state:get(State, xref_queries, []),
     QueryResults = lists:foldl(fun check_query/2, [], QueryChecks),
     stopped = xref:stop(xref),
-    rebar_utils:cleanup_code_path(OldPath),
+    rebar_paths:set_paths([plugins, deps], State),
     case XrefResults =:= [] andalso QueryResults =:= [] of
         true ->
             {ok, State};
diff --git a/test/rebar_compile_SUITE.erl b/test/rebar_compile_SUITE.erl
index 867460c..6b1d791 100644
--- a/test/rebar_compile_SUITE.erl
+++ b/test/rebar_compile_SUITE.erl
@@ -1856,7 +1856,7 @@ include_file_in_src_test_multiapp(Config) ->
     AppDir1 = filename:join([?config(apps, Config), "lib", Name1]),
     AppDir2 = filename:join([?config(apps, Config), "lib", Name2]),
     Vsn = rebar_test_utils:create_random_vsn(),
-    rebar_test_utils:create_app(AppDir1, Name1, Vsn, [kernel, stdlib]),
+    rebar_test_utils:create_app(AppDir1, Name1, Vsn, [kernel, stdlib, list_to_atom(Name2)]),
     rebar_test_utils:create_app(AppDir2, Name2, Vsn, [kernel, stdlib]),
 
     Src = "-module(test).\n"
@@ -1878,7 +1878,8 @@ include_file_in_src_test_multiapp(Config) ->
     RebarConfig = [],
     rebar_test_utils:run_and_check(Config, RebarConfig,
                                    ["as", "test", "compile"],
-                                   {ok, [{app, Name1}]}).
+                                   {ok, [{app, Name1}]}),
+    ok.
 
 %% this test sets the env var, compiles, records the file last modified timestamp,
 %% recompiles and compares the file last modified timestamp to ensure it hasn't
diff --git a/test/rebar_paths_SUITE.erl b/test/rebar_paths_SUITE.erl
new file mode 100644
index 0000000..48a7000
--- /dev/null
+++ b/test/rebar_paths_SUITE.erl
@@ -0,0 +1,211 @@
+-module(rebar_paths_SUITE).
+-include_lib("eunit/include/eunit.hrl").
+-include_lib("common_test/include/ct.hrl").
+-compile(export_all).
+
+all() ->
+    [%clashing_apps,
+     check_modules,
+     set_paths
+    ].
+
+init_per_testcase(Case, Config) ->
+    BasePaths = code:get_path(),
+    %% This test checks that the right module sets get loaded; however, we must
+    %% ensure that we do not have clashes with other test suites' loaded modules,
+    %% which we cannot track. As such, we have to ensure all module names here are
+    %% unique.
+    %%
+    %% This is done by hand; if you see this test suite failing on its own, you
+    %% probably wrote a test suite that clashes!
+    Dir = filename:join([?config(priv_dir, Config), atom_to_list(?MODULE),
+                         atom_to_list(Case)]),
+    InDir = fun(Path) -> filename:join([Dir, Path]) end,
+    ADep = fake_app(<<"rp_a">>, <<"1.0.0">>, InDir("_build/default/lib/rp_a/")),
+    BDep = fake_app(<<"rp_b">>, <<"1.0.0">>, InDir("_build/default/lib/rp_b/")),
+    CDep = fake_app(<<"rp_c">>, <<"1.0.0">>, InDir("_build/default/lib/rp_c/")),
+    DDep = fake_app(<<"rp_d">>, <<"1.0.0">>, InDir("_build/default/lib/rp_d/")),
+    RelxDep = fake_app(<<"relx">>, <<"1.0.0">>, InDir("_build/default/lib/relx/")),
+
+    APlug = fake_app(<<"rp_a">>, <<"1.0.0">>,
+                     InDir("_build/default/plugins/lib/rp_a/")),
+    RelxPlug = fake_app(<<"relx">>, <<"1.1.1">>,
+                        InDir("_build/default/plugins/lib/relx")),
+    EPlug = fake_app(<<"rp_e">>, <<"1.0.0">>,
+                     InDir("_build/default/plugins/lib/rp_e/")),
+
+    S0 = rebar_state:new(),
+    S1 = rebar_state:all_deps(S0, [ADep, BDep, CDep, DDep, RelxDep]),
+    S2 = rebar_state:all_plugin_deps(S1, [APlug, RelxPlug]),
+    S3 = rebar_state:code_paths(S2, default, code:get_path()),
+    S4 = rebar_state:code_paths(
+        S3,
+        all_deps,
+        [rebar_app_info:ebin_dir(A) || A <- [ADep, BDep, CDep, DDep, RelxDep]]
+    ),
+    S5 = rebar_state:code_paths(
+        S4,
+        all_plugin_deps,
+        [rebar_app_info:ebin_dir(A) || A <- [APlug, RelxPlug, EPlug]]
+    ),
+    [{base_paths, BasePaths}, {root_dir, Dir}, {state, S5} | Config].
+
+end_per_testcase(_, Config) ->
+    %% this is deeply annoying because we interfere with rebar3's own
+    %% path handling!
+    rebar_paths:unset_paths([plugins, deps], ?config(state, Config)),
+    Config.
+
+fake_app(Name, Vsn, OutDir) ->
+    {ok, App} = rebar_app_info:new(Name, Vsn, OutDir),
+    compile_fake_appmod(App),
+    App.
+
+compile_fake_appmod(App) ->
+    OutDir = rebar_app_info:ebin_dir(App),
+    Vsn = rebar_app_info:original_vsn(App),
+    Name = rebar_app_info:name(App),
+
+    ok = filelib:ensure_dir(filename:join([OutDir, ".touch"])),
+
+    AppFile = [
+      "{application,", Name, ", "
+      " [{description,  \"some app\"}, "
+      "  {vsn, \"", Vsn, "\"}, "
+      "  {modules, [",Name,"]}, "
+      "  {registered, []}, "
+      "  {applications, [stdlib, kernel]} "
+      " ]}. "],
+
+    ok = file:write_file(filename:join([OutDir, <<Name/binary, ".app">>]), AppFile),
+
+    Mod = [{attribute, 1, module, binary_to_atom(Name, utf8)},
+           {attribute, 2, export, [{f,0}]},
+           {function,3,f,0,
+            [{clause,3, [], [],
+              [{string,3,OutDir}]
+            }]}
+          ],
+
+    {ok, _, Bin} = compile:forms(Mod),
+    ok = file:write_file(filename:join([OutDir, <<Name/binary, ".beam">>]), Bin).
+
+clashing_apps(Config) ->
+    Clashes = rebar_paths:clashing_apps([deps, plugins],
+                                        ?config(state, Config)),
+    ct:pal("Clashes: ~p", [Clashes]),
+
+    ?assertEqual([<<"relx">>, <<"rp_a">>], lists:sort(proplists:get_value(deps, Clashes))),
+    ?assertEqual(undefined, proplists:get_value(plugins, Clashes)),
+    ok.
+
+set_paths(Config) ->
+    State = ?config(state, Config),
+    RootDir = filename:split(?config(root_dir, Config)),
+    rebar_paths:set_paths([plugins, deps], State),
+    PluginPaths = code:get_path(),
+    rebar_paths:set_paths([deps, plugins], State),
+    DepPaths = code:get_path(),
+
+    ?assertEqual(
+       RootDir ++ ["_build", "default", "plugins", "lib", "rp_a", "ebin"],
+       find_first_instance("rp_a", PluginPaths)
+    ),
+    ?assertEqual(
+       RootDir ++ ["_build", "default", "lib", "rp_b", "ebin"],
+       find_first_instance("rp_b", PluginPaths)
+    ),
+    ?assertEqual(
+       RootDir ++ ["_build", "default", "lib", "rp_c", "ebin"],
+       find_first_instance("rp_c", PluginPaths)
+    ),
+    ?assertEqual(
+       RootDir ++ ["_build", "default", "lib", "rp_d", "ebin"],
+       find_first_instance("rp_d", PluginPaths)
+    ),
+    ?assertEqual(
+       RootDir ++ ["_build", "default", "plugins", "lib",  "rp_e", "ebin"],
+       find_first_instance("rp_e", PluginPaths)
+    ),
+    ?assertEqual(
+       RootDir ++ ["_build", "default", "plugins", "lib", "relx", "ebin"],
+       find_first_instance("relx", PluginPaths)
+    ),
+
+
+    ?assertEqual(
+       RootDir ++ ["_build", "default", "lib", "rp_a", "ebin"],
+       find_first_instance("rp_a", DepPaths)
+    ),
+    ?assertEqual(
+       RootDir ++ ["_build", "default", "lib", "rp_b", "ebin"],
+       find_first_instance("rp_b", DepPaths)
+    ),
+    ?assertEqual(
+       RootDir ++ ["_build", "default", "lib", "rp_c", "ebin"],
+       find_first_instance("rp_c", DepPaths)
+    ),
+    ?assertEqual(
+       RootDir ++ ["_build", "default", "lib", "rp_d", "ebin"],
+       find_first_instance("rp_d", DepPaths)
+    ),
+    ?assertEqual(
+       RootDir ++ ["_build", "default", "plugins", "lib", "rp_e", "ebin"],
+       find_first_instance("rp_e", DepPaths)
+    ),
+    ?assertEqual(
+       RootDir ++ ["_build", "default", "lib", "relx", "ebin"],
+       find_first_instance("relx", DepPaths)
+    ),
+    ok.
+
+check_modules(Config) ->
+    State = ?config(state, Config),
+    RootDir = ?config(root_dir, Config)++"/",
+    rebar_paths:set_paths([plugins, deps], State),
+    ct:pal("code:get_path() -> ~p", [code:get_path()]),
+
+    ?assertEqual(RootDir ++ "_build/default/plugins/lib/rp_a/ebin", rp_a:f()),
+    ct:pal("~p", [catch file:list_dir(RootDir ++ "_build/default/lib/")]),
+    ct:pal("~p", [catch file:list_dir(RootDir ++ "_build/default/lib/rp_b/")]),
+    ct:pal("~p", [catch file:list_dir(RootDir ++ "_build/default/lib/rp_b/ebin")]),
+    ct:pal("~p", [catch b:module_info()]),
+    ?assertEqual(RootDir ++ "_build/default/lib/rp_b/ebin", rp_b:f()),
+    ?assertEqual(RootDir ++ "_build/default/lib/rp_c/ebin", rp_c:f()),
+    ?assertEqual(RootDir ++ "_build/default/lib/rp_d/ebin", rp_d:f()),
+    ?assertEqual(RootDir ++ "_build/default/plugins/lib/rp_e/ebin", rp_e:f()),
+    ?assertEqual(RootDir ++ "_build/default/plugins/lib/relx/ebin", relx:f()),
+    ?assertEqual(3, length(relx:module_info(exports))), % can't replace bundled
+
+    rebar_paths:set_paths([deps, plugins], State),
+    ct:pal("code:get_path() -> ~p", [code:get_path()]),
+
+    ?assertEqual(RootDir ++ "_build/default/lib/rp_a/ebin", rp_a:f()),
+    ?assertEqual(RootDir ++ "_build/default/lib/rp_b/ebin", rp_b:f()),
+    ?assertEqual(RootDir ++ "_build/default/lib/rp_c/ebin", rp_c:f()),
+    ?assertEqual(RootDir ++ "_build/default/lib/rp_d/ebin", rp_d:f()),
+    ?assertEqual(RootDir ++ "_build/default/plugins/lib/rp_e/ebin", rp_e:f()),
+    ?assertEqual(RootDir ++ "_build/default/lib/relx/ebin", relx:f()),
+    ?assertEqual(3, length(relx:module_info(exports))), % can't replace bundled
+
+    %% once again
+    rebar_paths:set_paths([plugins, deps], State),
+    ct:pal("code:get_path() -> ~p", [code:get_path()]),
+
+    ?assertEqual(RootDir ++ "_build/default/plugins/lib/rp_a/ebin", rp_a:f()),
+    ?assertEqual(RootDir ++ "_build/default/lib/rp_b/ebin", rp_b:f()),
+    ?assertEqual(RootDir ++ "_build/default/lib/rp_c/ebin", rp_c:f()),
+    ?assertEqual(RootDir ++ "_build/default/lib/rp_d/ebin", rp_d:f()),
+    ?assertEqual(RootDir ++ "_build/default/plugins/lib/rp_e/ebin", rp_e:f()),
+    ?assertEqual(RootDir ++ "_build/default/plugins/lib/relx/ebin", relx:f()),
+    ?assertEqual(3, length(relx:module_info(exports))), % can't replace bundled
+    ok.
+
+find_first_instance(Frag, []) ->
+    {not_found, Frag};
+find_first_instance(Frag, [Path|Rest]) ->
+    Frags = filename:split(Path),
+    case lists:member(Frag, Frags) of
+        true -> Frags;
+        false -> find_first_instance(Frag, Rest)
+    end.
diff --git a/test/rebar_plugins_SUITE.erl b/test/rebar_plugins_SUITE.erl
index 2d74539..c7a5d51 100644
--- a/test/rebar_plugins_SUITE.erl
+++ b/test/rebar_plugins_SUITE.erl
@@ -335,7 +335,7 @@ project_plugins(Config) ->
     rebar_test_utils:create_app(AppDir, Name, Vsn, [kernel, stdlib]),
 
     DepName = rebar_test_utils:create_random_name("dep1_"),
-    PluginName = "compile",
+    PluginName = "compile_plugin",
     PluginName2 = "release",
 
     Plugins = rebar_test_utils:expand_deps(git, [{PluginName, Vsn, []}, {PluginName2, Vsn, []}]),
-- 
cgit v1.1


From 311ee6b1371c3eea3611dc5d7945b1b5667c75bd Mon Sep 17 00:00:00 2001
From: Fred Hebert <mononcqc@ferd.ca>
Date: Sat, 6 Oct 2018 11:38:33 -0400
Subject: Fix a bug in compiler path handling

Also handle some formatting
---
 src/rebar_compiler.erl    |  7 +++----
 src/rebar_paths.erl       | 13 ++++++-------
 src/rebar_prv_compile.erl |  2 +-
 3 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/src/rebar_compiler.erl b/src/rebar_compiler.erl
index c2c514a..6e94cb2 100644
--- a/src/rebar_compiler.erl
+++ b/src/rebar_compiler.erl
@@ -34,11 +34,10 @@
 -define(RE_PREFIX, "^(?!\\._)").
 
 compile_all(Compilers, AppInfo) ->
-    OutDir = rebar_utils:to_list(rebar_app_info:out_dir(AppInfo)),
-
+    EbinDir = rebar_utils:to_list(rebar_app_info:ebin_dir(AppInfo)),
     %% Make sure that outdir is on the path
-    ok = rebar_file_utils:ensure_dir(OutDir),
-    true = code:add_patha(filename:absname(OutDir)),
+    ok = rebar_file_utils:ensure_dir(EbinDir),
+    true = code:add_patha(filename:absname(EbinDir)),
 
     %% necessary for erlang:function_exported/3 to work as expected
     %% called here for clarity as it's required by both opts_changed/2
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).
diff --git a/src/rebar_prv_compile.erl b/src/rebar_prv_compile.erl
index 405f73a..4b36636 100644
--- a/src/rebar_prv_compile.erl
+++ b/src/rebar_prv_compile.erl
@@ -115,7 +115,7 @@ copy_and_build_project_apps(State, Providers, Apps) ->
                    rebar_app_info:dir(AppInfo),
                    rebar_app_info:out_dir(AppInfo))
      || AppInfo <- Apps],
-    code:add_pathsa([rebar_app_info:out_dir(AppInfo) || AppInfo <- Apps]),
+    code:add_pathsa([rebar_app_info:ebin_dir(AppInfo) || AppInfo <- Apps]),
     [compile(State, Providers, AppInfo) || AppInfo <- Apps].
 
 
-- 
cgit v1.1


From af5cecd8eec9692f43d04ad53c8f28734012b873 Mon Sep 17 00:00:00 2001
From: Fred Hebert <mononcqc@ferd.ca>
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 ++++++++++++++++++++++++++++++++++++++++------
 test/rebar_paths_SUITE.erl | 37 +++++++++++++++++++++++++++++++++---
 2 files changed, 75 insertions(+), 9 deletions(-)

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].
 
diff --git a/test/rebar_paths_SUITE.erl b/test/rebar_paths_SUITE.erl
index 48a7000..f2832c0 100644
--- a/test/rebar_paths_SUITE.erl
+++ b/test/rebar_paths_SUITE.erl
@@ -4,11 +4,16 @@
 -compile(export_all).
 
 all() ->
-    [%clashing_apps,
+    [clashing_apps,
      check_modules,
-     set_paths
+     set_paths,
+     misloaded_mods
     ].
 
+%%%%%%%%%%%%%%%%%%
+%%% TEST SETUP %%%
+%%%%%%%%%%%%%%%%%%
+
 init_per_testcase(Case, Config) ->
     BasePaths = code:get_path(),
     %% This test checks that the right module sets get loaded; however, we must
@@ -90,13 +95,17 @@ compile_fake_appmod(App) ->
     {ok, _, Bin} = compile:forms(Mod),
     ok = file:write_file(filename:join([OutDir, <<Name/binary, ".beam">>]), Bin).
 
+%%%%%%%%%%%%%
+%%% TESTS %%%
+%%%%%%%%%%%%%
+
 clashing_apps(Config) ->
     Clashes = rebar_paths:clashing_apps([deps, plugins],
                                         ?config(state, Config)),
     ct:pal("Clashes: ~p", [Clashes]),
 
     ?assertEqual([<<"relx">>, <<"rp_a">>], lists:sort(proplists:get_value(deps, Clashes))),
-    ?assertEqual(undefined, proplists:get_value(plugins, Clashes)),
+    ?assertEqual([], proplists:get_value(plugins, Clashes)),
     ok.
 
 set_paths(Config) ->
@@ -201,6 +210,28 @@ check_modules(Config) ->
     ?assertEqual(3, length(relx:module_info(exports))), % can't replace bundled
     ok.
 
+misloaded_mods(_Config) ->
+    Res = rebar_paths:misloaded_modules(
+      [a,b,c,d,e,f],
+      ["/1/2/3/4",
+       "/1/2/4",
+       "/2/1/1",
+       "/3/4/5"],
+      [{a, "/0/1/2/file.beam"},
+       {aa, "/1/2/3/4/file.beam"},
+       {b, "/1/2/3/4/file.beam"},
+       {c, "/2/1/file.beam"},
+       {f, preloaded},
+       {d, "/3/5/7/file.beam"},
+       {e, "/3/4/5/file.beam"}]
+    ),
+    ?assertEqual([a,c,d], Res),
+    ok.
+
+%%%%%%%%%%%%%%%
+%%% HELPERS %%%
+%%%%%%%%%%%%%%%
+
 find_first_instance(Frag, []) ->
     {not_found, Frag};
 find_first_instance(Frag, [Path|Rest]) ->
-- 
cgit v1.1


From dada4e36e6d9a5c4b41bbe1f68389520e7c59ace Mon Sep 17 00:00:00 2001
From: Fred Hebert <mononcqc@ferd.ca>
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/rebar3.erl             | 15 +++++++++++-
 src/rebar_hooks.erl        |  7 ++----
 src/rebar_paths.erl        | 59 +++++++++++++++++++++++-----------------------
 src/rebar_plugins.erl      |  2 +-
 src/rebar_prv_compile.erl  | 12 +++++-----
 src/rebar_prv_xref.erl     |  3 +--
 test/rebar_paths_SUITE.erl |  2 --
 7 files changed, 53 insertions(+), 47 deletions(-)

diff --git a/src/rebar3.erl b/src/rebar3.erl
index c931a85..059d530 100644
--- a/src/rebar3.erl
+++ b/src/rebar3.erl
@@ -175,7 +175,20 @@ run_aux(State, RawArgs) ->
 
     State10 = rebar_state:code_paths(State9, default, code:get_path()),
 
-    rebar_core:init_command(rebar_state:command_args(State10, Args), Task).
+    case rebar_core:init_command(rebar_state:command_args(State10, Args), Task) of
+        {ok, State11} ->
+            case rebar_state:get(State11, caller, command_line) of
+                api ->
+                    rebar_paths:unset_paths([deps, plugins], State11),
+                    {ok, State11};
+                _ ->
+                    {ok, State11}
+            end;
+        Other ->
+            Other
+    end.
+
+
 
 %% @doc set up base configuration having to do with verbosity, where
 %% to find config files, and so on, and return an internal rebar3 state term.
diff --git a/src/rebar_hooks.erl b/src/rebar_hooks.erl
index f2ef8a3..358458e 100644
--- a/src/rebar_hooks.erl
+++ b/src/rebar_hooks.erl
@@ -42,9 +42,7 @@ run_provider_hooks_(Dir, Type, Command, Providers, TypeHooks, State) ->
         [] ->
             State;
         HookProviders ->
-            %PluginDepsPaths = lists:usort(rebar_state:code_paths(State, all_plugin_deps)),
-            %code:add_pathsa(PluginDepsPaths),
-            rebar_paths:set_paths([plugins, deps], State),
+            rebar_paths:set_paths([plugins], State),
             Providers1 = rebar_state:providers(State),
             State1 = rebar_state:providers(rebar_state:dir(State, Dir), Providers++Providers1),
             case rebar_core:do(HookProviders, State1) of
@@ -52,8 +50,7 @@ run_provider_hooks_(Dir, Type, Command, Providers, TypeHooks, State) ->
                     ?DEBUG(format_error({bad_provider, Type, Command, ProviderName}), []),
                     throw(?PRV_ERROR({bad_provider, Type, Command, ProviderName}));
                 {ok, State2} ->
-                    %rebar_utils:remove_from_code_path(PluginDepsPaths),
-                    rebar_paths:set_paths([deps, plugins], State2),
+                    rebar_paths:set_paths([deps], State2),
                     State2
             end
     end.
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
diff --git a/src/rebar_plugins.erl b/src/rebar_plugins.erl
index a9c7e00..2a78c6e 100644
--- a/src/rebar_plugins.erl
+++ b/src/rebar_plugins.erl
@@ -125,7 +125,7 @@ handle_plugin(Profile, Plugin, State, Upgrade) ->
 
         %% Store plugin code paths so we can remove them when compiling project apps
         State4 = rebar_state:update_code_paths(State3, all_plugin_deps, PreBuiltPaths++NewCodePaths),
-        rebar_paths:set_paths([plugins, deps], State4),
+        rebar_paths:set_paths([plugins], State4),
 
         {plugin_providers(Plugin), State4}
     catch
diff --git a/src/rebar_prv_compile.erl b/src/rebar_prv_compile.erl
index 4b36636..ee96d9f 100644
--- a/src/rebar_prv_compile.erl
+++ b/src/rebar_prv_compile.erl
@@ -37,7 +37,7 @@ init(State) ->
 -spec do(rebar_state:t()) -> {ok, rebar_state:t()} | {error, string()}.
 do(State) ->
     IsDepsOnly = is_deps_only(State),
-    rebar_paths:set_paths([deps, plugins], State),
+    rebar_paths:set_paths([deps], State),
 
     Providers = rebar_state:providers(State),
     Deps = rebar_state:deps_to_build(State),
@@ -50,7 +50,7 @@ do(State) ->
                      handle_project_apps(Providers, State)
              end,
 
-    rebar_paths:set_paths([plugins, deps], State1),
+    rebar_paths:set_paths([plugins], State1),
 
     {ok, State1}.
 
@@ -172,10 +172,10 @@ compile(State, Providers, AppInfo) ->
     %% The rebar_otp_app compilation step is safe regarding the
     %% overall path management, so we can just load all plugins back
     %% in memory.
-    rebar_paths:set_paths([plugins, deps], State),
+    rebar_paths:set_paths([plugins], State),
     AppFileCompileResult = rebar_otp_app:compile(State, AppInfo4),
-    %% Clean up after ourselves, leave things as they were.
-    rebar_paths:set_paths([deps, plugins], State),
+    %% Clean up after ourselves, leave things as they were with deps first
+    rebar_paths:set_paths([deps], State),
 
     case AppFileCompileResult of
         {ok, AppInfo5} ->
@@ -203,7 +203,7 @@ build_app(AppInfo, State) ->
                     %% load plugins since thats where project builders would be
                     rebar_paths:set_paths([plugins, deps], State),
                     Res = Module:build(AppInfo),
-                    rebar_paths:set_paths([deps, plugins], State),
+                    rebar_paths:set_paths([deps], State),
                     case Res of
                         ok -> ok;
                         {error, Reason} -> throw({error, {Module, Reason}})
diff --git a/src/rebar_prv_xref.erl b/src/rebar_prv_xref.erl
index a6b1f73..12063d5 100644
--- a/src/rebar_prv_xref.erl
+++ b/src/rebar_prv_xref.erl
@@ -36,7 +36,7 @@ init(State) ->
 
 -spec do(rebar_state:t()) -> {ok, rebar_state:t()} | {error, string()}.
 do(State) ->
-    rebar_paths:set_paths([deps, plugins], State),
+    rebar_paths:set_paths([deps], State),
     XrefChecks = prepare(State),
     XrefIgnores = rebar_state:get(State, xref_ignores, []),
     %% Run xref checks
@@ -47,7 +47,6 @@ do(State) ->
     QueryChecks = rebar_state:get(State, xref_queries, []),
     QueryResults = lists:foldl(fun check_query/2, [], QueryChecks),
     stopped = xref:stop(xref),
-    rebar_paths:set_paths([plugins, deps], State),
     case XrefResults =:= [] andalso QueryResults =:= [] of
         true ->
             {ok, State};
diff --git a/test/rebar_paths_SUITE.erl b/test/rebar_paths_SUITE.erl
index f2832c0..96cda45 100644
--- a/test/rebar_paths_SUITE.erl
+++ b/test/rebar_paths_SUITE.erl
@@ -212,13 +212,11 @@ check_modules(Config) ->
 
 misloaded_mods(_Config) ->
     Res = rebar_paths:misloaded_modules(
-      [a,b,c,d,e,f],
       ["/1/2/3/4",
        "/1/2/4",
        "/2/1/1",
        "/3/4/5"],
       [{a, "/0/1/2/file.beam"},
-       {aa, "/1/2/3/4/file.beam"},
        {b, "/1/2/3/4/file.beam"},
        {c, "/2/1/file.beam"},
        {f, preloaded},
-- 
cgit v1.1


From fb6de6e0e5dc0da1c4e64c166bcb69327420cb60 Mon Sep 17 00:00:00 2001
From: Fred Hebert <mononcqc@ferd.ca>
Date: Sun, 14 Oct 2018 12:43:28 -0400
Subject: Drop outdated comments

---
 src/rebar_paths.erl | 2 --
 1 file changed, 2 deletions(-)

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