Merge pull request #9477 from nicholasdower/nickd/race

Fix installing gems with native extensions + transitive dependencies

Author: Edouard-chin

bundler/lib/bundler/installer/parallel_installer.rb

@@ -6,7 +6,7 @@
 module Bundler
   class ParallelInstaller
     class SpecInstallation
-      attr_accessor :spec, :name, :full_name, :post_install_message, :state, :error
+      attr_accessor :spec, :name, :full_name, :post_install_message, :state, :error, :dependencies
       def initialize(spec)
         @spec = spec
         @name = spec.name
@@ -46,25 +46,11 @@ def has_post_install_message?
         !post_install_message.empty?
       end
 
-      def ignorable_dependency?(dep)
-        dep.type == :development || dep.name == @name
-      end
-
-      # Checks installed dependencies against spec's dependencies to make
-      # sure needed dependencies have been installed.
+      # Recursively checks that all dependencies (direct and transitive) have been installed.
       def dependencies_installed?(installed_specs)
-        dependencies.all? {|d| installed_specs.include? d.name }
-      end
-
-      # Represents only the non-development dependencies, the ones that are
-      # itself and are in the total list.
-      def dependencies
-        @dependencies ||= all_dependencies.reject {|dep| ignorable_dependency? dep }
-      end
-
-      # Represents all dependencies
-      def all_dependencies
-        @spec.dependencies
+        dependencies.all? do |dep|
+          installed_specs.include?(dep.name) && dep.dependencies_installed?(installed_specs)
+        end
       end
 
       def to_s
@@ -85,6 +71,12 @@ def initialize(installer, all_specs, size, standalone, force, local: false, skip
       @force = force
       @local = local
       @specs = all_specs.map {|s| SpecInstallation.new(s) }
+      specs_by_name = @specs.to_h {|s| [s.name, s] }
+      @specs.each do |spec_install|
+        spec_install.dependencies = spec_install.spec.dependencies.filter_map do |dep|
+          specs_by_name[dep.name] unless dep.type == :development || dep.name == spec_install.name
+        end
+      end
       @specs.each do |spec_install|
         spec_install.state = :installed if skip.include?(spec_install.name)
       end if skip

spec/bundler/installer/spec_installation_spec.rb

@@ -3,18 +3,17 @@
 require "bundler/installer/parallel_installer"
 
 RSpec.describe Bundler::ParallelInstaller::SpecInstallation do
-  let!(:dep) do
-    a_spec = Object.new
-    def a_spec.name
-      "I like tests"
-    end
-
-    def a_spec.full_name
-      "I really like tests"
-    end
-    a_spec
+  def build_spec(name, extensions: [])
+    spec = Object.new
+    spec.define_singleton_method(:name) { name }
+    spec.define_singleton_method(:full_name) { "#{name}-1.0" }
+    spec.define_singleton_method(:extensions) { extensions }
+    spec.define_singleton_method(:dependencies) { [] }
+    spec
   end
 
+  let!(:dep) { build_spec("I like tests") }
+
   describe "#ready_to_enqueue?" do
     context "when in enqueued state" do
       it "is falsey" do
@@ -39,29 +38,51 @@ def a_spec.full_name
   end
 
   describe "#dependencies_installed?" do
-    context "when all dependencies are installed" do
-      it "returns true" do
-        dependencies = []
-        dependencies << instance_double("SpecInstallation", spec: "alpha", name: "alpha", installed?: true, all_dependencies: [], type: :production)
-        dependencies << instance_double("SpecInstallation", spec: "beta", name: "beta", installed?: true, all_dependencies: [], type: :production)
-        all_specs = dependencies + [instance_double("SpecInstallation", spec: "gamma", name: "gamma", installed?: false, all_dependencies: [], type: :production)]
+    it "returns true when all dependencies are installed" do
+      alpha = described_class.new(build_spec("alpha"))
+      alpha.dependencies = []
+
+      beta = described_class.new(build_spec("beta"))
+      beta.dependencies = [alpha]
+
+      gamma = described_class.new(build_spec("gamma"))
+      gamma.dependencies = [beta]
+
+      expect(gamma.dependencies_installed?({})).to be_falsey
+      expect(gamma.dependencies_installed?({ "beta" => true })).to be_falsey
+      expect(gamma.dependencies_installed?({ "alpha" => true, "beta" => true })).to be_truthy
+    end
+  end
+
+  describe "#ready_to_install?" do
+    context "when spec has no extensions" do
+      it "returns true regardless of dependencies" do
+        beta = described_class.new(build_spec("beta"))
+        beta.dependencies = []
+
         spec = described_class.new(dep)
-        allow(spec).to receive(:all_dependencies).and_return(dependencies)
-        installed_specs = all_specs.select(&:installed?).map {|s| [s.name, true] }.to_h
-        expect(spec.dependencies_installed?(installed_specs)).to be_truthy
+        spec.state = :downloaded
+        spec.dependencies = [beta]
+
+        expect(spec.ready_to_install?({})).to be_truthy
       end
     end
 
-    context "when all dependencies are not installed" do
-      it "returns false" do
-        dependencies = []
-        dependencies << instance_double("SpecInstallation", spec: "alpha", name: "alpha", installed?: false, all_dependencies: [], type: :production)
-        dependencies << instance_double("SpecInstallation", spec: "beta", name: "beta", installed?: true, all_dependencies: [], type: :production)
-        all_specs = dependencies + [instance_double("SpecInstallation", spec: "gamma", name: "gamma", installed?: false, all_dependencies: [], type: :production)]
-        spec = described_class.new(dep)
-        allow(spec).to receive(:all_dependencies).and_return(dependencies)
-        installed_specs = all_specs.select(&:installed?).map {|s| [s.name, true] }.to_h
-        expect(spec.dependencies_installed?(installed_specs)).to be_falsey
+    context "when spec has extensions" do
+      it "returns true when all dependencies are installed" do
+        alpha = described_class.new(build_spec("alpha"))
+        alpha.dependencies = []
+
+        beta = described_class.new(build_spec("beta"))
+        beta.dependencies = [alpha]
+
+        gamma = described_class.new(build_spec("gamma", extensions: ["ext/Rakefile"]))
+        gamma.state = :downloaded
+        gamma.dependencies = [beta]
+
+        expect(gamma.ready_to_install?({})).to be_falsey
+        expect(gamma.ready_to_install?({ "beta" => true })).to be_falsey
+        expect(gamma.ready_to_install?({ "alpha" => true, "beta" => true })).to be_truthy
       end
     end
   end

spec/commands/install_spec.rb

@@ -1379,6 +1379,48 @@ def run
     end
   end
 
+  describe "when a native extension requires a transitive dependency at build time" do
+    before do
+      build_repo4 do
+        build_gem "alpha", "1.0.0" do |s|
+          extension = "ext/alpha/extconf.rb"
+          s.extensions = extension
+          s.write(extension, <<~CODE)
+            require "mkmf"
+            sleep 1
+            create_makefile("alpha")
+          CODE
+          s.write "lib/alpha.rb", "ALPHA = '1.0.0'"
+        end
+
+        build_gem "beta", "1.0.0" do |s|
+          s.add_dependency "alpha"
+          s.write "lib/beta.rb", "require 'alpha'\nBETA = '1.0.0'"
+        end
+
+        build_gem "gamma", "1.0.0" do |s|
+          s.add_dependency "beta"
+          extension = "ext/gamma/extconf.rb"
+          s.extensions = extension
+          s.write(extension, <<~EXTCONF)
+            require "beta"
+            require "mkmf"
+            create_makefile("gamma")
+          EXTCONF
+        end
+      end
+    end
+
+    it "installs successfully" do
+      install_gemfile <<~G
+        source "https://gem.repo4"
+        gem "gamma"
+      G
+
+      expect(the_bundle).to include_gems "alpha 1.0.0", "beta 1.0.0", "gamma 1.0.0"
+    end
+  end
+
   describe "when configured path is UTF-8 and a file inside a gem package too" do
     let(:app_path) do
       path = tmp("♥")