Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add override_for to dep options #14156

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/mix/lib/mix/dep.ex
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,7 @@ defmodule Mix.Dep do
if dep.opts[:from_umbrella] || other.opts[:from_umbrella] do
"Please remove the conflicting options from your definition"
else
"Ensure they match or specify one of the above in your deps and set \"override: true\""
"Ensure they match or specify one of the above in your deps and set \"override: true\" or \"override_for:\""
end
end

Expand Down
6 changes: 5 additions & 1 deletion lib/mix/lib/mix/dep/converger.ex
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@ defmodule Mix.Dep.Converger do
end

cond do
in_upper? && other_opts[:override] ->
in_upper? && (other_opts[:override] || override_for?(other_opts, app)) ->
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was most confused in this section of the converge logic.

If a dep passes this check once, does it never fail/process again? I initially had a test where I wanted dependency resolution to fail if override_for: [:foo] is specified, but :baz also required the override.

{:match, list}

not converge?(other, dep) ->
Expand Down Expand Up @@ -331,6 +331,10 @@ defmodule Mix.Dep.Converger do
end
end

defp override_for?(opts, app) do
!!opts[:override_for] && app in opts[:override_for]
end

defp with_matching_only_and_targets(other, dep, in_upper?) do
%{opts: opts} = dep

Expand Down
39 changes: 39 additions & 0 deletions lib/mix/lib/mix/dep/fetcher.ex
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,8 @@ defmodule Mix.Dep.Fetcher do
Mix.Dep.Lock.write(lock, opts)
mark_as_fetched(parent_deps)

evaluate_overrides(Enum.filter(deps, & &1.opts[:override_for]), deps)

# See if any of the deps diverged and abort.
show_diverged!(Enum.filter(all_deps, &Mix.Dep.diverged?/1))

Expand Down Expand Up @@ -156,6 +158,43 @@ defmodule Mix.Dep.Fetcher do
end)
end

defp evaluate_overrides([], _deps), do: :ok

defp evaluate_overrides(override_deps, deps) do
for override_dep <- override_deps,
dep <- deps,
dep_dependency <- dep.deps,
override_dep.app == dep_dependency.app do
overridden? = dep.app in override_dep.opts[:override_for]

{:ok, satisfied?} =
Mix.Dep.Loader.vsn_match(dep_dependency.requirement, override_dep.requirement, dep.app)

cond do
overridden? && satisfied? ->
show_expired_override(override_dep, dep.app)

not overridden? && not satisfied? ->
show_missing_override(override_dep, dep.app)

true ->
:ok
end
end

:ok
end

defp show_expired_override(dep, app) do
Mix.shell().info(
"Dependency #{Mix.Dep.format_dep(dep)} no longer requires override_for on #{app}"
)
end

defp show_missing_override(dep, app) do
Mix.shell().info("Dependency #{Mix.Dep.format_dep(dep)} requires override_for on #{app}")
end

defp show_diverged!([]), do: :ok

defp show_diverged!(deps) do
Expand Down
11 changes: 11 additions & 0 deletions lib/mix/test/fixtures/deps_status/custom/override_for_repo/mix.exs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
defmodule OverrideForRepo do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding a new fixture, could we use one of the existing ones? In particular, some fixtures allows you to configure them dynamically by passing some options.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to find a fixture that would let me configure it, but I'm only seeing opts as configurable, and not the actual requirement for the dependencies themselves to force an override requirement.

I ended up just having override_for: [] and using git_repo and deps_repo like most of the other tests currently do.

use Mix.Project

def project do
[
app: :override_for_repo,
version: "0.1.0",
deps: [{:git_repo, "0.3.0", [git: MixTest.Case.fixture_path("git_repo")]}]
]
end
end
43 changes: 43 additions & 0 deletions lib/mix/test/mix/dep_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -1240,6 +1240,49 @@ defmodule Mix.DepTest do
end
end

describe "override_for" do
test "warns when overrides are no longer needed for an app" do
deps = [
{:deps_repo, "0.1.0", path: "custom/deps_repo"},
{:git_repo, "0.1.0",
git: MixTest.Case.fixture_path("git_repo"), override_for: [:deps_repo]}
]

with_deps(deps, fn ->
in_fixture("deps_status", fn ->
Mix.Tasks.Deps.Get.run([])
Mix.Tasks.Deps.Compile.run([])
end)
end)

message =
"Dependency git_repo (#{fixture_path("git_repo")}) no longer requires override_for on deps_repo"

assert_received {:mix_shell, :info, [^message]}
end

test "errors when more overrides are needed for an app" do
deps = [
{:deps_repo, "0.1.0", path: "custom/deps_repo"},
{:override_for_repo, "0.1.0", path: "custom/override_for_repo"},
{:git_repo, "0.2.0",
git: MixTest.Case.fixture_path("git_repo"), override_for: [:deps_repo]}
]

with_deps(deps, fn ->
in_fixture("deps_status", fn ->
Mix.Tasks.Deps.Get.run([])
Mix.Tasks.Deps.Compile.run([])
end)
end)

message =
"Dependency git_repo (#{fixture_path("git_repo")}) requires override_for on override_for_repo"

assert_received {:mix_shell, :info, [^message]}
end
end

describe "app generation" do
test "considers runtime from current app on nested deps" do
Process.put(:custom_deps_git_repo_opts, runtime: false)
Expand Down