handle new tests for mix package types (git/pkg)

4 files changed, 111 insertions, 94 deletions

diff --git a/src/rebar_app_discover.erl b/src/rebar_app_discover.erl
index 63429b5..ae0fd73 100644
--- a/src/rebar_app_discover.erl
+++ b/src/rebar_app_discover.erl
@@ -33,9 +33,10 @@ do(State, LibDirs) ->
     %% Handle top level deps
     State1 = lists:foldl(fun(Profile, StateAcc) ->
                                  ProfileDeps = rebar_state:get(StateAcc, {deps, Profile}, []),
+                                 ProfileDeps2 = rebar_utils:tup_dedup(rebar_utils:tup_sort(ProfileDeps)),
                                  ParsedDeps = parse_profile_deps(Profile
                                                                 ,TopLevelApp
-                                                                ,ProfileDeps
+                                                                ,ProfileDeps2
                                                                 ,StateAcc),
                                  rebar_state:set(StateAcc, {parsed_deps, Profile}, ParsedDeps)
                          end, State, lists:reverse(CurrentProfiles)),
@@ -93,9 +94,10 @@ handle_profile(Profile, Name, AppState, State) ->
     {TopSrc, TopPkg} = rebar_state:get(State, {parsed_deps, Profile}, {[], []}),
     TopLevelProfileDeps = rebar_state:get(State, {deps, Profile}, []),
     AppProfileDeps = rebar_state:get(AppState, {deps, Profile}, []),
+    AppProfileDeps2 = rebar_utils:tup_dedup(rebar_utils:tup_sort(AppProfileDeps)),
     ProfileDeps2 = rebar_utils:tup_dedup(rebar_utils:tup_umerge(
                                            rebar_utils:tup_sort(TopLevelProfileDeps)
-                                           ,rebar_utils:tup_sort(AppProfileDeps))),
+                                           ,rebar_utils:tup_sort(AppProfileDeps2))),
     State1 = rebar_state:set(State, {deps, Profile}, ProfileDeps2),
 
     %% Only deps not also specified in the top level config need
diff --git a/src/rebar_digraph.erl b/src/rebar_digraph.erl
index 1132a6c..f2bb540 100644
--- a/src/rebar_digraph.erl
+++ b/src/rebar_digraph.erl
@@ -3,6 +3,7 @@
 -export([compile_order/1
         ,restore_graph/1
         ,cull_deps/2
+        ,cull_deps/3
         ,subgraph/2
         ,format_error/1]).
 
@@ -69,8 +70,12 @@ restore_graph({Vs, Es}) ->
 %% The first dep while traversing the graph is chosen and any conflicting
 %% dep encountered later on is ignored.
 cull_deps(Graph, Vertices) ->
-    {Solution, Levels} = build_initial_dicts(Vertices),
-    cull_deps(Graph, Vertices, Levels, Solution, []).
+    cull_deps(Graph, Vertices, sets:new()).
+
+cull_deps(Graph, Vertices, Seen) ->
+    Vertices1 = lists:keysort(2, Vertices),
+    {Solution, Levels, Discarded} = {dict:new(), dict:new(), []},
+    cull_deps(Graph, Vertices1, Levels, Solution, Seen, Discarded).
 
 format_error(no_solution) ->
     io_lib:format("No solution for packages found.", []).
@@ -79,79 +84,109 @@ format_error(no_solution) ->
 %% Internal Functions
 %%====================================================================
 
-cull_deps(_Graph, [], Levels, Solution, Discarded) ->
+cull_deps(_Graph, [], Levels, Solution, _, Discarded) ->
     {_, Vertices} = lists:unzip(dict:to_list(Solution)),
-    LvlVertices = [{Profile, {Parent,App,Vsn,dict:fetch(App,Levels)}} || {Profile, {Parent,App,Vsn}} <- Vertices],
+    LvlVertices = [{Profile, {Parent, App, Vsn, dict:fetch(App, Levels)}}
+                  || {Profile, {Parent,App,Vsn}} <- Vertices],
     {ok, LvlVertices, Discarded};
-cull_deps(Graph, [{Profile, Level, Vs} | Vertices], Levels, Solution, Discarded) ->
+cull_deps(Graph, [{Profile, Level, Vs} | Vertices], Levels, Solution, Seen, Discarded) ->
     {NV, NS, LS, DS} =
-        lists:foldl(fun({_, Name, Vsn}, {Acc, SolutionAcc, LevelsAcc, DiscardedAcc}) ->
-                            OutNeighbors = lists:keysort(1, digraph:out_neighbours(Graph, {Name,Vsn})),
-                            handle_neighbors(Profile, Level, Name
-                                            ,OutNeighbors, Acc, SolutionAcc
-                                            ,LevelsAcc, DiscardedAcc)
-
-                    end, {[], Solution, Levels, Discarded}, lists:keysort(2, Vs)),
-
-    cull_deps(Graph, Vertices++NV, LS, NS, DS).
+        lists:foldl(fun({Parent, Name, Vsn}, {Acc, SolutionAcc, LevelsAcc, DiscardedAcc}) ->
+                            {SolutionAcc1, LevelsAcc1, DiscardedAcc1} =
+                                maybe_add_to_solution(Profile, Level, Name, {Name, Vsn}, Parent
+                                                     ,SolutionAcc
+                                                     ,LevelsAcc, Seen, DiscardedAcc),
+                            OutNeighbors = digraph:out_neighbours(Graph, {Name,Vsn}),
+                            {NewVertices, DiscardedAcc2} = handle_neighbors(Profile, Level, Name
+                                                          ,OutNeighbors, Acc, SolutionAcc1
+                                                                           ,Seen, DiscardedAcc1),
+                            {NewVertices, SolutionAcc1, LevelsAcc1, DiscardedAcc2}
+                    end, {[], Solution, Levels, Discarded}, Vs),
+    NewVertices = combine_profile_levels(Vertices, NV),
+    cull_deps(Graph, NewVertices, LS, NS, Seen, DS).
+
+%% Combine lists of deps that have the same profile and level
+combine_profile_levels(Vertices, NewVertices) ->
+    V = lists:foldl(fun({Profile, Level, Vs}, Acc) ->
+                        case ec_lists:find(fun({P, L, _}) when P =:= Profile
+                                                             , L =:= Level ->
+                                                   true;
+                                              (_) ->
+                                                   false
+                                           end, Acc) of
+                            {ok, {_, _, OldVs}=Old} ->
+                                lists:delete(Old, Acc)++[{Profile, Level, lists:keysort(1, OldVs++Vs)}];
+                            error ->
+                                Acc++[{Profile, Level, Vs}]
+                        end
+                end, Vertices, NewVertices),
+    lists:keysort(2, V).
 
 %% For each outgoing edge of a dep check if it should be added to the solution
 %% and add it to the list of vertices to do the same for
 handle_neighbors(Profile, Level, Parent, OutNeighbors, Vertices
-                ,Solution, Levels, Discarded) ->
-        case lists:foldl(fun({Name, _}=N, {NewVertices, Solution1, Levels1, Discarded1}) ->
-                            maybe_add_to_solution(Profile, Level, Name, N, Parent
-                                                 ,NewVertices, Solution1
-                                                 ,Levels1, Discarded1)
-                    end, {[], Solution, Levels, Discarded}, OutNeighbors) of
-            {[], SolutionAcc2, LevelsAcc2, DiscardedAcc2} ->
-                {Vertices, SolutionAcc2, LevelsAcc2, DiscardedAcc2};
-            {NewVertices1, SolutionAcc2, LevelsAcc2, DiscardedAcc2} ->
-                {Vertices++[{Profile, Level+1, NewVertices1}]
-                ,SolutionAcc2, LevelsAcc2, DiscardedAcc2}
+                ,Solution, Seen, Discarded) ->
+    case lists:foldl(fun({Name, Vsn}=Value, {NewVertices, Discarded1}) ->
+                             case dict:find(Name, Solution) of
+                                 {ok, {Profile, {Parent, Name, Vsn}}} -> % already seen
+                                     {NewVertices,
+                                      Discarded1};
+                                 {ok, _} -> % conflict resolution!
+                                     %% Warn on different version
+                                     {NewVertices,
+                                      [Value|Discarded1]};
+                                 error ->
+                                     %% We check Seen separately because we don't care
+                                     %% to warn if the exact same version of a package
+                                     %% was already part of the solution but we do
+                                     %% if it was simply seen in source deps
+                                     case sets:is_element(Name, Seen) of
+                                         true ->
+                                             {NewVertices,
+                                              [Value|Discarded1]};
+                                         false ->
+                                             {[{Parent, Name, Vsn} | NewVertices],
+                                              Discarded1}
+                                     end
+                             end
+                    end, {[], Discarded}, OutNeighbors) of
+            {[], DiscardedAcc2} ->
+                {Vertices, DiscardedAcc2};
+            {NewVertices1, DiscardedAcc2} ->
+                {Vertices++[{Profile, Level+1, NewVertices1}] ,DiscardedAcc2}
         end.
 
 maybe_add_to_solution(Profile, Level, Key, {Name, Vsn}=Value, Parent
-                     ,Vertices ,Solution, Levels, Discarded) ->
+                     ,Solution, Levels, Seen, Discarded) ->
     case dict:find(Key, Solution) of
         {ok, {Profile, {Parent, Name, Vsn}}} -> % already seen
-            {Vertices,
-             Solution,
+            {Solution,
              Levels,
              Discarded};
         {ok, _} -> % conflict resolution!
-            {Vertices,
-             Solution,
+            %% Warn on different version
+            {Solution,
              Levels,
              [Value|Discarded]};
         error ->
-            {[{Parent, Name, Vsn} | Vertices],
-             dict:store(Key, {Profile, {Parent, Name, Vsn}}, Solution),
-             dict:store(Key, Level+1, Levels),
-             Discarded}
+            %% We check Seen separately because we don't care to warn if the exact
+            %% same version of a package was already part of the solution but we do
+            %% if it was simply seen in source deps
+            case sets:is_element(Name, Seen) of
+                true ->
+                    {Solution,
+                     Levels,
+                     [Value|Discarded]};
+                false ->
+                    {dict:store(Key, {Profile, {Parent, Name, Vsn}}, Solution),
+                     dict:store(Key, Level, Levels),
+                     Discarded}
+            end
     end.
 
 subgraph(Graph, Vertices) ->
     digraph_utils:subgraph(Graph, Vertices).
 
-maybe_add_to_dict(Key, Value, Dict) ->
-    case dict:is_key(Key, Dict) of
-        true ->
-            Dict;
-        false ->
-            dict:store(Key, Value, Dict)
-    end.
-
-%% Track the profile (so we know where to install it), name/vsn of each dep
-%% and the level it is from (for the lock file)
-build_initial_dicts(Vertices) ->
-    lists:foldl(fun({Profile, Level, Vs}, {Solution, Levels}) ->
-                        lists:foldl(fun({Parent, Key, Vsn}, {SAcc, LAcc}) ->
-                                            {maybe_add_to_dict(Key, {Profile, {Parent,Key,Vsn}}, SAcc),
-                                             maybe_add_to_dict(Key, Level, LAcc)}
-                                    end, {Solution, Levels}, Vs)
-                end, {dict:new(), dict:new()}, Vertices).
-
 -spec names_to_apps([atom()], [rebar_app_info:t()]) -> [rebar_app_info:t()].
 names_to_apps(Names, Apps) ->
     [element(2, App) || App <- [find_app_by_name(Name, Apps) || Name <- Names], App =/= error].
diff --git a/src/rebar_prv_install_deps.erl b/src/rebar_prv_install_deps.erl
index 74de109..ad469c3 100644
--- a/src/rebar_prv_install_deps.erl
+++ b/src/rebar_prv_install_deps.erl
@@ -193,9 +193,12 @@ handle_profile_pkg_level(PkgDeps, AllApps, Seen, Upgrade, Locks, State) ->
     State1 = rebar_state:packages(rebar_state:registry(State, Registry)
                                  ,{Packages, Graph}),
 
-    S = case rebar_digraph:cull_deps(Graph, lists:keysort(2, PkgDeps)) of
-            {ok, [], _} ->
+    S = case rebar_digraph:cull_deps(Graph, lists:keysort(2, PkgDeps), Seen) of
+            {ok, [], []} ->
                 throw({rebar_digraph, no_solution});
+            {ok, [], Discarded} ->
+                [warn_skip_pkg(Pkg, State) || Pkg <- Discarded, not(pkg_locked(Pkg, Locks))],
+                [];
             {ok, Solution, []} ->
                 Solution;
             {ok, Solution, Discarded} ->
diff --git a/test/rebar_install_deps_SUITE.erl b/test/rebar_install_deps_SUITE.erl
index 3fabc56..66c762e 100644
--- a/test/rebar_install_deps_SUITE.erl
+++ b/test/rebar_install_deps_SUITE.erl
@@ -18,8 +18,7 @@ groups() ->
         m_flat1, m_flat2, m_circular1, m_circular2, m_circular3,
         m_pick_source1, m_pick_source2, m_pick_source3,
         m_pick_source4, m_pick_source5, m_source_to_pkg,
-        m_pkg_level1, m_pkg_level2,
-        m_pkg_src_override
+        m_pkg_level1, m_pkg_level2, m_pkg_level3, m_pkg_level3_alpha_order
      ]}
     ].
 
@@ -221,12 +220,12 @@ mdeps(m_pick_source3) ->
     %% The order of declaration is important.
     {[{"b", []},
       {"B", []}],
-     ["B"],
+     [],
      {ok, ["b"]}};
 mdeps(m_pick_source4) ->
     {[{"B", []},
       {"b", []}],
-     ["b"],
+     [],
      {ok, ["B"]}};
 mdeps(m_pick_source5) ->
     {[{"B", [{"d", []}]},
@@ -247,10 +246,16 @@ mdeps(m_pkg_level2) ->
       {"C", [{"D", [{"e", "2", []}]}]}],
      [{"e","2"}],
      {ok, ["B","C","D",{"e","1"}]}};
-mdeps(m_pkg_src_override) ->
-    %% This is all handled in setup_project
-    {[],[],{ok,[]}}.
-
+mdeps(m_pkg_level3_alpha_order) ->
+    {[{"B", [{"d", [{"f", "1", []}]}]},
+      {"C", [{"E", [{"f", "2", []}]}]}],
+     [{"f","2"}],
+     {ok, ["B","C","d","E",{"f","1"}]}};
+mdeps(m_pkg_level3) ->
+    {[{"B", [{"d", [{"f", "1", []}]}]},
+      {"C", [{"E", [{"G", [{"f", "2", []}]}]}]}],
+     [{"f","2"}],
+     {ok, ["B","C","d","E","G",{"f","1"}]}}.
 
 setup_project(fail_conflict, Config0, Deps) ->
     DepsType = ?config(deps_type, Config0),
@@ -309,23 +314,6 @@ setup_project(nondefault_pick_highest, Config0, _) ->
             mock_pkg_resource:mock([{pkgdeps, PkgDeps}])
     end,
     [{rebarconfig, RebarConf} | Config];
-setup_project(m_pkg_src_override, Config0, _) ->
-    Config = rebar_test_utils:init_rebar_state(Config0, "m_pkg_src_override_mixed_"),
-    AppDir = ?config(apps, Config),
-    rebar_test_utils:create_app(AppDir, "A", "0.0.0", [kernel, stdlib]),
-    DefaultDeps = rebar_test_utils:expand_deps(mixed, [{"b", [{"c", []}]}]),
-    OverrideDeps = rebar_test_utils:expand_deps(mixed, [{"C", []}]),
-    DefaultTop = rebar_test_utils:top_level_deps(DefaultDeps),
-    OverrideTop = rebar_test_utils:top_level_deps(OverrideDeps),
-    RebarConf = rebar_test_utils:create_config(
-            AppDir,
-            [{deps, DefaultTop},
-             {overrides, [{override, b, [{deps, OverrideTop}]}]}]
-    ),
-    {SrcDeps,PkgDeps} = rebar_test_utils:flat_deps(DefaultDeps++OverrideDeps),
-    mock_git_resource:mock([{deps, SrcDeps}]),
-    mock_pkg_resource:mock([{pkgdeps, PkgDeps}]),
-    [{rebarconfig, RebarConf} | Config];
 setup_project(Case, Config0, Deps) ->
     DepsType = ?config(deps_type, Config0),
     Config = rebar_test_utils:init_rebar_state(
@@ -459,19 +447,8 @@ m_pick_source5(Config) -> run(Config).
 m_source_to_pkg(Config) -> run(Config).
 m_pkg_level1(Config) -> run(Config).
 m_pkg_level2(Config) -> run(Config).
-
-
-m_pkg_src_override(Config) ->
-    %% Detect the invalid override where a package dep's are overriden
-    %% with source dependencies. We only test with overrides because
-    %% we trust the package index to be correct there and not introduce
-    %% that kind of error.
-    {ok, RebarConfig} = file:consult(?config(rebarconfig, Config)),
-    rebar_test_utils:run_and_check(
-        Config, RebarConfig, ["lock"],
-        {error, {rebar_prv_install_deps,
-                 {package_dep_override, <<"b">>}}}
-    ).
+m_pkg_level3(Config) -> run(Config).
+m_pkg_level3_alpha_order(Config) -> run(Config).
 
 run(Config) ->
     {ok, RebarConfig} = file:consult(?config(rebarconfig, Config)),