Merge pull request #474 from tsloughter/fix_plugin_install

fixing plugin install and paths

src/rebar_core.erl

@@ -119,6 +119,7 @@ do([ProviderName | Rest], State) ->
                                   ,rebar_state:providers(State)
                                   ,rebar_state:namespace(State))
     end,
+
     case providers:do(Provider, State) of
         {ok, State1} ->
             do(Rest, State1);

src/rebar_hooks.erl

@@ -10,11 +10,17 @@ run_all_hooks(Dir, Type, Command, Providers, State) ->
     run_hooks(Dir, Type, Command, State).
 
 run_provider_hooks(Dir, Type, Command, Providers, State) ->
-    State1 = rebar_state:providers(rebar_state:dir(State, Dir), Providers),
+    PluginDepsPaths = rebar_state:code_paths(State, all_plugin_deps),
+    code:add_pathsa(PluginDepsPaths),
+    Providers1 = rebar_state:providers(State),
+    State1 = rebar_state:providers(rebar_state:dir(State, Dir), Providers++Providers1),
     AllHooks = rebar_state:get(State1, provider_hooks, []),
     TypeHooks = proplists:get_value(Type, AllHooks, []),
     HookProviders = proplists:get_all_values(Command, TypeHooks),
-    rebar_core:do(HookProviders, State1).
+
+    State2 = rebar_core:do(HookProviders, State1),
+    rebar_utils:remove_from_code_path(PluginDepsPaths),
+    State2.
 
 run_hooks(Dir, Type, Command, State) ->
     Hooks = case Type of

src/rebar_plugins.erl

@@ -32,31 +32,50 @@ handle_plugins(Plugins, State) ->
 
 handle_plugins(Profile, Plugins, State) ->
     %% Set deps dir to plugins dir so apps are installed there
+    Locks = rebar_state:lock(State),
+    DepsDir = rebar_state:get(State, deps_dir, ?DEFAULT_DEPS_DIR),
     State1 = rebar_state:set(State, deps_dir, ?DEFAULT_PLUGINS_DIR),
 
-    PluginProviders = lists:flatmap(fun(Plugin) ->
-                                            handle_plugin(Profile, Plugin, State1)
-                                    end, Plugins),
+    %% Install each plugin individually so if one fails to install it doesn't effect the others
+    {PluginProviders, State2} =
+        lists:foldl(fun(Plugin, {PluginAcc, StateAcc}) ->
+                            {NewPlugins, NewState} = handle_plugin(Profile, Plugin, StateAcc),
+                            {PluginAcc++NewPlugins, NewState}
+                      end, {[], State1}, Plugins),
 
     %% reset deps dir
-    State2 = rebar_state:set(State1, deps_dir, ?DEFAULT_DEPS_DIR),
+    State3 = rebar_state:set(State2, deps_dir, DepsDir),
+    State4 = rebar_state:lock(State3, Locks),
 
-    rebar_state:create_logic_providers(PluginProviders, State2).
+    rebar_state:create_logic_providers(PluginProviders, State4).
 
 handle_plugin(Profile, Plugin, State) ->
     try
-        {ok, _, State2} = rebar_prv_install_deps:handle_deps(Profile, State, [Plugin]),
+        {ok, Apps, State2} = rebar_prv_install_deps:handle_deps(Profile, State, [Plugin]),
+        {no_cycle, Sorted} = rebar_prv_install_deps:find_cycles(Apps),
+        ToBuild = rebar_prv_install_deps:cull_compile(Sorted, []),
 
-        Apps = rebar_state:all_deps(State2),
-        ToBuild = lists:dropwhile(fun rebar_app_info:valid/1, Apps),
+        %% Add already built plugin deps to the code path
+        CodePaths = [rebar_app_info:ebin_dir(A) || A <- Apps -- ToBuild],
+        code:add_pathsa(CodePaths),
+
+        %% Build plugin and its deps
         [build_plugin(AppInfo) || AppInfo <- ToBuild],
-        [true = code:add_patha(filename:join(rebar_app_info:dir(AppInfo), "ebin")) || AppInfo <- Apps],
-        plugin_providers(Plugin)
+
+        %% 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],
+        code:add_pathsa(CodePaths),
+
+        %% Store plugin code paths so we can remove them when compiling project apps
+        State4 = rebar_state:update_code_paths(State3, all_plugin_deps, CodePaths++NewCodePaths),
+
+        {plugin_providers(Plugin), State4}
     catch
         C:T ->
             ?DEBUG("~p ~p ~p", [C, T, erlang:get_stacktrace()]),
             ?WARN("Plugin ~p not available. It will not be used.", [Plugin]),
-            []
+            {[], State}
     end.
 
 build_plugin(AppInfo) ->
@@ -65,6 +84,8 @@ build_plugin(AppInfo) ->
     S = rebar_state:new(rebar_state:new(), C, AppDir),
     rebar_prv_compile:compile(S, [], AppInfo).
 
+plugin_providers({Plugin, _, _, _}) when is_atom(Plugin) ->
+    validate_plugin(Plugin);
 plugin_providers({Plugin, _, _}) when is_atom(Plugin) ->
     validate_plugin(Plugin);
 plugin_providers({Plugin, _}) when is_atom(Plugin) ->

src/rebar_prv_compile.erl

@@ -32,6 +32,8 @@ init(State) ->
 -spec do(rebar_state:t()) -> {ok, rebar_state:t()} | {error, string()}.
 do(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),
 
     ProjectApps = rebar_state:project_apps(State),

src/rebar_prv_install_deps.erl

@@ -37,7 +37,10 @@
 
 -export([handle_deps/3,
          handle_deps/4,
-         handle_deps/5]).
+         handle_deps/5,
+
+         find_cycles/1,
+         cull_compile/2]).
 
 -export_type([dep/0]).
 
@@ -76,6 +79,10 @@ do(State) ->
         {Apps, State1} =
             lists:foldl(fun deps_per_profile/2, {[], State}, lists:reverse(Profiles)),
 
+        State2 = rebar_state:update_all_deps(State1, Apps),
+        CodePaths = [rebar_app_info:ebin_dir(A) || A <- Apps],
+        State3 = rebar_state:update_code_paths(State2, all_deps, CodePaths),
+
         Source = ProjectApps ++ Apps,
         case find_cycles(Source) of
             {cycles, Cycles} ->
@@ -84,7 +91,7 @@ do(State) ->
                 {error, Error};
             {no_cycle, Sorted} ->
                 ToCompile = cull_compile(Sorted, ProjectApps),
-                {ok, rebar_state:deps_to_build(State1, ToCompile)}
+                {ok, rebar_state:deps_to_build(State3, ToCompile)}
         end
     catch
         %% maybe_fetch will maybe_throw an exception to break out of some loops
@@ -158,11 +165,7 @@ handle_deps(Profile, State0, Deps, Upgrade, Locks) ->
                              ,lists:ukeysort(2, SrcApps)
                              ,lists:ukeysort(2, Solved)),
 
-    State5 = rebar_state:update_all_deps(State4, AllDeps),
-    CodePaths = [rebar_app_info:ebin_dir(A) || A <- AllDeps],
-    State6 = rebar_state:update_code_paths(State5, all_deps, CodePaths),
-
-    {ok, AllDeps, State6}.
+    {ok, AllDeps, State4}.
 
 %% ===================================================================
 %% Internal functions
@@ -224,7 +227,22 @@ handle_pkg_dep(Profile, Pkg, Packages, Upgrade, DepsDir, Fetched, Seen, Locks, S
     Level = rebar_app_info:dep_level(AppInfo),
     {NewSeen, NewState} = maybe_lock(Profile, AppInfo, Seen, State, Level),
     {_, AppInfo1} = maybe_fetch(AppInfo, Profile, Upgrade, Seen, NewState),
-    {[AppInfo1 | Fetched], NewSeen, NewState}.
+
+    Profiles = rebar_state:current_profiles(State),
+    Name = rebar_app_info:name(AppInfo1),
+    C = rebar_config:consult(rebar_app_info:dir(AppInfo1)),
+    BaseDir = rebar_state:get(State, base_dir, []),
+    S1 = rebar_state:new(rebar_state:set(rebar_state:new(), base_dir, BaseDir),
+                        C, rebar_app_info:dir(AppInfo1)),
+    S2 = rebar_state:apply_profiles(S1, Profiles),
+    S3 = rebar_state:apply_overrides(S2, Name),
+    AppInfo2 = rebar_app_info:state(AppInfo1, S3),
+
+    %% Dep may have plugins to install. Find and install here.
+    S4 = rebar_plugins:handle_plugins(rebar_state:get(S3, plugins, []), S3),
+    AppInfo3 = rebar_app_info:state(AppInfo2, S4),
+
+    {[AppInfo3 | Fetched], NewSeen, NewState}.
 
 maybe_lock(Profile, AppInfo, Seen, State, Level) ->
     case rebar_app_info:is_checkout(AppInfo) of
@@ -383,14 +401,16 @@ handle_dep(State, DepsDir, AppInfo, Locks, Level) ->
     AppInfo1 = rebar_app_info:state(AppInfo, S3),
 
     %% Dep may have plugins to install. Find and install here.
-    State1 = rebar_plugins:handle_plugins(rebar_state:get(S3, plugins, []), State),
-    Deps = rebar_state:get(S3, deps, []),
+    S4 = rebar_plugins:handle_plugins(rebar_state:get(S3, plugins, []), S3),
+    AppInfo2 = rebar_app_info:state(AppInfo1, S4),
+
     %% Upgrade lock level to be the level the dep will have in this dep tree
+    Deps = rebar_state:get(S4, deps, []),
     NewLocks = [{DepName, Source, LockLevel+Level} ||
-                   {DepName, Source, LockLevel} <- rebar_state:get(S3, {locks, default}, [])],
-    AppInfo2 = rebar_app_info:deps(AppInfo1, rebar_state:deps_names(Deps)),
-    {SrcDeps, PkgDeps} = parse_deps(DepsDir, Deps, S3, Locks, Level+1),
-    {AppInfo2, SrcDeps, PkgDeps, Locks++NewLocks, State1}.
+                   {DepName, Source, LockLevel} <- rebar_state:get(S4, {locks, default}, [])],
+    AppInfo3 = rebar_app_info:deps(AppInfo2, rebar_state:deps_names(Deps)),
+    {SrcDeps, PkgDeps} = parse_deps(DepsDir, Deps, S4, Locks, Level+1),
+    {AppInfo3, SrcDeps, PkgDeps, Locks++NewLocks, State}.
 
 -spec maybe_fetch(rebar_app_info:t(), atom(), boolean(),
                   sets:set(binary()), rebar_state:t()) -> {boolean(), rebar_app_info:t()}.

src/rebar_state.erl

@@ -24,6 +24,7 @@
 
          project_apps/1, project_apps/2,
          deps_to_build/1, deps_to_build/2,
+         all_plugin_deps/1, all_plugin_deps/2, update_all_plugin_deps/2,
          all_deps/1, all_deps/2, update_all_deps/2,
          namespace/1, namespace/2,
 
@@ -55,6 +56,7 @@
 
                   project_apps        = []          :: [rebar_app_info:t()],
                   deps_to_build       = []          :: [rebar_app_info:t()],
+                  all_plugin_deps     = []          :: [rebar_app_info:t()],
                   all_deps            = []          :: [rebar_app_info:t()],
 
                   packages            = undefined   :: {rebar_dict(), rebar_digraph()} | undefined,
@@ -347,6 +349,15 @@ all_deps(#state_t{all_deps=Apps}) ->
 all_deps(State=#state_t{}, NewApps) ->
     State#state_t{all_deps=NewApps}.
 
+all_plugin_deps(#state_t{all_plugin_deps=Apps}) ->
+    Apps.
+
+all_plugin_deps(State=#state_t{}, NewApps) ->
+    State#state_t{all_plugin_deps=NewApps}.
+
+update_all_plugin_deps(State=#state_t{all_plugin_deps=Apps}, NewApps) ->
+    State#state_t{all_plugin_deps=Apps++NewApps}.
+
 update_all_deps(State=#state_t{all_deps=Apps}, NewApps) ->
     State#state_t{all_deps=Apps++NewApps}.
 

src/rebar_utils.erl

@@ -48,6 +48,7 @@
          erl_opts/1,
          indent/1,
          update_code/1,
+         remove_from_code_path/1,
          cleanup_code_path/1,
          args_to_tasks/1,
          expand_env_variable/3,
@@ -581,6 +582,22 @@ update_code(Paths) ->
                           end
                   end, Paths).
 
+remove_from_code_path(Paths) ->
+    lists:foreach(fun(Path) ->
+                          Name = filename:basename(Path, "/ebin"),
+                          App = list_to_atom(Name),
+                          application:load(App),
+                          case application:get_key(App, modules) of
+                              undefined ->
+                                  application:unload(App),
+                                  ok;
+                              {ok, Modules} ->
+                                  application:unload(App),
+                                  [begin code:purge(M), code:delete(M) end || M <- Modules]
+                          end,
+                          code:del_path(Path)
+                  end, Paths).
+
 cleanup_code_path(OrigPath) ->
     CurrentPath = code:get_path(),
     AddedPaths = CurrentPath -- OrigPath,

test/mock_pkg_resource.erl

@@ -73,6 +73,7 @@ mock_vsn(_Opts) ->
 %%   into a `rebar.config' file to describe dependencies.
 mock_download(Opts) ->
     Deps = proplists:get_value(pkgdeps, Opts, []),
+    Config = proplists:get_value(config, Opts, []),
     meck:expect(
         ?MOD, download,
         fun (Dir, {pkg, AppBin, Vsn}, _) ->
@@ -83,7 +84,7 @@ mock_download(Opts) ->
                 Dir, App, binary_to_list(Vsn),
                 [kernel, stdlib] ++ [element(1,D) || D  <- AppDeps]
             ),
-            rebar_test_utils:create_config(Dir, [{deps, AppDeps}]),
+            rebar_test_utils:create_config(Dir, [{deps, AppDeps}]++Config),
             TarApp = App++"-"++binary_to_list(Vsn)++".tar",
             Tarball = filename:join([Dir, TarApp]),
             Contents = filename:join([Dir, "contents.tar.gz"]),

test/rebar_compile_SUITE.erl

@@ -20,6 +20,7 @@
          checkout_priority/1,
          compile_plugins/1,
          compile_global_plugins/1,
+         complex_plugins/1,
          highest_version_of_pkg_dep/1,
          parse_transform_test/1]).
 
@@ -49,7 +50,7 @@ all() ->
      recompile_when_opts_change, dont_recompile_when_opts_dont_change,
      dont_recompile_yrl_or_xrl, delete_beam_if_source_deleted,
      deps_in_path, checkout_priority, compile_plugins, compile_global_plugins,
-     highest_version_of_pkg_dep, parse_transform_test].
+     complex_plugins, highest_version_of_pkg_dep, parse_transform_test].
 
 build_basic_app(Config) ->
     AppDir = ?config(apps, Config),
@@ -408,17 +409,20 @@ compile_plugins(Config) ->
 
     DepName = rebar_test_utils:create_random_name("dep1_"),
     PluginName = rebar_test_utils:create_random_name("plugin1_"),
-    mock_git_resource:mock([{config, [{plugins, [
-                                                {list_to_atom(PluginName), Vsn}
-                                                ]}]}]),
-    mock_pkg_resource:mock([
-                           {pkgdeps, [{{iolist_to_binary(PluginName), iolist_to_binary(Vsn)}, []}]}
-                           ]),
+
+    Plugins = rebar_test_utils:expand_deps(git, [{PluginName, Vsn, []}]),
+    mock_git_resource:mock([{deps, rebar_test_utils:flat_deps(Plugins)}]),
+
+    mock_pkg_resource:mock([{pkgdeps, [{{list_to_binary(DepName), list_to_binary(Vsn)}, []}]},
+                            {config, [{plugins, [
+                                                {list_to_atom(PluginName),
+                                                 {git, "http://site.com/user/"++PluginName++".git",
+                                                 {tag, Vsn}}}]}]}]),
 
     RConfFile =
         rebar_test_utils:create_config(AppDir,
                                       [{deps, [
-                                              {list_to_atom(DepName), {git, "http://site.com/user/"++DepName++".git", {tag, Vsn}}}
+                                              list_to_atom(DepName)
                                               ]}]),
     {ok, RConf} = file:consult(RConfFile),
 
@@ -480,6 +484,49 @@ compile_global_plugins(Config) ->
 
     meck:unload(rebar_dir).
 
+%% Tests installing of plugin with transitive deps
+complex_plugins(Config) ->
+    AppDir = ?config(apps, Config),
+
+    meck:new(rebar_dir, [passthrough]),
+
+    Name = rebar_test_utils:create_random_name("app1_"),
+    Vsn = rebar_test_utils:create_random_vsn(),
+    Vsn2 = rebar_test_utils:create_random_vsn(),
+    rebar_test_utils:create_app(AppDir, Name, Vsn, [kernel, stdlib]),
+
+    DepName = rebar_test_utils:create_random_name("dep1_"),
+    DepName2 = rebar_test_utils:create_random_name("dep2_"),
+    DepName3 = rebar_test_utils:create_random_name("dep3_"),
+    PluginName = rebar_test_utils:create_random_name("plugin1_"),
+
+    Deps = rebar_test_utils:expand_deps(git, [{PluginName, Vsn2, [{DepName2, Vsn,
+                                                                  [{DepName3, Vsn, []}]}]}
+                                             ,{DepName, Vsn, []}]),
+    mock_git_resource:mock([{deps, rebar_test_utils:flat_deps(Deps)}]),
+
+    RConfFile =
+        rebar_test_utils:create_config(AppDir,
+                                       [{deps, [
+                                               {list_to_atom(DepName), {git, "http://site.com/user/"++DepName++".git", {tag, Vsn}}}
+                                               ]},
+                                       {plugins, [
+                                                 {list_to_atom(PluginName), {git, "http://site.com/user/"++PluginName++".git", {tag, Vsn2}}}
+                                                 ]}]),
+    {ok, RConf} = file:consult(RConfFile),
+
+    %% Build with deps.
+    rebar_test_utils:run_and_check(
+        Config, RConf, ["compile"],
+        {ok, [{app, Name},
+              {plugin, PluginName, Vsn2},
+              {plugin, DepName2},
+              {plugin, DepName3},
+              {dep, DepName}]}
+     ),
+
+    meck:unload(rebar_dir).
+
 highest_version_of_pkg_dep(Config) ->
     AppDir = ?config(apps, Config),
 

test/rebar_test_utils.erl

@@ -70,7 +70,8 @@ run_and_check(Config, RebarConfig, Command, Expect) ->
 %% - src/<file>.app.src
 %% And returns a `rebar_app_info' object.
 create_app(AppDir, Name, Vsn, Deps) ->
-    write_src_file(AppDir, Name),
+    write_src_file(AppDir, Name ++ ".erl"),
+    write_src_file(AppDir, "not_a_real_src_" ++ Name ++ ".erl"),
     write_app_src_file(AppDir, Name, Vsn, Deps),
     rebar_app_info:new(Name, Vsn, AppDir, Deps).
 
@@ -297,9 +298,9 @@ check_results(AppDir, Expected) ->
         end, Expected).
 
 write_src_file(Dir, Name) ->
-    Erl = filename:join([Dir, "src", "not_a_real_src_" ++ Name ++ ".erl"]),
+    Erl = filename:join([Dir, "src", Name]),
     ok = filelib:ensure_dir(Erl),
-    ok = ec_file:write(Erl, erl_src_file("not_a_real_src_" ++ Name ++ ".erl")).
+    ok = ec_file:write(Erl, erl_src_file(Name)).
 
 write_eunitized_src_file(Dir, Name) ->
     Erl = filename:join([Dir, "src", "not_a_real_src_" ++ Name ++ ".erl"]),