Ensure dependencies are properly skipped when running in another environment

Signed-off-by: José Valim <jose.valim@plataformatec.com.br>

Author: José Valim

lib/mix/lib/mix/dep/converger.ex

@@ -49,16 +49,12 @@ defmodule Mix.Dep.Converger do
   end
 
   defp all(acc, lock, opts, callback) do
-    deps = Mix.Dep.Loader.children()
-    deps = Enum.map(deps, &(%{&1 | top_level: true}))
-    lock_given? = !!lock
+    main = Mix.Dep.Loader.children()
+    main = Enum.map(main, &(%{&1 | top_level: true}))
+    apps = Enum.map(main, &(&1.app))
 
-    # Filter the dependencies per environment. We pass the ones
-    # left out as accumulator and upper breadth to help catch
-    # inconsistencies across environment, specially regarding
-    # the :only option. They are filtered again later.
-    current = Enum.map(deps, &(&1.app))
-    {main, only} = Mix.Dep.Loader.partition_by_env(deps, opts)
+    lock_given? = !!lock
+    env = opts[:env]
 
     # If no lock was given, let's read one to fill in the deps
     lock = lock || Mix.Dep.Lock.read
@@ -68,17 +64,14 @@ defmodule Mix.Dep.Converger do
     # lazily loaded, we need to check for it on every
     # iteration.
     {deps, rest, lock} =
-      all(main, only, [], current, callback, acc, lock, fn dep ->
+      all(main, apps, callback, acc, lock, env, fn dep ->
         if (remote = Mix.RemoteConverger.get) && remote.remote?(dep) do
           {:loaded, dep}
         else
           {:unloaded, dep, nil}
         end
       end)
 
-    # Filter deps per environment once more. If the filtered
-    # dependencies had no conflicts, they are removed now.
-    {deps, _} = Mix.Dep.Loader.partition_by_env(deps, opts)
     diverged? = Enum.any?(deps, &Mix.Dep.diverged?/1)
 
     # Run remote converger if one is available and rerun Mix's
@@ -99,19 +92,26 @@ defmodule Mix.Dep.Converger do
       # In case no lock was given, we will use the local lock
       # which is potentially stale. So remote.deps/2 needs to always
       # check if the data it finds in the lock is actually valid.
-      all(main, [], [], Enum.map(main, &(&1.app)), callback, rest, lock, fn dep ->
-        cond do
-          cached = deps[dep.app] ->
-            {:loaded, cached}
-          true ->
-            {:unloaded, dep, remote.deps(dep, lock)}
+      all(main, apps, callback, rest, lock, env, fn dep ->
+        if cached = deps[dep.app] do
+          {:loaded, cached}
+        else
+          {:unloaded, dep, remote.deps(dep, lock)}
         end
       end)
     else
       {deps, rest, lock}
     end
   end
 
+  defp all(main, apps, callback, rest, lock, env, cache) do
+    {deps, rest, lock} = all(main, [], [], apps, callback, rest, lock, env, cache)
+    # When traversing dependencies, we keep skipped ones to
+    # find conflicts. We remove them now after traversal.
+    {deps, _} = Mix.Dep.Loader.partition_by_env(deps, env)
+    {deps, rest, lock}
+  end
+
   # We traverse the tree of dependencies in a breadth-first
   # fashion. The reason for this is that we converge
   # dependencies, but allow the parent to override any
@@ -151,10 +151,14 @@ defmodule Mix.Dep.Converger do
   # Now, since "d" was specified in a parent project, no
   # exception is going to be raised since d is considered
   # to be the authoritative source.
-  defp all([dep|t], acc, upper_breadths, current_breadths, callback, rest, lock, cache) do
+  defp all([dep|t], acc, upper_breadths, current_breadths, callback, rest, lock, env, cache) do
     cond do
       new_acc = diverged_deps(acc, upper_breadths, dep) ->
-        all(t, new_acc, upper_breadths, current_breadths, callback, rest, lock, cache)
+        all(t, new_acc, upper_breadths, current_breadths, callback, rest, lock, env, cache)
+      Mix.Dep.Loader.skip?(dep, env) ->
+        # We still keep skipped dependencies around to detect conflicts.
+        # They must be rejected after every all iteration.
+        all(t, [dep|acc], upper_breadths, current_breadths, callback, rest, lock, env, cache)
       true ->
         dep =
           case cache.(dep) do
@@ -170,12 +174,15 @@ defmodule Mix.Dep.Converger do
           end
 
         dep = %{dep | deps: reject_non_fullfilled_optional(dep.deps, current_breadths)}
-        {acc, rest, lock} = all(t, [dep|acc], upper_breadths, current_breadths, callback, rest, lock, cache)
-        all(dep.deps, acc, current_breadths, Enum.map(dep.deps, &(&1.app)) ++ current_breadths, callback, rest, lock, cache)
+        {acc, rest, lock} =
+          all(t, [dep|acc], upper_breadths, current_breadths, callback, rest, lock, env, cache)
+
+        new_breadths = Enum.map(dep.deps, &(&1.app)) ++ current_breadths
+        all(dep.deps, acc, current_breadths, new_breadths, callback, rest, lock, env, cache)
     end
   end
 
-  defp all([], acc, _upper, _current, _callback, rest, lock, _cache) do
+  defp all([], acc, _upper, _current, _callback, rest, lock, _env, _cache) do
     {acc, rest, lock}
   end
 

lib/mix/lib/mix/dep/loader.ex

@@ -21,18 +21,18 @@ defmodule Mix.Dep.Loader do
   @doc """
   Partitions loaded dependencies by environment.
   """
-  def partition_by_env(deps, opts) do
-    if env = opts[:env] do
-      Enum.partition(deps, fn
-        %Mix.Dep{status: {:divergedonly, _}} ->
-          true
-        %Mix.Dep{opts: opts} ->
-          only = opts[:only] |> List.wrap |> validate_only!
-          only == [] or env in List.wrap(only)
-      end)
-    else
-      {deps, []}
-    end
+  def partition_by_env(deps, nil), do: {deps, []}
+  def partition_by_env(deps, env), do: Enum.partition(deps, &not skip?(&1, env))
+
+  @doc """
+  Check if a dependency must be skipped according to the environment.
+  """
+  def skip?(_dep, nil), do: false
+  def skip?(%Mix.Dep{status: {:divergedonly, _}}, _), do: false
+  def skip?(%Mix.Dep{opts: opts}, env) do
+    only = opts[:only]
+    validate_only!(only)
+    only != nil and not env in List.wrap(only)
   end
 
   @doc """
@@ -283,7 +283,7 @@ defmodule Mix.Dep.Loader do
   end
 
   defp validate_only!(only) do
-    for entry <- only, not is_atom(entry) do
+    for entry <- List.wrap(only), not is_atom(entry) do
       Mix.raise "Expected :only in dependency to be an atom or a list of atoms, got: #{inspect only}"
     end
     only
@@ -293,7 +293,7 @@ defmodule Mix.Dep.Loader do
     from = Path.absname("mix.exs")
     (Mix.Project.config[:deps] || [])
     |> Enum.map(&to_dep(&1, from))
-    |> partition_by_env(opts)
+    |> partition_by_env(opts[:env])
     |> elem(0)
   end
 

lib/mix/test/mix/dep_test.exs

@@ -255,9 +255,8 @@ defmodule Mix.DepTest do
         deps = Mix.Dep.loaded([])
         assert length(deps) == 2
 
-        deps = Mix.Dep.loaded([env: :prod])
-        assert length(deps) == 1
-        assert Enum.find deps, &match?(%Mix.Dep{app: :foo}, &1)
+        assert [dep] = Mix.Dep.loaded([env: :prod])
+        assert dep.app == :foo
       end
     end
   end

lib/mix/test/mix/umbrella_test.exs

@@ -113,15 +113,19 @@ defmodule Mix.UmbrellaTest do
         end
         """
 
-        # Should work across all environments
+        # Does not fetch when filtered
+        Mix.Tasks.Deps.Get.run ["--only", "dev"]
+        refute_received {:mix_shell, :info, ["* Getting git_repo" <> _]}
+
+        # But works across all environments
         Mix.Tasks.Deps.Get.run []
         assert_received {:mix_shell, :info, ["* Getting git_repo" <> _]}
 
-        # Works on the current environment only
+        # Does not show by default
         Mix.Tasks.Deps.run []
-        refute_received {:mix_shell, :info, ["* git_repo " <> _]}
+        refute_received {:mix_shell, :info, ["* git_repo" <> _]}
 
-        # Works on the other environment only
+        # But shows on proper environment
         Mix.env(:other)
         Mix.Tasks.Deps.run []
         assert_received {:mix_shell, :info, ["* git_repo " <> _]}