Merge pull request #44 from rebar/fix-circular-deps

Partial fix to circular deps (#40)

src/rebar_digraph.erl

@@ -18,7 +18,15 @@ compile_order(Apps) ->
                   end, Apps),
     case digraph_utils:topsort(Graph) of
         false ->
-            {error, no_sort};
+            case digraph_utils:is_acyclic(Graph) of
+                true ->
+                    {error, no_sort};
+                false ->
+                    Cycles = lists:sort(
+                        [lists:sort(Comp) || Comp <- digraph_utils:strong_components(Graph),
+                                             length(Comp)>1]),
+                    {error, {cycles, Cycles}}
+            end;
         V ->
             {ok, names_to_apps(lists:reverse(V), Apps)}
     end.

src/rebar_prv_install_deps.erl

@@ -81,6 +81,8 @@ do(State) ->
                 {ok, rebar_state:deps_to_build(State1,
                                               lists:dropwhile(fun rebar_app_info:valid/1
                                                              , Sort -- ProjectApps))};
+            {error, {cycles, Cycles}} ->
+                {error, {?MODULE, {cycles, Cycles}}};
             {error, Error} ->
                 {error, Error}
         end
@@ -91,6 +93,12 @@ do(State) ->
     end.
 
 -spec format_error(any()) -> iolist().
+format_error({cycles, Cycles}) ->
+    Prints = [["applications: ",
+               [io_lib:format("~s ", [Dep]) || Dep <- Cycle],
+               "depend on each other~n"]
+              || Cycle <- Cycles],
+    ["Dependency cycle(s) detected:~n", Prints];
 format_error(Reason) ->
     io_lib:format("~p", [Reason]).
 
@@ -151,7 +159,7 @@ update_pkg_deps(Pkgs, Packages, Update, Seen, State) ->
                                                       ,Packages
                                                       ,Pkg),
                               {SeenAcc1, StateAcc1} = maybe_lock(AppInfo, SeenAcc, StateAcc),
-                              case maybe_fetch(StateAcc1, AppInfo, Update, SeenAcc) of
+                              case maybe_fetch(StateAcc1, AppInfo, Update, SeenAcc1) of
                                   true ->
                                       {[AppInfo | Acc], SeenAcc1, StateAcc1};
                                   false ->
@@ -167,7 +175,7 @@ maybe_lock(AppInfo, Seen, State) ->
             {sets:add_element(Name, Seen),
              rebar_state:lock(State, AppInfo)};
         true ->
-            {sets:add_element(Name, Seen), State}
+            {Seen, State}
     end.
 
 package_to_app(DepsDir, Packages, {Name, Vsn}) ->
@@ -204,7 +212,7 @@ update_src_deps(Level, SrcDeps, PkgDeps, SrcApps, State, Update, Seen) ->
                                                               ,Level
                                                               ,StateAcc1);
                                              _ ->
-                                                 maybe_fetch(StateAcc, AppInfo, false, SeenAcc),
+                                                 maybe_fetch(StateAcc, AppInfo, false, SeenAcc1),
                                                  handle_dep(AppInfo
                                                            ,SrcDepsAcc
                                                            ,PkgDepsAcc

test/mock_git_resource.erl

@@ -102,8 +102,8 @@ mock_download(Opts) ->
     meck:expect(
         ?MOD, download,
         fun (Dir, Git) ->
-            {git, Url, {_, Vsn}} = normalize_git(Git, Overrides, Default),
             filelib:ensure_dir(Dir),
+            {git, Url, {_, Vsn}} = normalize_git(Git, Overrides, Default),
             App = app(Url),
             AppDeps = proplists:get_value(App, Deps, []),
             rebar_test_utils:create_app(

test/rebar_compile_SUITE.erl

@@ -33,5 +33,5 @@ build_basic_app(Config) ->
     Vsn = rebar_test_utils:create_random_vsn(),
     rebar_test_utils:create_app(AppDir, Name, Vsn, [kernel, stdlib]),
 
-    rebar_test_utils:run_and_check(Config, [], "compile", [{app, Name}]).
+    rebar_test_utils:run_and_check(Config, [], "compile", {ok, [{app, Name}]}).
 

test/rebar_deps_SUITE.erl

@@ -4,7 +4,8 @@
 -include_lib("common_test/include/ct.hrl").
 -include_lib("eunit/include/eunit.hrl").
 
-all() -> [flat, pick_highest_left, pick_highest_right, pick_earliest].
+all() -> [flat, pick_highest_left, pick_highest_right, pick_earliest,
+          circular1, circular2].
 
 init_per_suite(Config) ->
     application:start(meck),
@@ -15,29 +16,42 @@ end_per_suite(_Config) ->
 
 init_per_testcase(Case, Config) ->
     {Deps, Expect} = deps(Case),
-    [{expect,
-      [case Dep of
+    Expected = case Expect of
+        {ok, List} -> {ok, format_expected_deps(List)};
+        {error, Reason} -> {error, Reason}
+    end,
+    [{expect, Expected} | setup_project(Case, Config, expand_deps(Deps))].
+
+format_expected_deps(Deps) ->
+    [case Dep of
         {N,V} -> {dep, N, V};
         N -> {dep, N}
-       end || Dep <- Expect]}
-     | setup_project(Case, Config, expand_deps(Deps))].
+     end || Dep <- Deps].
 
 deps(flat) ->
     {[{"B", []},
       {"C", []}],
-     ["B", "C"]};
+     {ok, ["B", "C"]}};
 deps(pick_highest_left) ->
     {[{"B", [{"C", "2", []}]},
       {"C", "1", []}],
-     ["B", {"C", "1"}]}; % Warn C2
+     {ok, ["B", {"C", "1"}]}}; % Warn C2
 deps(pick_highest_right) ->
     {[{"B", "1", []},
       {"C", [{"B", "2", []}]}],
-     [{"B","1"}, "C"]}; % Warn B2
+     {ok, [{"B","1"}, "C"]}}; % Warn B2
 deps(pick_earliest) ->
     {[{"B", [{"D", "1", []}]},
       {"C", [{"D", "2", []}]}],
-     ["B","C",{"D","1"}]}. % Warn D2
+     {ok, ["B","C",{"D","1"}]}}; % Warn D2
+deps(circular1) ->
+    {[{"B", [{"A", []}]}, % A is the top-level app
+      {"C", []}],
+     {error, {rebar_prv_install_deps, {cycles, [[<<"A">>,<<"B">>]]}}}};
+deps(circular2) ->
+    {[{"B", [{"C", [{"B", []}]}]},
+      {"C", []}],
+     {error, {rebar_prv_install_deps, {cycles, [[<<"B">>,<<"C">>]]}}}}.
 
 end_per_testcase(_, Config) ->
     mock_git_resource:unmock(),
@@ -55,6 +69,7 @@ expand_deps([{Name, Vsn, Deps} | Rest]) ->
 setup_project(Case, Config0, Deps) ->
     Config = rebar_test_utils:init_rebar_state(Config0, atom_to_list(Case)),
     AppDir = ?config(apps, Config),
+    rebar_test_utils:create_app(AppDir, "A", "0.0.0", [kernel, stdlib]),
     TopDeps = top_level_deps(Deps),
     RebarConf = rebar_test_utils:create_config(AppDir, [{deps, TopDeps}]),
     mock_git_resource:mock([{deps, flat_deps(Deps)}]),
@@ -76,6 +91,8 @@ flat(Config) -> run(Config).
 pick_highest_left(Config) -> run(Config).
 pick_highest_right(Config) -> run(Config).
 pick_earliest(Config) -> run(Config).
+circular1(Config) -> run(Config).
+circular2(Config) -> run(Config).
 
 run(Config) ->
     {ok, RebarConfig} = file:consult(?config(rebarconfig, Config)),

test/rebar_test_utils.erl

@@ -45,27 +45,14 @@ run_and_check(Config, RebarConfig, Command, Expect) ->
     %% Assumes init_rebar_state has run first
     AppDir = ?config(apps, Config),
     State = ?config(state, Config),
-    {ok,_} = rebar3:run(rebar_state:new(State, RebarConfig, AppDir), Command),
-    BuildDir = filename:join([AppDir, "_build", "default", "lib"]),
-    Deps = rebar_app_discover:find_apps([BuildDir], all),
-    DepsNames = [{ec_cnv:to_list(rebar_app_info:name(App)), App} || App <- Deps],
-    lists:foreach(
-        fun({app, Name}) ->
-                [App] = rebar_app_discover:find_apps([AppDir]),
-                ct:pal("Name: ~p", [Name]),
-                ?assertEqual(Name, ec_cnv:to_list(rebar_app_info:name(App)))
-        ;  ({dep, Name}) ->
-                ct:pal("Name: ~p", [Name]),
-                ?assertNotEqual(false, lists:keyfind(Name, 1, DepsNames))
-        ;  ({dep, Name, Vsn}) ->
-                ct:pal("Name: ~p, Vsn: ~p", [Name, Vsn]),
-                case lists:keyfind(Name, 1, DepsNames) of
-                    false ->
-                        error({app_not_found, Name});
-                    {Name, App} ->
-                        ?assertEqual(Vsn, rebar_app_info:original_vsn(App))
-                end
-        end, Expect).
+    Res = rebar3:run(rebar_state:new(State, RebarConfig, AppDir), Command),
+    case Expect of
+        {error, Reason} ->
+            ?assertEqual({error, Reason}, Res);
+        {ok, Expected} ->
+            {ok, _} = Res,
+            check_results(AppDir, Expected)
+    end.
 
 %% @doc Creates a dummy application including:
 %% - src/<file>.erl
@@ -108,6 +95,28 @@ create_random_vsn() ->
 %%%%%%%%%%%%%%%
 %%% Helpers %%%
 %%%%%%%%%%%%%%%
+check_results(AppDir, Expected) ->
+    BuildDir = filename:join([AppDir, "_build", "default", "lib"]),
+    Deps = rebar_app_discover:find_apps([BuildDir], all),
+    DepsNames = [{ec_cnv:to_list(rebar_app_info:name(App)), App} || App <- Deps],
+    lists:foreach(
+        fun({app, Name}) ->
+                [App] = rebar_app_discover:find_apps([AppDir]),
+                ct:pal("Name: ~p", [Name]),
+                ?assertEqual(Name, ec_cnv:to_list(rebar_app_info:name(App)))
+        ;  ({dep, Name}) ->
+                ct:pal("Name: ~p", [Name]),
+                ?assertNotEqual(false, lists:keyfind(Name, 1, DepsNames))
+        ;  ({dep, Name, Vsn}) ->
+                ct:pal("Name: ~p, Vsn: ~p", [Name, Vsn]),
+                case lists:keyfind(Name, 1, DepsNames) of
+                    false ->
+                        error({app_not_found, Name});
+                    {Name, App} ->
+                        ?assertEqual(Vsn, rebar_app_info:original_vsn(App))
+                end
+        end, Expected).
+
 write_src_file(Dir, Name) ->
     Erl = filename:join([Dir, "src", "not_a_real_src" ++ Name ++ ".erl"]),
     ok = filelib:ensure_dir(Erl),
@@ -136,3 +145,4 @@ get_app_metadata(Name, Vsn, Deps) ->
       {included_applications, []},
       {registered, []},
       {applications, Deps}]}.
+