Merge #7522

7522: Improve platform specific gem resolution r=deivid-rodriguez a=kou

### What was the end-user problem that led to this PR?

Platform specific gems aren't resolved when platform specific gems are conflicted.

For example, in the following situation, foo-1.0.0-x64-mingw32 that requires bar<1 is conflicted because there is no bar<1. Without this change, Bundler raises a conflict error. But users can use foo-1.0.0 (no x64-mingw32) in this situation. With this change, Bundler resolves to foo-1.0.0 (no x64-mingw32).

```ruby
@index = build_index do
  gem "bar", "1.0.0"
  gem "foo", "1.0.0"
  gem "foo", "1.0.0", "x64-mingw32" do
    dep "bar", "< 1"
  end
end
dep "foo"
platforms "x64-mingw32"
````

See also #6247. This change includes the specs that were added in #6247.

### What was your diagnosis of the problem?

Not platform specific gem (foo-1.0.0 in the above case) isn't tried to be resolved when platform specific gem (foo-1.0.0-x64-mingw32 in the above case) is available.

### What is your fix for the problem, implemented in this PR?

In this PR, not platform specific gem (foo-1.0.0 in the above case) is also tried to be resolved even when platform specific gem (foo-1.0.0-x64-mingw32 in the above case) is available. Priority is "platform specific gem" -> "not platform specific gem". So platform specific gem is usable, platform specific gem is used. Not platform specific gem is used as fallback.

`search_for` represents this. Here is the `search_for` specification:
https://github.com/bundler/bundler/blob/master/lib/bundler/vendor/molinillo/lib/molinillo/modules/specification_provider.rb#L11-L13

> Search for the specifications that match the given dependency. The specifications in the returned array will be considered in reverse order, so the latest version ought to be last.

## Why did you choose this fix out of the possible options?

I choose this fix because this is based on Molinillo's specification.

Co-authored-by: Lars Kanis <lars@greiz-reinsdorf.de>
Co-authored-by: Samuel Giddins <segiddins@segiddins.me>
Co-authored-by: Sutou Kouhei <kou@clear-code.com>

12 files changed, 183 insertions, 73 deletions

diff --git a/lib/bundler/definition.rb b/lib/bundler/definition.rb
index d6fbb0b5b7..3e5186039a 100644
--- a/lib/bundler/definition.rb
+++ b/lib/bundler/definition.rb
@@ -82,7 +82,11 @@ module Bundler
         @lockfile_contents = Bundler.read_file(lockfile)
         @locked_gems = LockfileParser.new(@lockfile_contents)
         @locked_platforms = @locked_gems.platforms
-        @platforms = @locked_platforms.dup
+        if Bundler.settings[:force_ruby_platform]
+          @platforms = [Gem::Platform::RUBY]
+        else
+          @platforms = @locked_platforms.dup
+        end
         @locked_bundler_version = @locked_gems.bundler_version
         @locked_ruby_version = @locked_gems.ruby_version
 
@@ -228,12 +232,13 @@ module Bundler
     end
 
     def current_dependencies
-      dependencies.select(&:should_include?)
+      dependencies.select do |d|
+        d.should_include? && !d.gem_platforms(@platforms).empty?
+      end
     end
 
     def specs_for(groups)
-      deps = dependencies.select {|d| (d.groups & groups).any? }
-      deps.delete_if {|d| !d.should_include? }
+      deps = dependencies_for(groups)
       specs.for(expand_dependencies(deps))
     end
 
@@ -706,9 +711,6 @@ module Bundler
         elsif dep.source
           dep.source = sources.get(dep.source)
         end
-        if dep.source.is_a?(Source::Gemspec)
-          dep.platforms.concat(@platforms.map {|p| Dependency::REVERSE_PLATFORM_MAP[p] }.flatten(1)).uniq!
-        end
       end
 
       changes = false
@@ -907,10 +909,16 @@ module Bundler
       deps
     end
 
+    def dependencies_for(groups)
+      current_dependencies.reject do |d|
+        (d.groups & groups).empty?
+      end
+    end
+
     def requested_dependencies
       groups = requested_groups
       groups.map!(&:to_sym)
-      dependencies.reject {|d| !d.should_include? || (d.groups & groups).empty? }
+      dependencies_for(groups)
     end
 
     def source_requirements
diff --git a/lib/bundler/dependency.rb b/lib/bundler/dependency.rb
index 6c2642163e..26e5f3d1a5 100644
--- a/lib/bundler/dependency.rb
+++ b/lib/bundler/dependency.rb
@@ -74,15 +74,6 @@ module Bundler
       :x64_mingw_26 => Gem::Platform::X64_MINGW,
     }.freeze
 
-    REVERSE_PLATFORM_MAP = {}.tap do |reverse_platform_map|
-      PLATFORM_MAP.each do |key, value|
-        reverse_platform_map[value] ||= []
-        reverse_platform_map[value] << key
-      end
-
-      reverse_platform_map.each {|_, platforms| platforms.freeze }
-    end.freeze
-
     def initialize(name, version, options = {}, &blk)
       type = options["type"] || :runtime
       super(name, version, type)
diff --git a/lib/bundler/dsl.rb b/lib/bundler/dsl.rb
index 99a369281a..bb92a28381 100644
--- a/lib/bundler/dsl.rb
+++ b/lib/bundler/dsl.rb
@@ -75,8 +75,7 @@ module Bundler
 
         @gemspecs << spec
 
-        gem_platforms = Bundler::Dependency::REVERSE_PLATFORM_MAP[Bundler::GemHelpers.generic_local_platform]
-        gem spec.name, :name => spec.name, :path => path, :glob => glob, :platforms => gem_platforms
+        gem spec.name, :name => spec.name, :path => path, :glob => glob
 
         group(development_group) do
           spec.development_dependencies.each do |dep|
diff --git a/lib/bundler/lazy_specification.rb b/lib/bundler/lazy_specification.rb
index 32c8bb9557..7a6f5614ef 100644
--- a/lib/bundler/lazy_specification.rb
+++ b/lib/bundler/lazy_specification.rb
@@ -46,6 +46,14 @@ module Bundler
       identifier == other.identifier
     end
 
+    def eql?(other)
+      identifier.eql?(other.identifier)
+    end
+
+    def hash
+      identifier.hash
+    end
+
     def satisfies?(dependency)
       @name == dependency.name && dependency.requirement.satisfied_by?(Gem::Version.new(@version))
     end
diff --git a/lib/bundler/resolver.rb b/lib/bundler/resolver.rb
index c7caf01c7d..2374ed3e5a 100644
--- a/lib/bundler/resolver.rb
+++ b/lib/bundler/resolver.rb
@@ -146,7 +146,26 @@ module Bundler
           @gem_version_promoter.sort_versions(dependency, spec_groups)
         end
       end
-      search.select {|sg| sg.for?(platform) }.each {|sg| sg.activate_platform!(platform) }
+      selected_sgs = []
+      search.each do |sg|
+        next unless sg.for?(platform)
+        # Add a spec group for "non platform specific spec" as the fallback
+        # spec group.
+        sg_ruby = sg.copy_for(Gem::Platform::RUBY)
+        selected_sgs << sg_ruby if sg_ruby
+        sg_all_platforms = nil
+        all_platforms = @platforms + [platform]
+        sorted_all_platforms = self.class.sort_platforms(all_platforms)
+        sorted_all_platforms.reverse_each do |other_platform|
+          if sg_all_platforms.nil?
+            sg_all_platforms = sg.copy_for(other_platform)
+          else
+            sg_all_platforms.activate_platform!(other_platform)
+          end
+        end
+        selected_sgs << sg_all_platforms
+      end
+      selected_sgs
     end
 
     def index_for(dependency)
@@ -183,9 +202,7 @@ module Bundler
     end
 
     def requirement_satisfied_by?(requirement, activated, spec)
-      return false unless requirement.matches_spec?(spec) || spec.source.is_a?(Source::Gemspec)
-      spec.activate_platform!(requirement.__platform) if !@platforms || @platforms.include?(requirement.__platform)
-      true
+      requirement.matches_spec?(spec) || spec.source.is_a?(Source::Gemspec)
     end
 
     def relevant_sources_for_vertex(vertex)
@@ -223,8 +240,9 @@ module Bundler
     end
 
     def self.platform_sort_key(platform)
-      return ["", "", ""] if Gem::Platform::RUBY == platform
-      platform.to_a.map {|part| part || "" }
+      # Prefer specific platform to not specific platform
+      return ["99-LAST", "", "", ""] if Gem::Platform::RUBY == platform
+      ["00", *platform.to_a.map {|part| part || "" }]
     end
 
   private
diff --git a/lib/bundler/resolver/spec_group.rb b/lib/bundler/resolver/spec_group.rb
index e5772eed81..d5d12f7a2d 100644
--- a/lib/bundler/resolver/spec_group.rb
+++ b/lib/bundler/resolver/spec_group.rb
@@ -9,6 +9,7 @@ module Bundler
       attr_accessor :ignores_bundler_dependencies
 
       def initialize(all_specs)
+        @all_specs = all_specs
         raise ArgumentError, "cannot initialize with an empty value" unless exemplary_spec = all_specs.first
         @name = exemplary_spec.name
         @version = exemplary_spec.version
@@ -28,7 +29,7 @@ module Bundler
           lazy_spec = LazySpecification.new(name, version, s.platform, source)
           lazy_spec.dependencies.replace s.dependencies
           lazy_spec
-        end.compact
+        end.compact.uniq
       end
 
       def activate_platform!(platform)
@@ -37,13 +38,25 @@ module Bundler
         @activated_platforms << platform
       end
 
+      def copy_for(platform)
+        copied_sg = self.class.new(@all_specs)
+        copied_sg.ignores_bundler_dependencies = @ignores_bundler_dependencies
+        return nil unless copied_sg.for?(platform)
+        copied_sg.activate_platform!(platform)
+        copied_sg
+      end
+
+      def spec_for(platform)
+        @specs[platform]
+      end
+
       def for?(platform)
-        spec = @specs[platform]
-        !spec.nil?
+        !spec_for(platform).nil?
       end
 
       def to_s
-        @to_s ||= "#{name} (#{version})"
+        activated_platforms_string = sorted_activated_platforms.join(", ")
+        "#{name} (#{version}) (#{activated_platforms_string})"
       end
 
       def dependencies_for_activated_platforms
@@ -58,6 +71,7 @@ module Bundler
         return unless other.is_a?(SpecGroup)
         name == other.name &&
           version == other.version &&
+          sorted_activated_platforms == other.sorted_activated_platforms &&
           source == other.source
       end
 
@@ -65,11 +79,18 @@ module Bundler
         return unless other.is_a?(SpecGroup)
         name.eql?(other.name) &&
           version.eql?(other.version) &&
+          sorted_activated_platforms.eql?(other.sorted_activated_platforms) &&
           source.eql?(other.source)
       end
 
       def hash
-        to_s.hash ^ source.hash
+        name.hash ^ version.hash ^ sorted_activated_platforms.hash ^ source.hash
+      end
+
+    protected
+
+      def sorted_activated_platforms
+        @activated_platforms.sort_by(&:to_s)
       end
 
     private
diff --git a/spec/bundler/dsl_spec.rb b/spec/bundler/dsl_spec.rb
index 319472d4b0..9299c014af 100644
--- a/spec/bundler/dsl_spec.rb
+++ b/spec/bundler/dsl_spec.rb
@@ -174,41 +174,6 @@ RSpec.describe Bundler::Dsl do
     end
   end
 
-  describe "#gemspec" do
-    let(:spec) do
-      Gem::Specification.new do |gem|
-        gem.name = "example"
-        gem.platform = platform
-      end
-    end
-
-    before do
-      allow(Dir).to receive(:[]).and_return(["spec_path"])
-      allow(Bundler).to receive(:load_gemspec).with("spec_path").and_return(spec)
-      allow(Bundler).to receive(:default_gemfile).and_return(Pathname.new("./Gemfile"))
-    end
-
-    context "with a ruby platform" do
-      let(:platform) { "ruby" }
-
-      it "keeps track of the ruby platforms in the dependency" do
-        allow(Gem::Platform).to receive(:local).and_return(rb)
-        subject.gemspec
-        expect(subject.dependencies.last.platforms).to eq(Bundler::Dependency::REVERSE_PLATFORM_MAP[Gem::Platform::RUBY])
-      end
-    end
-
-    context "with a jruby platform" do
-      let(:platform) { "java" }
-
-      it "keeps track of the jruby platforms in the dependency" do
-        allow(Gem::Platform).to receive(:local).and_return(java)
-        subject.gemspec
-        expect(subject.dependencies.last.platforms).to eq(Bundler::Dependency::REVERSE_PLATFORM_MAP[Gem::Platform::JAVA])
-      end
-    end
-  end
-
   context "can bundle groups of gems with" do
     # git "https://github.com/rails/rails.git" do
     #   gem "railties"
diff --git a/spec/install/gemfile/platform_spec.rb b/spec/install/gemfile/platform_spec.rb
index eab5e7f00c..52e1cf86fa 100644
--- a/spec/install/gemfile/platform_spec.rb
+++ b/spec/install/gemfile/platform_spec.rb
@@ -379,8 +379,6 @@ RSpec.describe "bundle install with platform conditionals" do
   end
 
   it "prints a helpful warning when a dependency is unused on any platform" do
-    skip "prints warning but bundle install fails" if Gem.win_platform?
-
     simulate_platform "ruby"
     simulate_ruby_engine "ruby"
 
@@ -401,8 +399,6 @@ The dependency #{Gem::Dependency.new("rack", ">= 0")} will be unused by any of t
     before { bundle! "config set disable_platform_warnings true" }
 
     it "does not print the warning when a dependency is unused on any platform" do
-      skip "skips warning but bundle install fails" if Gem.win_platform?
-
       simulate_platform "ruby"
       simulate_ruby_engine "ruby"
 
diff --git a/spec/install/gems/resolving_spec.rb b/spec/install/gems/resolving_spec.rb
index 523f9eb151..547d13134f 100644
--- a/spec/install/gems/resolving_spec.rb
+++ b/spec/install/gems/resolving_spec.rb
@@ -156,6 +156,13 @@ RSpec.describe "bundle install with install-time dependencies" do
 
       let(:ruby_requirement) { %("#{RUBY_VERSION}") }
       let(:error_message_requirement) { "~> #{RUBY_VERSION}.0" }
+      let(:error_message_platform) do
+        if Bundler.feature_flag.specific_platform?
+          " #{Bundler.local_platform}"
+        else
+          ""
+        end
+      end
 
       shared_examples_for "ruby version conflicts" do
         it "raises an error during resolution" do
@@ -172,9 +179,9 @@ RSpec.describe "bundle install with install-time dependencies" do
           nice_error = strip_whitespace(<<-E).strip
             Bundler found conflicting requirements for the Ruby\0 version:
               In Gemfile:
-                Ruby\0 (#{error_message_requirement})
+                Ruby\0 (#{error_message_requirement})#{error_message_platform}
 
-                require_ruby was resolved to 1.0, which depends on
+                require_ruby#{error_message_platform} was resolved to 1.0, which depends on
                   Ruby\0 (> 9000)
 
             Ruby\0 (> 9000), which is required by gem 'require_ruby', is not available in the local ruby installation
diff --git a/spec/resolver/platform_spec.rb b/spec/resolver/platform_spec.rb
index fee0cf1f1c..415c5458df 100644
--- a/spec/resolver/platform_spec.rb
+++ b/spec/resolver/platform_spec.rb
@@ -28,6 +28,98 @@ RSpec.describe "Resolving platform craziness" do
     end
   end
 
+  it "takes the latest ruby gem, even if an older platform specific version is available" do
+    @index = build_index do
+      gem "foo", "1.0.0"
+      gem "foo", "1.0.0", "x64-mingw32"
+      gem "foo", "1.1.0"
+    end
+    dep "foo"
+    platforms "x64-mingw32"
+
+    should_resolve_as %w[foo-1.1.0]
+  end
+
+  it "takes the ruby version if the platform version is incompatible" do
+    @index = build_index do
+      gem "bar", "1.0.0"
+      gem "foo", "1.0.0"
+      gem "foo", "1.0.0", "x64-mingw32" do
+        dep "bar", "< 1"
+      end
+    end
+    dep "foo"
+    platforms "x64-mingw32"
+
+    should_resolve_as %w[foo-1.0.0]
+  end
+
+  it "prefers the platform specific gem to the ruby version" do
+    @index = build_index do
+      gem "foo", "1.0.0"
+      gem "foo", "1.0.0", "x64-mingw32"
+    end
+    dep "foo"
+    platforms "x64-mingw32"
+
+    should_resolve_as %w[foo-1.0.0-x64-mingw32]
+  end
+
+  it "takes the latest ruby gem if the platform specific gem doesn't match the required_ruby_version" do
+    @index = build_index do
+      gem "foo", "1.0.0"
+      gem "foo", "1.0.0", "x64-mingw32"
+      gem "foo", "1.1.0"
+      gem "foo", "1.1.0", "x64-mingw32" do |s|
+        s.required_ruby_version = [">= 2.0", "< 2.4"]
+      end
+      gem "Ruby\0", "2.5.1"
+    end
+    dep "foo"
+    dep "Ruby\0", "2.5.1"
+    platforms "x64-mingw32"
+
+    should_resolve_as %w[foo-1.1.0]
+  end
+
+  it "takes the latest ruby gem with required_ruby_version if the platform specific gem doesn't match the required_ruby_version" do
+    @index = build_index do
+      gem "foo", "1.0.0"
+      gem "foo", "1.0.0", "x64-mingw32"
+      gem "foo", "1.1.0" do |s|
+        s.required_ruby_version = [">= 2.0"]
+      end
+      gem "foo", "1.1.0", "x64-mingw32" do |s|
+        s.required_ruby_version = [">= 2.0", "< 2.4"]
+      end
+      gem "Ruby\0", "2.5.1"
+    end
+    dep "foo"
+    dep "Ruby\0", "2.5.1"
+    platforms "x64-mingw32"
+
+    should_resolve_as %w[foo-1.1.0]
+  end
+
+  it "takes the latest ruby gem if the platform specific gem doesn't match the required_ruby_version with multiple platforms" do
+    @index = build_index do
+      gem "foo", "1.0.0"
+      gem "foo", "1.0.0", "x64-mingw32"
+      gem "foo", "1.1.0" do |s|
+        s.required_ruby_version = [">= 2.0"]
+      end
+      gem "foo", "1.1.0", "x64-mingw32" do |s|
+        s.required_ruby_version = [">= 2.0", "< 2.4"]
+      end
+      gem "Ruby\0", "2.5.1"
+    end
+    dep "foo"
+    dep "Ruby\0", "2.5.1"
+    platforms "x86_64-linux", "x64-mingw32"
+
+    should_resolve_as %w[foo-1.1.0]
+  end
+
   describe "with mingw32" do
     before :each do
       @index = build_index do
@@ -90,11 +182,11 @@ RSpec.describe "Resolving platform craziness" do
       end
     end
 
-    it "reports on the conflict" do
+    it "takes the ruby version as fallback" do
       platforms "ruby", "java"
       dep "foo"
 
-      should_conflict_on "baz"
+      should_resolve_as %w[bar-1.0.0 baz-1.0.0 foo-1.0.0]
     end
   end
 end
diff --git a/spec/runtime/platform_spec.rb b/spec/runtime/platform_spec.rb
index ad9c56d14e..70c7594395 100644
--- a/spec/runtime/platform_spec.rb
+++ b/spec/runtime/platform_spec.rb
@@ -113,6 +113,7 @@ RSpec.describe "Bundler.setup with multi platform stuff" do
       bundle! "install"
 
       expect(the_bundle).to include_gems "platform_specific 1.0 RUBY"
+      expect(the_bundle).to not_include_gems "nokogiri"
     end
   end
 
diff --git a/spec/support/indexes.rb b/spec/support/indexes.rb
index dc6e0bd1e9..7440523fc9 100644
--- a/spec/support/indexes.rb
+++ b/spec/support/indexes.rb
@@ -26,6 +26,10 @@ module Spec
         end
       end
       source_requirements ||= {}
+      args[0] ||= [] # base
+      args[1] ||= Bundler::GemVersionPromoter.new # gem_version_promoter
+      args[2] ||= [] # additional_base_requirements
+      args[3] ||= @platforms # platforms
       Bundler::Resolver.resolve(deps, @index, source_requirements, *args)
     end