Merge pull request #1715 from ericmj/mix-deps-order

Preserve correct order when deps are overriding

Author: José Valim

lib/mix/lib/mix/deps.ex

@@ -105,22 +105,25 @@ defmodule Mix.Deps do
   defdelegate children(), to: Mix.Deps.Retriever
 
   @doc """
-  Return all dependencies depending on the given dependencies.
+  Return all given dependencies and their depending dependencies.
   """
-  def depending(deps, all_deps // all)
+  def with_depending(deps, all_deps // all) do
+    deps ++ do_with_depending(deps, all_deps)
+      |> Enum.uniq(&(&1.app))
+  end
 
-  def depending([], _all_deps) do
+  defp do_with_depending([], _all_deps) do
     []
   end
 
-  def depending(deps, all_deps) do
+  defp do_with_depending(deps, all_deps) do
     dep_names = Enum.map(deps, fn dep -> dep.app end)
 
     parents = Enum.filter all_deps, fn dep ->
       Enum.any?(dep.deps, fn child_dep -> child_dep.app in dep_names end)
     end
 
-    parents ++ depending(parents, all_deps)
+    do_with_depending(parents, all_deps) ++ parents
   end
 
   @doc """

lib/mix/lib/mix/deps/converger.ex

@@ -83,7 +83,9 @@ defmodule Mix.Deps.Converger do
 
   # Look for an overriding dep in the upper breadths, if
   # found return a new acc without the overriden dep and
-  # with the proper status set on the overrider
+  # with the proper status set on the overrider. The
+  # overrider is moved to the front of the accumulator to
+  # preserve the position of the removed dep.
   defp overriden_deps(acc, upper_breadths, dep) do
     overriden = Enum.any?(upper_breadths, fn(other) ->
       other.app == dep.app
@@ -92,18 +94,21 @@ defmodule Mix.Deps.Converger do
     if overriden do
       Mix.Dep[app: app] = dep
 
-      Enum.map(acc, fn other ->
-        Mix.Dep[app: other_app, opts: other_opts] = other
+      { overrider, acc } =
+        Enum.reduce(acc, { nil , [] }, fn other, { overrider, acc } ->
+          Mix.Dep[app: other_app, opts: other_opts] = other
 
-        cond do
-          app == other_app && (other_opts[:override] || converge?(dep, other)) ->
-            other
-          app == other_app ->
-            other.status({ :overriden, dep })
-          true ->
-            other
-        end
-      end)
+          cond do
+            app == other_app && (other_opts[:override] || converge?(dep, other)) ->
+              { other, acc }
+            app == other_app ->
+              { other.status({ :overriden, dep }), acc }
+            true ->
+              { overrider, [other|acc] }
+          end
+        end)
+
+      [ overrider | Enum.reverse(acc) ]
     end
   end
 

lib/mix/lib/mix/tasks/deps.compile.ex

@@ -29,7 +29,8 @@ defmodule Mix.Tasks.Deps.Compile do
   """
 
   import Mix.Deps, only: [ all: 0, available?: 1, by_name: 2, compile_paths: 1,
-                           depending: 2, format_dep: 1, make?: 1, mix?: 1, rebar?: 1 ]
+                           with_depending: 2, format_dep: 1, make?: 1, mix?: 1,
+                           rebar?: 1 ]
 
   def run(args) do
     Mix.Project.get! # Require the project to be available
@@ -39,8 +40,8 @@ defmodule Mix.Tasks.Deps.Compile do
         do_run(Enum.filter(all, &available?/1), opts)
       { opts, tail, _ } ->
         all_deps = all
-        deps = by_name(tail, all_deps)
-        do_run(deps ++ depending(deps, all_deps), opts)
+        deps = by_name(tail, all_deps) |> with_depending(all_deps)
+        do_run(deps, opts)
     end
   end
 

lib/mix/lib/mix/tasks/deps.update.ex

@@ -22,7 +22,7 @@ defmodule Mix.Tasks.Deps.Update do
   """
 
   import Mix.Deps, only: [ all: 0, all: 2, available?: 1, by_name: 2,
-                           depending: 2, format_dep: 1 ]
+                           with_depending: 2, format_dep: 1 ]
 
   def run(args) do
     Mix.Project.get! # Require the project to be available
@@ -33,9 +33,10 @@ defmodule Mix.Tasks.Deps.Update do
         acc = all(init, &deps_updater/2)
       rest != [] ->
         all_deps = all
-        deps = Enum.map by_name(rest, all_deps), &check_unavailable!/1
-        deps = deps ++ depending(deps, all_deps)
-        { _, acc } = Enum.map_reduce deps, init, &deps_updater/2
+        deps = by_name(rest, all_deps)
+          |> Enum.map(&check_unavailable!/1)
+          |> with_depending(all_deps)
+        { _, acc } = Enum.map_reduce(deps, init, &deps_updater/2)
       true ->
         raise Mix.Error, message: "mix deps.update expects dependencies as arguments or " <>
                                   "the --all option to update all dependencies"

lib/mix/test/mix/deps_test.exs

@@ -48,4 +48,27 @@ defmodule Mix.DepsTest do
   after
     Mix.Project.pop
   end
+
+  defmodule ConvergedDepsApp do
+    def project do
+      [
+        app: :raw_sample,
+        version: "0.1.0",
+        deps: [
+          { :deps_repo, "0.1.0", path: "custom/deps_repo" },
+          { :git_repo, "0.1.0", git: MixTest.Case.fixture_path("git_repo") }
+        ]
+      ]
+    end
+  end
+
+  test "correctly order overriden deps" do
+    Mix.Project.push ConvergedDepsApp
+
+    in_fixture "deps_status", fn ->
+      assert [:git_repo, :deps_repo] == Enum.map(Mix.Deps.all, &(&1.app))
+    end
+  after
+    Mix.Project.pop
+  end
 end

lib/mix/test/mix/tasks/deps_test.exs

@@ -283,6 +283,8 @@ defmodule Mix.Tasks.DepsTest do
       assert_received { :mix_shell, :info, ["* Compiling deps_repo"] }
       assert_received { :mix_shell, :info, ["Generated git_repo.app"] }
 
+      Mix.Task.clear
+
       Mix.Tasks.Deps.Update.run ["--all"]
       assert_received { :mix_shell, :info, ["* Updating deps_repo (0.1.0) [path: \"custom/deps_repo\"]"] }
       assert_received { :mix_shell, :info, ["* Compiling deps_repo"] }