From d29a053fe9ee6f07b1d7d6f501f882fa307c9390 Mon Sep 17 00:00:00 2001 From: Samuel Giddins Date: Mon, 17 Jul 2017 15:08:56 -0500 Subject: [PATCH] Filter pre-releases in #requirement_satified_by? rather than when searching --- lib/molinillo/resolution.rb | 29 +++++++----------- spec/resolver_spec.rb | 6 ++-- spec/spec_helper/index.rb | 48 +++++++++++++++++------------- spec/spec_helper/naive_resolver.rb | 2 +- 4 files changed, 43 insertions(+), 42 deletions(-) diff --git a/lib/molinillo/resolution.rb b/lib/molinillo/resolution.rb index adb4f84..4663eaf 100644 --- a/lib/molinillo/resolution.rb +++ b/lib/molinillo/resolution.rb @@ -351,7 +351,15 @@ def attempt_to_activate debug(depth) { "Found existing spec (#{existing_vertex.payload})" } attempt_to_filter_existing_spec(existing_vertex) else - activate_new_spec + possibility.possibilities.select! do |possibility| + requirement_satisfied_by?(requirement, activated, possibility) + end + if possibility.latest_version.nil? + create_conflict + unwind_for_conflict + else + activate_new_spec + end end end @@ -360,8 +368,7 @@ def attempt_to_activate def attempt_to_filter_existing_spec(vertex) filtered_set = filtered_possibility_set(vertex) if !filtered_set.possibilities.empty? && - (vertex.payload.dependencies == dependencies_for(possibility) || - possibility_set_contains_prereleases(filtered_set)) + (vertex.payload.dependencies == dependencies_for(possibility)) activated.set_payload(name, filtered_set) new_requirements = requirements.dup push_state_for_requirements(new_requirements, false) @@ -388,21 +395,7 @@ def filtered_possibility_set(vertex) vertex.requirements.uniq.all? { |req| requirement_satisfied_by?(req, activated, poss) } end - # TODO: ideally we should ensure the possibilities set is unique here - - PossibilitySet.new(vertex.payload.dependencies, (filtered_old_values + filtered_new_values)) - end - - # Checks whether a `PossibilitySet` contains any pre-release versions. These - # need special treatment, as calling `search_for` on a previous, non-pre-release - # requirement won't have returned any pre-release versions - # @param [PossibilitySet] the possibility set to check - # @return [Boolean] whether or not the given `PossibilitySet` contains any - # pre-release version - def possibility_set_contains_prereleases(possibility_set) - # TODO: figure out a way to check for pre-release versions that doesn't - # call `.version` on user-provided possibilities objects - possibility_set.possibilities.any? { |poss| poss.version.prerelease? } + PossibilitySet.new(vertex.payload.dependencies, filtered_old_values | filtered_new_values) end # @param [String] requirement_name the spec name to search for diff --git a/spec/resolver_spec.rb b/spec/resolver_spec.rb index 7841dec..2034f76 100644 --- a/spec/resolver_spec.rb +++ b/spec/resolver_spec.rb @@ -46,12 +46,12 @@ def initialize(fixture_path) end def run(index_class, context) - return if ignore?(index_class) - test_case = self context.instance_eval do it test_case.name do + skip 'does not yet reliably pass' if test_case.ignore?(index_class) + resolve = lambda do index = index_class.new(test_case.index.specs) resolver = Resolver.new(index, TestUI.new) @@ -84,7 +84,7 @@ def run(index_class, context) end def ignore?(index_class) - if [BerkshelfIndex, ReverseBundlerIndex].include?(index_class) && + if [BerkshelfIndex, ReverseBundlerIndex, RandomSortIndex].include?(index_class) && name == 'can resolve when two specs have the same dependencies and swapping happens' # These indexes don't do a great job sorting, and segiddins has been diff --git a/spec/spec_helper/index.rb b/spec/spec_helper/index.rb index 1f6f8be..17f66a7 100644 --- a/spec/spec_helper/index.rb +++ b/spec/spec_helper/index.rb @@ -22,7 +22,12 @@ def initialize(specs_by_name) self.specs = specs_by_name end - def requirement_satisfied_by?(requirement, _activated, spec) + def requirement_satisfied_by?(requirement, activated, spec) + if spec.version.prerelease? && !requirement.prerelease? + vertex = activated.vertex_named(spec.name) + return false if vertex.requirements.none?(&:prerelease?) + end + case requirement when TestSpecification requirement.version == spec.version @@ -34,10 +39,8 @@ def requirement_satisfied_by?(requirement, _activated, spec) def search_for(dependency) @search_for ||= {} @search_for[dependency] ||= begin - prerelease = dependency_prerelease?(dependency) - Array(specs[dependency.name]).select do |spec| - (prerelease ? true : !spec.version.prerelease?) && - dependency.requirement.satisfied_by?(spec.version) + specs[dependency.name].select do |spec| + dependency.requirement.satisfied_by?(spec.version) end end @search_for[dependency].dup @@ -55,18 +58,12 @@ def sort_dependencies(dependencies, activated, conflicts) dependencies.sort_by do |d| [ activated.vertex_named(d.name).payload ? 0 : 1, - dependency_prerelease?(d) ? 0 : 1, + d.prerelease? ? 0 : 1, conflicts[d.name] ? 0 : 1, activated.vertex_named(d.name).payload ? 0 : search_for(d).count, ] end end - - private - - def dependency_prerelease?(dependency) - dependency.prerelease? - end end class BundlerIndex < TestIndex @@ -126,14 +123,14 @@ def sort_dependencies(dependencies, activated, conflicts) dependencies.sort_by do |d| [ activated.vertex_named(d.name).payload ? 0 : 1, - dependency_prerelease?(d) ? 0 : 1, + d.prerelease? ? 0 : 1, conflicts[d.name] ? 0 : 1, search_for(d).count, ] end end - def requirement_satisfied_by?(requirement, activated, spec) + def requirement_satisfied_by?(requirement, activated, spec) # rubocop:disable Metrics/CyclomaticComplexity requirement = case requirement when TestSpecification Gem::Dependency.new(requirement.name, requirement.version) @@ -142,10 +139,14 @@ def requirement_satisfied_by?(requirement, activated, spec) end shared_possibility_versions = - existing_possibility_versions_for_namespace(requirement, activated). - inject { |agg, set| agg & set } + existing_possibility_versions_for_namespace(requirement, activated) + + if spec.version.prerelease? && !requirement.prerelease? + vertex = activated.vertex_named(spec.name) + return false if vertex.requirements.none?(&:prerelease?) + end - if existing_possibility_versions_for_namespace(requirement, activated).any? + if !existing_possibility_versions_for_namespace(requirement, activated).empty? shared_possibility_versions.include?(spec.version) && requirement.requirement.satisfied_by?(spec.version) else @@ -156,15 +157,22 @@ def requirement_satisfied_by?(requirement, activated, spec) private def existing_possibility_versions_for_namespace(requirement, activated) - activated.vertices.values.map do |vertex| + prerelease_requirement = requirement.prerelease? + existing = activated.vertices.values.map do |vertex| next unless vertex.payload next unless vertex.name.split('/').first == requirement.name.split('/').first + + prerelease_requirement ||= vertex.requirements.any?(&:prerelease?) + if vertex.payload.respond_to?(:possibilities) vertex.payload.possibilities.map(&:version) else [vertex.payload.version] end - end.compact + end.compact.flatten(1) + # must use #select! instead of #reject! for 1.8.7 compatibility + existing.select! { |possibility| !possibility.prerelease? } unless prerelease_requirement + existing end end @@ -194,7 +202,7 @@ def versions_of(dependency_name) BundlerIndex, ReverseBundlerIndex, BundlerSingleAllNoPenaltyIndex, - # RandomSortIndex, this isn't yet always passing + RandomSortIndex, CocoaPodsIndex, BerkshelfIndex, ].freeze diff --git a/spec/spec_helper/naive_resolver.rb b/spec/spec_helper/naive_resolver.rb index cc87229..ff1676c 100644 --- a/spec/spec_helper/naive_resolver.rb +++ b/spec/spec_helper/naive_resolver.rb @@ -17,7 +17,7 @@ def self.resolve(index, dependencies) break unless vertex possibilities = possibilities_by_level[level] ||= index.search_for(Gem::Dependency.new(vertex.name, '>= 0.0.0-a')) possibilities.select! do |spec| - vertex.requirements.all? { |r| r.requirement.satisfied_by?(spec.version) && (!spec.version.prerelease? || index.send(:dependency_prerelease?, r)) } && + vertex.requirements.all? { |r| r.requirement.satisfied_by?(spec.version) && (!spec.version.prerelease? || r.prerelease?) } && spec.dependencies.all? { |d| v = activated.vertex_named(d.name); !v || !v.payload || d.satisfied_by?(v.payload.version) } end warn "level = #{level} possibilities = #{possibilities.map(&:to_s)} requirements = #{vertex.requirements.map(&:to_s)}"