diff --git a/lib/hex_solver/package_lister.ex b/lib/hex_solver/package_lister.ex index 4a76abd..2290a52 100644 --- a/lib/hex_solver/package_lister.ex +++ b/lib/hex_solver/package_lister.ex @@ -6,47 +6,80 @@ defmodule HexSolver.PackageLister do defstruct registry: nil, root_dependencies: [], + root_parents: %{}, locked: [], overrides: [], already_prefetched: MapSet.new([{nil, "$lock"}]), - already_returned: %{} + already_returned: %{}, + versions: %{}, + candidates: %{}, + dependencies: %{} + + def new(registry, root_dependencies, locked, overrides) do + %PackageLister{ + registry: registry, + root_dependencies: root_dependencies, + root_parents: root_parents(root_dependencies), + locked: locked, + overrides: overrides + } + end # Prefer packages with few remaining versions so that if there is conflict # later it will be forced quickly def pick_package(lister, package_ranges) do - {package_range, versions} = - package_ranges - |> Enum.sort_by(& &1.name) - |> Enum.map(fn package_range -> - case versions(lister, package_range.repo, package_range.name) do - {:ok, versions} -> - allowed = Enum.filter(versions, &Constraint.allows?(package_range.constraint, &1)) - {package_range, allowed} - - :error -> - throw({__MODULE__, :minimal_versions, package_range}) - end - end) - |> Enum.min_by(fn {_package_range, versions} -> length(versions) end) + pick_package(lister, Enum.sort_by(package_ranges, & &1.name), nil) + end + + defp pick_package(lister, [package_range | package_ranges], best) do + case candidates(lister, package_range) do + {lister, {:ok, count, version}} -> + best = + if best == nil or count < elem(best, 1), + do: {package_range, count, version}, + else: best + + pick_package(lister, package_ranges, best) - {:ok, package_range, List.first(Enum.sort(versions, &Version.prioritize/2))} - catch - :throw, {__MODULE__, :minimal_versions, package_range} -> - {:error, package_range} + {lister, :error} -> + {:error, lister, package_range} + end end - def dependencies_as_incompatibilities(%PackageLister{} = lister, package_repo, package, version) do - {:ok, versions} = versions(lister, package_repo, package) - already_returned = Map.get(lister.already_returned, {package_repo, package}, %{}) + defp pick_package(lister, [], {package_range, _count, version}) do + {:ok, lister, package_range, version} + end - # Dependencies of package keyed on package version - versions_dependencies = - Enum.map(versions, fn version -> - {:ok, dependencies} = dependencies(lister, package_repo, package, version) - {version, dependencies} - end) + # The number of versions a package range allows and the preferred one. The + # same package ranges stay unsatisfied over many decisions, so cache them. + defp candidates(lister, %PackageRange{repo: repo, name: name, constraint: constraint}) do + key = {repo, name, constraint} + + case lister.candidates do + %{^key => candidates} -> + {lister, candidates} - {_version, dependencies} = List.keyfind(versions_dependencies, version, 0) + %{} -> + {lister, versions} = versions(lister, repo, name) + + candidates = + case versions do + {:ok, versions} -> + allowed = Enum.filter(versions, &Constraint.allows?(constraint, &1)) + {:ok, length(allowed), List.first(Enum.sort(allowed, &Version.prioritize/2))} + + :error -> + :error + end + + {%{lister | candidates: Map.put(lister.candidates, key, candidates)}, candidates} + end + end + + def dependencies_as_incompatibilities(%PackageLister{} = lister, package_repo, package, version) do + {lister, {:ok, versions}} = versions(lister, package_repo, package) + {lister, {:ok, dependencies}} = dependencies(lister, package_repo, package, version) + already_returned = Map.get(lister.already_returned, {package_repo, package}, %{}) # Dependencies of package version filtered of overrides and already returned dependencies dependencies = @@ -62,30 +95,31 @@ defmodule HexSolver.PackageLister do end end) - incompatibilities = - Enum.map(dependencies, fn {dependency, - %{ - repo: dependency_repo, - constraint: constraint, - optional: optional - }} -> - version_constraints = - Enum.map(versions_dependencies, fn {version, dependencies} -> - case Map.fetch(dependencies, dependency) do - {:ok, %{constraint: constraint, optional: optional}} -> - {version, {constraint, optional}} - - :error -> - {version, nil} - end - end) - + index = Enum.find_index(versions, &(&1 == version)) + versions = List.to_tuple(versions) + + {incompatibilities, lister} = + Enum.map_reduce(dependencies, lister, fn {dependency, + %{ + repo: dependency_repo, + constraint: constraint, + optional: optional + }}, + lister -> # Find range of versions around the current version for which the # constraint is the same to create an incompatibility based on a # larger set of versions for the parent package. # This optimization let us skip many versions during conflict resolution. - lower = lower_bound(Enum.reverse(version_constraints), version, {constraint, optional}) - upper = upper_bound(version_constraints, version, {constraint, optional}) + run = %{ + repo: package_repo, + package: package, + versions: versions, + dependency: dependency, + constraint: {constraint, optional} + } + + {lister, lower} = lower_bound(lister, run, index - 1, version) + {lister, upper} = upper_bound(lister, run, index + 1) range = %Range{min: lower, max: upper, include_min: !!lower} package_range = %PackageRange{repo: package_repo, name: package, constraint: range} @@ -103,7 +137,7 @@ defmodule HexSolver.PackageLister do optional: optional } - Incompatibility.new([package_term, dependency_term], :dependency) + {Incompatibility.new([package_term, dependency_term], :dependency), lister} end) prefetch = @@ -140,62 +174,71 @@ defmodule HexSolver.PackageLister do {lister, incompatibilities} end - defp lower_bound(versions_dependencies, version, constraint) do - [{version, _} | versions_dependencies] = skip_to_version(versions_dependencies, version) - - skip_to_last_constraint(versions_dependencies, constraint, version) + # The lowest version in the run of versions below the index that depend on + # the dependency with the same constraint, or nil if the run reaches the + # first version + defp lower_bound(lister, _run, -1, _last) do + {lister, nil} end - defp upper_bound(versions_dependencies, version, constraint) do - versions_dependencies = skip_to_version(versions_dependencies, version) - skip_to_after_constraint(versions_dependencies, constraint) + defp lower_bound(lister, run, index, last) do + case same_constraint?(lister, run, index) do + {lister, true} -> lower_bound(lister, run, index - 1, elem(run.versions, index)) + {lister, false} -> {lister, last} + end end - defp skip_to_version([{version, _constraint} | _] = versions_dependencies, version) do - versions_dependencies + # The first version after the run of versions from the index that depend on + # the dependency with the same constraint, or nil if the run reaches the last + # version + defp upper_bound(lister, run, index) when index == tuple_size(run.versions) do + {lister, nil} end - defp skip_to_version([_ | versions_dependencies], version) do - skip_to_version(versions_dependencies, version) + defp upper_bound(lister, run, index) do + case same_constraint?(lister, run, index) do + {lister, true} -> upper_bound(lister, run, index + 1) + {lister, false} -> {lister, elem(run.versions, index)} + end end - defp skip_to_last_constraint( - [{version, constraint} | versions_dependencies], - {_constraint, _optional} = constraint, - _last - ) do - skip_to_last_constraint(versions_dependencies, constraint, version) - end + # Loads the dependencies of the version, so only the versions in a run and + # the one that ends it are loaded + defp same_constraint?(lister, run, index) do + version = elem(run.versions, index) + {lister, {:ok, dependencies}} = dependencies(lister, run.repo, run.package, version) + dependency = run.dependency - defp skip_to_last_constraint([], _constraint, _last) do - nil - end + case dependencies do + %{^dependency => %{constraint: constraint, optional: optional}} -> + {lister, {constraint, optional} == run.constraint} - defp skip_to_last_constraint(_versions_dependencies, _constraint, last) do - last + %{} -> + {lister, false} + end end - defp skip_to_after_constraint( - [{_, constraint}, {version, constraint} | versions_dependencies], - {_constraint, _optional} = constraint - ) do - skip_to_after_constraint([{version, constraint} | versions_dependencies], constraint) - end + @version_1 Elixir.Version.parse!("1.0.0") - defp skip_to_after_constraint([_, {version, _} | _versions_dependencies], _) do - version - end + # Fetch each package's versions once per solve, so the solver works with the + # same versions throughout even if the registry filters them by time + defp versions(%PackageLister{} = lister, repo, package) do + key = {repo, package} - defp skip_to_after_constraint(_, _constraint) do - nil - end + case lister.versions do + %{^key => result} -> + {lister, result} - @version_1 Elixir.Version.parse!("1.0.0") + %{} -> + result = uncached_versions(lister, repo, package) + {%{lister | versions: Map.put(lister.versions, key, result)}, result} + end + end - defp versions(_lister, _repo, "$root"), do: {:ok, [@version_1]} - defp versions(_lister, _repo, "$lock"), do: {:ok, [@version_1]} + defp uncached_versions(_lister, _repo, "$root"), do: {:ok, [@version_1]} + defp uncached_versions(_lister, _repo, "$lock"), do: {:ok, [@version_1]} - defp versions(%PackageLister{} = lister, repo, package) do + defp uncached_versions(%PackageLister{} = lister, repo, package) do root_dependency = find_root_dependency(lister, repo, package) if root_dependency do @@ -205,7 +248,21 @@ defmodule HexSolver.PackageLister do end end - defp dependencies(%PackageLister{} = lister, _repo, "$root", @version_1) do + # Dependencies are loaded as the solver needs them and kept for the solve + defp dependencies(%PackageLister{} = lister, repo, package, version) do + key = {repo, package, version} + + case lister.dependencies do + %{^key => result} -> + {lister, result} + + %{} -> + result = uncached_dependencies(lister, repo, package, version) + {%{lister | dependencies: Map.put(lister.dependencies, key, result)}, result} + end + end + + defp uncached_dependencies(%PackageLister{} = lister, _repo, "$root", @version_1) do lock_dependency = if lister.locked == [] do [] @@ -221,7 +278,7 @@ defmodule HexSolver.PackageLister do {:ok, dependency_map(lock_dependency ++ root_dependencies)} end - defp dependencies(%PackageLister{locked: locked}, _repo, "$lock", @version_1) do + defp uncached_dependencies(%PackageLister{locked: locked}, _repo, "$lock", @version_1) do {:ok, Map.new(locked, fn dependency -> {dependency.name, @@ -235,7 +292,7 @@ defmodule HexSolver.PackageLister do end)} end - defp dependencies(%PackageLister{} = lister, repo, package, version) do + defp uncached_dependencies(%PackageLister{} = lister, repo, package, version) do root_dependency = find_root_dependency(lister, repo, package) if root_dependency do @@ -261,7 +318,16 @@ defmodule HexSolver.PackageLister do end) end - defp find_root_dependency(%PackageLister{root_dependencies: dependencies}, repo, package) do - Enum.find(dependencies, &(&1.repo == repo and &1.name == package and &1.dependencies != [])) + # Root dependencies with sub dependencies stand in for registry packages. + # Index them by package, keeping the first declaration of each. + defp root_parents(dependencies) do + Enum.reduce(dependencies, %{}, fn + %{dependencies: []}, parents -> parents + dependency, parents -> Map.put_new(parents, {dependency.repo, dependency.name}, dependency) + end) + end + + defp find_root_dependency(%PackageLister{root_parents: parents}, repo, package) do + Map.get(parents, {repo, package}) end end diff --git a/lib/hex_solver/solver.ex b/lib/hex_solver/solver.ex index 0845cce..66d12a1 100644 --- a/lib/hex_solver/solver.ex +++ b/lib/hex_solver/solver.ex @@ -110,17 +110,18 @@ defmodule HexSolver.Solver do :done else case PackageLister.pick_package(state.lister, unsatisfied) do - {:ok, package_range, nil} -> + {:ok, lister, package_range, nil} -> # If no version satisfies the constraint then add an incompatibility that indicates that + state = %{state | lister: lister} term = %Term{positive: true, package_range: package_range} incompatibility = Incompatibility.new([term], :no_versions) state = add_incompatibility(state, incompatibility) {:choice, package_range.name, state} - {:ok, %PackageRange{} = package_range, version} -> + {:ok, lister, %PackageRange{} = package_range, version} -> {lister, incompatibilities} = PackageLister.dependencies_as_incompatibilities( - state.lister, + lister, package_range.repo, package_range.name, version @@ -154,7 +155,8 @@ defmodule HexSolver.Solver do state = %{state | solution: solution} {:choice, package_range.name, state} - {:error, %PackageRange{} = package_range} -> + {:error, lister, %PackageRange{} = package_range} -> + state = %{state | lister: lister} package_range = %PackageRange{package_range | constraint: Util.any()} term = %Term{positive: true, package_range: package_range} incompatibility = Incompatibility.new([term], :package_not_found) @@ -361,12 +363,7 @@ defmodule HexSolver.Solver do %{repo: repo, name: package, constraint: version, label: label} end) - lister = %PackageLister{ - registry: registry, - root_dependencies: root_dependencies, - locked: locked, - overrides: overrides - } + lister = PackageLister.new(registry, root_dependencies, locked, overrides) %{ solution: %PartialSolution{}, diff --git a/test/hex_solver/package_lister_test.exs b/test/hex_solver/package_lister_test.exs index e44f889..75c07ce 100644 --- a/test/hex_solver/package_lister_test.exs +++ b/test/hex_solver/package_lister_test.exs @@ -1,9 +1,122 @@ defmodule HexSolver.PackageListerTest do use HexSolver.Case, async: true - alias HexSolver.PackageLister + alias HexSolver.{PackageLister, PackageRange} alias HexSolver.Registry.Process, as: Registry + defmodule CountingRegistry do + @behaviour HexSolver.Registry + + alias HexSolver.Registry.Process, as: Registry + + def versions(repo, package) do + count({:versions, repo, package}) + Registry.versions(repo, package) + end + + def dependencies(repo, package, version) do + count({:dependencies, repo, package, to_string(version)}) + Registry.dependencies(repo, package, version) + end + + defdelegate prefetch(packages), to: Registry + + def calls(), do: Process.get({__MODULE__, :calls}, %{}) + + defp count(call) do + Process.put({__MODULE__, :calls}, Map.update(calls(), call, 1, &(&1 + 1))) + end + end + + describe "pick_package/2" do + test "fetches the versions of each package once" do + Registry.put("foo", "1.0.0", []) + Registry.put("foo", "1.1.0", []) + Registry.prefetch([{nil, "foo"}]) + + lister = PackageLister.new(CountingRegistry, [], [], []) + + package_range = %PackageRange{ + name: "foo", + constraint: HexSolver.Requirement.to_constraint!("~> 1.0") + } + + assert {:ok, lister, ^package_range, version} = + PackageLister.pick_package(lister, [package_range]) + + assert version == v("1.1.0") + + package_range = %PackageRange{ + name: "foo", + constraint: HexSolver.Requirement.to_constraint!("1.0.0") + } + + assert {:ok, _lister, ^package_range, version} = + PackageLister.pick_package(lister, [package_range]) + + assert version == v("1.0.0") + assert CountingRegistry.calls() == %{{:versions, nil, "foo"} => 1} + end + + test "picks the package with the fewest allowed versions, the first by name on a tie" do + for package <- ["foo", "bar", "baz"], version <- ["1.0.0", "1.1.0"] do + Registry.put(package, version, []) + end + + Registry.put("baz", "1.2.0", []) + Registry.prefetch([{nil, "foo"}, {nil, "bar"}, {nil, "baz"}]) + + lister = PackageLister.new(Registry, [], [], []) + foo = package_range("foo", "~> 1.0") + bar = package_range("bar", "~> 1.0") + baz = package_range("baz", "~> 1.0") + + assert {:ok, lister, ^bar, version} = PackageLister.pick_package(lister, [foo, baz, bar]) + assert version == v("1.1.0") + + foo = package_range("foo", "1.0.0") + + assert {:ok, _lister, ^foo, version} = PackageLister.pick_package(lister, [baz, bar, foo]) + assert version == v("1.0.0") + end + + test "reuses the candidates of a package range" do + Registry.put("foo", "1.0.0", []) + Registry.put("foo", "1.1.0", []) + Registry.put("repo", "foo", "2.0.0", []) + Registry.prefetch([{nil, "foo"}, {"repo", "foo"}]) + + lister = PackageLister.new(Registry, [], [], []) + foo = package_range("foo", "~> 1.0") + + assert {:ok, lister, ^foo, version} = PackageLister.pick_package(lister, [foo]) + assert version == v("1.1.0") + assert lister.candidates == %{{nil, "foo", foo.constraint} => {:ok, 2, v("1.1.0")}} + + cached = %{lister | candidates: %{{nil, "foo", foo.constraint} => {:ok, 1, v("1.0.0")}}} + assert {:ok, _lister, ^foo, version} = PackageLister.pick_package(cached, [foo]) + assert version == v("1.0.0") + + repo_foo = package_range("repo", "foo", ">= 0.0.0") + assert {:ok, _lister, ^repo_foo, version} = PackageLister.pick_package(lister, [repo_foo]) + assert version == v("2.0.0") + end + + test "returns the first package by name without versions after cached candidates" do + Registry.put("foo", "1.0.0", []) + Registry.prefetch([{nil, "foo"}, {nil, "bar"}, {nil, "baz"}]) + + lister = PackageLister.new(Registry, [], [], []) + foo = package_range("foo", "~> 1.0") + bar = package_range("bar", "~> 1.0") + baz = package_range("baz", "~> 1.0") + + assert {:ok, lister, ^foo, _version} = PackageLister.pick_package(lister, [foo]) + assert {:error, lister, ^bar} = PackageLister.pick_package(lister, [foo, baz, bar]) + assert {:error, _lister, ^baz} = PackageLister.pick_package(lister, [foo, baz]) + end + end + describe "dependencies_as_incompatibilities/4" do test "skips dependencies already returned for the version" do Registry.put("foo", "1.0.0", [{"bar", "~> 1.0"}]) @@ -23,6 +136,30 @@ defmodule HexSolver.PackageListerTest do PackageLister.dependencies_as_incompatibilities(lister, nil, "foo", v("1.0.0")) end + test "loads the dependencies of versions only as far as the constraint is the same" do + Registry.put("foo", "0.1.0", [{"bar", "~> 0.1"}]) + Registry.put("foo", "1.0.0", [{"bar", "~> 1.0"}]) + Registry.put("foo", "2.0.0", [{"bar", "~> 2.0"}]) + Registry.put("foo", "3.0.0", [{"bar", "~> 2.0"}]) + Registry.put("foo", "4.0.0", [{"bar", "~> 2.0"}]) + Registry.put("foo", "5.0.0", [{"bar", "~> 3.0"}]) + Registry.put("foo", "6.0.0", [{"bar", "~> 3.0"}]) + Registry.prefetch([{nil, "foo"}]) + + lister = PackageLister.new(CountingRegistry, [], [], []) + + assert {_lister, [incompatibility]} = + PackageLister.dependencies_as_incompatibilities(lister, nil, "foo", v("3.0.0")) + + assert to_string(incompatibility) == ~s("foo >= 2.0.0 and < 5.0.0" depends on "bar ~> 2.0") + + loaded = + for {{:dependencies, nil, "foo", version}, 1} <- CountingRegistry.calls(), + do: version + + assert Enum.sort(loaded) == ["1.0.0", "2.0.0", "3.0.0", "4.0.0", "5.0.0"] + end + test "returns dependencies again for a package with the same name in another repo" do Registry.put("foo", "1.0.0", [{"bar", "~> 1.0"}]) Registry.put("repo", "foo", "1.0.0", [{"bar", "~> 1.0"}]) @@ -38,4 +175,12 @@ defmodule HexSolver.PackageListerTest do PackageLister.dependencies_as_incompatibilities(lister, "repo", "foo", v("1.0.0")) end end + + defp package_range(repo \\ nil, name, requirement) do + %PackageRange{ + repo: repo, + name: name, + constraint: HexSolver.Requirement.to_constraint!(requirement) + } + end end