Partial fix to circular deps (#40)

- Adding tests
- fixing use of set fetching to find repeated deps and prevent infinite
  loops

On a circular loop rebar3 now fails with `{error, no_sort}`, which is
uncaught and should be handled to consider the issue fully fixed.

src/rebar_prv_install_deps.erl

@@ -151,7 +151,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 +167,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 +204,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, no_sort}}; % circular dep
+deps(circular2) ->
+    {[{"B", [{"C", [{"B", []}]}]},
+      {"C", []}],
+     {error, no_sort}}. % circular dep
 
 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}]}.
+