Merge pull request #7527 from rubygems/resolver-improvements

Fix resolver error message when it runs out of versions due to `--strict --patch` filtering out everything

(cherry picked from commit a384c33)

Author: deivid-rodriguez

bundler/lib/bundler/gem_version_promoter.rb

@@ -45,17 +45,37 @@ def level=(value)
 
     # Given a Resolver::Package and an Array of Specifications of available
     # versions for a gem, this method will return the Array of Specifications
-    # sorted (and possibly truncated if strict is true) in an order to give
-    # preference to the current level (:major, :minor or :patch) when resolution
-    # is deciding what versions best resolve all dependencies in the bundle.
+    # sorted in an order to give preference to the current level (:major, :minor
+    # or :patch) when resolution is deciding what versions best resolve all
+    # dependencies in the bundle.
     # @param package [Resolver::Package] The package being resolved.
     # @param specs [Specification] An array of Specifications for the package.
-    # @return [Specification] A new instance of the Specification Array sorted and
-    #    possibly filtered.
+    # @return [Specification] A new instance of the Specification Array sorted.
     def sort_versions(package, specs)
-      specs = filter_dep_specs(specs, package) if strict
+      locked_version = package.locked_version
 
-      sort_dep_specs(specs, package)
+      result = specs.sort do |a, b|
+        unless package.prerelease_specified? || pre?
+          a_pre = a.prerelease?
+          b_pre = b.prerelease?
+
+          next 1 if a_pre && !b_pre
+          next -1 if b_pre && !a_pre
+        end
+
+        if major? || locked_version.nil?
+          b <=> a
+        elsif either_version_older_than_locked?(a, b, locked_version)
+          b <=> a
+        elsif segments_do_not_match?(a, b, :major)
+          a <=> b
+        elsif !minor? && segments_do_not_match?(a, b, :minor)
+          a <=> b
+        else
+          b <=> a
+        end
+      end
+      post_sort(result, package.unlock?, locked_version)
     end
 
     # @return [bool] Convenience method for testing value of level variable.
@@ -73,9 +93,18 @@ def pre?
       pre == true
     end
 
-    private
+    # Given a Resolver::Package and an Array of Specifications of available
+    # versions for a gem, this method will truncate the Array if strict
+    # is true. That means filtering out downgrades from the version currently
+    # locked, and filtering out upgrades that go past the selected level (major,
+    # minor, or patch).
+    # @param package [Resolver::Package] The package being resolved.
+    # @param specs [Specification] An array of Specifications for the package.
+    # @return [Specification] A new instance of the Specification Array
+    #   truncated.
+    def filter_versions(package, specs)
+      return specs unless strict
 
-    def filter_dep_specs(specs, package)
       locked_version = package.locked_version
       return specs if locked_version.nil? || major?
 
@@ -89,32 +118,7 @@ def filter_dep_specs(specs, package)
       end
     end
 
-    def sort_dep_specs(specs, package)
-      locked_version = package.locked_version
-
-      result = specs.sort do |a, b|
-        unless package.prerelease_specified? || pre?
-          a_pre = a.prerelease?
-          b_pre = b.prerelease?
-
-          next -1 if a_pre && !b_pre
-          next  1 if b_pre && !a_pre
-        end
-
-        if major? || locked_version.nil?
-          a <=> b
-        elsif either_version_older_than_locked?(a, b, locked_version)
-          a <=> b
-        elsif segments_do_not_match?(a, b, :major)
-          b <=> a
-        elsif !minor? && segments_do_not_match?(a, b, :minor)
-          b <=> a
-        else
-          a <=> b
-        end
-      end
-      post_sort(result, package.unlock?, locked_version)
-    end
+    private
 
     def either_version_older_than_locked?(a, b, locked_version)
       a.version < locked_version || b.version < locked_version
@@ -133,13 +137,13 @@ def post_sort(result, unlock, locked_version)
       if unlock || locked_version.nil?
         result
       else
-        move_version_to_end(result, locked_version)
+        move_version_to_beginning(result, locked_version)
       end
     end
 
-    def move_version_to_end(result, version)
+    def move_version_to_beginning(result, version)
       move, keep = result.partition {|s| s.version.to_s == version.to_s }
-      keep.concat(move)
+      move.concat(keep)
     end
   end
 end

bundler/lib/bundler/resolver.rb

@@ -50,26 +50,26 @@ def setup_solver
         specs[name] = matches.sort_by {|s| [s.version, s.platform.to_s] }
       end
 
+      @all_versions = Hash.new do |candidates, package|
+        candidates[package] = all_versions_for(package)
+      end
+
       @sorted_versions = Hash.new do |candidates, package|
-        candidates[package] = if package.root?
-          [root_version]
-        else
-          all_versions_for(package).sort
-        end
+        candidates[package] = filtered_versions_for(package).sort
       end
 
+      @sorted_versions[root] = [root_version]
+
       root_dependencies = prepare_dependencies(@requirements, @packages)
 
       @cached_dependencies = Hash.new do |dependencies, package|
-        dependencies[package] = if package.root?
-          { root_version => root_dependencies }
-        else
-          Hash.new do |versions, version|
-            versions[version] = to_dependency_hash(version.dependencies.reject {|d| d.name == package.name }, @packages)
-          end
+        dependencies[package] = Hash.new do |versions, version|
+          versions[version] = to_dependency_hash(version.dependencies.reject {|d| d.name == package.name }, @packages)
         end
       end
 
+      @cached_dependencies[root] = { root_version => root_dependencies }
+
       logger = Bundler::UI::Shell.new
       logger.level = debug? ? "debug" : "warn"
 
@@ -156,9 +156,15 @@ def parse_dependency(package, dependency)
     end
 
     def versions_for(package, range=VersionRange.any)
-      versions = range.select_versions(@sorted_versions[package])
+      versions = select_sorted_versions(package, range)
 
-      sort_versions(package, versions)
+      # Conditional avoids (among other things) calling
+      # sort_versions_by_preferred with the root package
+      if versions.size > 1
+        sort_versions_by_preferred(package, versions)
+      else
+        versions
+      end
     end
 
     def no_versions_incompatibility_for(package, unsatisfied_term)
@@ -247,7 +253,7 @@ def all_versions_for(package)
       locked_requirement = base_requirements[name]
       results = filter_matching_specs(results, locked_requirement) if locked_requirement
 
-      versions = results.group_by(&:version).reduce([]) do |groups, (version, specs)|
+      results.group_by(&:version).reduce([]) do |groups, (version, specs)|
         platform_specs = package.platforms.map {|platform| select_best_platform_match(specs, platform) }
 
         # If package is a top-level dependency,
@@ -274,8 +280,6 @@ def all_versions_for(package)
 
         groups
       end
-
-      sort_versions(package, versions)
     end
 
     def source_for(name)
@@ -334,6 +338,21 @@ def raise_not_found!(package)
 
     private
 
+    def filtered_versions_for(package)
+      @gem_version_promoter.filter_versions(package, @all_versions[package])
+    end
+
+    def raise_all_versions_filtered_out!(package)
+      level = @gem_version_promoter.level
+      name = package.name
+      locked_version = package.locked_version
+      requirement = package.dependency
+
+      raise GemNotFound,
+        "#{name} is locked to #{locked_version}, while Gemfile is requesting #{requirement}. " \
+        "--strict --#{level} was specified, but there are no #{level} level upgrades from #{locked_version} satisfying #{requirement}, so version solving has failed"
+    end
+
     def filter_matching_specs(specs, requirements)
       Array(requirements).flat_map do |requirement|
         specs.select {| spec| requirement_satisfied_by?(requirement, spec) }
@@ -357,12 +376,8 @@ def requirement_satisfied_by?(requirement, spec)
       requirement.satisfied_by?(spec.version) || spec.source.is_a?(Source::Gemspec)
     end
 
-    def sort_versions(package, versions)
-      if versions.size > 1
-        @gem_version_promoter.sort_versions(package, versions).reverse
-      else
-        versions
-      end
+    def sort_versions_by_preferred(package, versions)
+      @gem_version_promoter.sort_versions(package, versions)
     end
 
     def repository_for(package)
@@ -379,12 +394,19 @@ def prepare_dependencies(requirements, packages)
 
         next [dep_package, dep_constraint] if name == "bundler"
 
-        versions = versions_for(dep_package, dep_constraint.range)
+        dep_range = dep_constraint.range
+        versions = select_sorted_versions(dep_package, dep_range)
         if versions.empty? && dep_package.ignores_prereleases?
+          @all_versions.delete(dep_package)
           @sorted_versions.delete(dep_package)
           dep_package.consider_prereleases!
-          versions = versions_for(dep_package, dep_constraint.range)
+          versions = select_sorted_versions(dep_package, dep_range)
         end
+
+        if versions.empty? && select_all_versions(dep_package, dep_range).any?
+          raise_all_versions_filtered_out!(dep_package)
+        end
+
         next [dep_package, dep_constraint] unless versions.empty?
 
         next unless dep_package.current_platform?
@@ -393,6 +415,14 @@ def prepare_dependencies(requirements, packages)
       end.compact.to_h
     end
 
+    def select_sorted_versions(package, range)
+      range.select_versions(@sorted_versions[package])
+    end
+
+    def select_all_versions(package, range)
+      range.select_versions(@all_versions[package])
+    end
+
     def other_specs_matching_message(specs, requirement)
       message = String.new("The source contains the following gems matching '#{requirement}':\n")
       message << specs.map {|s| "  * #{s.full_name}" }.join("\n")

bundler/lib/bundler/resolver/candidate.rb

@@ -15,7 +15,7 @@ class Resolver
     # considered separately.
     #
     # Some candidates may also keep some information explicitly about the
-    # package the refer to. These candidates are referred to as "canonical" and
+    # package they refer to. These candidates are referred to as "canonical" and
     # are used when materializing resolution results back into RubyGems
     # specifications that can be installed, written to lock files, and so on.
     #

bundler/spec/bundler/gem_version_promoter_spec.rb

@@ -33,13 +33,13 @@ def sorted_versions(candidates:, current:, name: "foo", locked: [])
 
     it "numerically sorts versions" do
       versions = sorted_versions(candidates: %w[1.7.7 1.7.8 1.7.9 1.7.15 1.8.0], current: "1.7.8")
-      expect(versions).to eq %w[1.7.7 1.7.8 1.7.9 1.7.15 1.8.0]
+      expect(versions).to eq %w[1.8.0 1.7.15 1.7.9 1.7.8 1.7.7]
     end
 
     context "with no options" do
       it "defaults to level=:major, strict=false, pre=false" do
         versions = sorted_versions(candidates: %w[0.2.0 0.3.0 0.3.1 0.9.0 1.0.0 2.0.1 2.1.0], current: "0.3.0")
-        expect(versions).to eq %w[0.2.0 0.3.0 0.3.1 0.9.0 1.0.0 2.0.1 2.1.0]
+        expect(versions).to eq %w[2.1.0 2.0.1 1.0.0 0.9.0 0.3.1 0.3.0 0.2.0]
       end
     end
 
@@ -51,25 +51,25 @@ def sorted_versions(candidates:, current:, name: "foo", locked: [])
 
         it "keeps downgrades" do
           versions = sorted_versions(candidates: %w[0.2.0 0.3.0 0.3.1 0.9.0 1.0.0 2.0.1 2.1.0], current: "0.3.0")
-          expect(versions).to eq %w[0.2.0 0.3.0 0.3.1 0.9.0 1.0.0 2.0.1 2.1.0]
+          expect(versions).to eq %w[2.1.0 2.0.1 1.0.0 0.9.0 0.3.1 0.3.0 0.2.0]
         end
       end
 
       context "when level is minor" do
         before { gvp.level = :minor }
 
-        it "removes downgrades and major upgrades" do
+        it "sorts highest minor within same major in first position" do
           versions = sorted_versions(candidates: %w[0.2.0 0.3.0 0.3.1 0.9.0 1.0.0 2.0.1 2.1.0], current: "0.3.0")
-          expect(versions).to eq %w[0.3.0 0.3.1 0.9.0]
+          expect(versions).to eq %w[0.9.0 0.3.1 0.3.0 1.0.0 2.1.0 2.0.1 0.2.0]
         end
       end
 
       context "when level is patch" do
         before { gvp.level = :patch }
 
-        it "removes downgrades and major and minor upgrades" do
+        it "sorts highest patch within same minor in first position" do
           versions = sorted_versions(candidates: %w[0.2.0 0.3.0 0.3.1 0.9.0 1.0.0 2.0.1 2.1.0], current: "0.3.0")
-          expect(versions).to eq %w[0.3.0 0.3.1]
+          expect(versions).to eq %w[0.3.1 0.3.0 0.9.0 1.0.0 2.0.1 2.1.0 0.2.0]
         end
       end
     end
@@ -82,25 +82,25 @@ def sorted_versions(candidates:, current:, name: "foo", locked: [])
 
         it "orders by version" do
           versions = sorted_versions(candidates: %w[0.2.0 0.3.0 0.3.1 0.9.0 1.0.0 2.0.1 2.1.0], current: "0.3.0")
-          expect(versions).to eq %w[0.2.0 0.3.0 0.3.1 0.9.0 1.0.0 2.0.1 2.1.0]
+          expect(versions).to eq %w[2.1.0 2.0.1 1.0.0 0.9.0 0.3.1 0.3.0 0.2.0]
         end
       end
 
       context "when level is minor" do
         before { gvp.level = :minor }
 
-        it "favors downgrades, then upgrades by major descending, minor ascending, patch ascending" do
+        it "favors minor upgrades, then patch upgrades, then major upgrades, then downgrades" do
           versions = sorted_versions(candidates: %w[0.2.0 0.3.0 0.3.1 0.9.0 1.0.0 2.0.1 2.1.0], current: "0.3.0")
-          expect(versions).to eq %w[0.2.0 2.0.1 2.1.0 1.0.0 0.3.0 0.3.1 0.9.0]
+          expect(versions).to eq %w[0.9.0 0.3.1 0.3.0 1.0.0 2.1.0 2.0.1 0.2.0]
         end
       end
 
       context "when level is patch" do
         before { gvp.level = :patch }
 
-        it "favors downgrades, then upgrades by major descending, minor descending, patch ascending" do
+        it "favors patch upgrades, then minor upgrades, then major upgrades, then downgrades" do
           versions = sorted_versions(candidates: %w[0.2.0 0.3.0 0.3.1 0.9.0 1.0.0 2.0.1 2.1.0], current: "0.3.0")
-          expect(versions).to eq %w[0.2.0 2.1.0 2.0.1 1.0.0 0.9.0 0.3.0 0.3.1]
+          expect(versions).to eq %w[0.3.1 0.3.0 0.9.0 1.0.0 2.0.1 2.1.0 0.2.0]
         end
       end
     end
@@ -110,7 +110,7 @@ def sorted_versions(candidates:, current:, name: "foo", locked: [])
 
       it "sorts regardless of prerelease status" do
         versions = sorted_versions(candidates: %w[1.7.7.pre 1.8.0 1.8.1.pre 1.8.1 2.0.0.pre 2.0.0], current: "1.8.0")
-        expect(versions).to eq %w[1.7.7.pre 1.8.0 1.8.1.pre 1.8.1 2.0.0.pre 2.0.0]
+        expect(versions).to eq %w[2.0.0 2.0.0.pre 1.8.1 1.8.1.pre 1.8.0 1.7.7.pre]
       end
     end
 
@@ -119,16 +119,16 @@ def sorted_versions(candidates:, current:, name: "foo", locked: [])
 
       it "deprioritizes prerelease gems" do
         versions = sorted_versions(candidates: %w[1.7.7.pre 1.8.0 1.8.1.pre 1.8.1 2.0.0.pre 2.0.0], current: "1.8.0")
-        expect(versions).to eq %w[1.7.7.pre 1.8.1.pre 2.0.0.pre 1.8.0 1.8.1 2.0.0]
+        expect(versions).to eq %w[2.0.0 1.8.1 1.8.0 2.0.0.pre 1.8.1.pre 1.7.7.pre]
       end
     end
 
     context "when locking and not major" do
       before { gvp.level = :minor }
 
-      it "keeps the current version last" do
+      it "keeps the current version first" do
         versions = sorted_versions(candidates: %w[0.2.0 0.3.0 0.3.1 0.9.0 1.0.0 2.1.0 2.0.1], current: "0.3.0", locked: ["bar"])
-        expect(versions.last).to eq("0.3.0")
+        expect(versions.first).to eq("0.3.0")
       end
     end
   end