Auto merge of #4063 - A5308Y:master, r=segiddins

Extracting a new "GemInstaller" from installer.rb

My goal was to reveal the main part of install_gem_from_spec. From my
perspective the main part is:
1. Install
2. Generate stubs
3. Print message to the user
(4. handle exceptions)

While working on it I found that Bundler::Installer's run method and
instance variables weren't used when installing gems with
install_gem_from_spec. The installer was (and still is) only used to
generate executable stubs.
So I inserted the new GemInstaller in ParallelInstaller; passing the
instantiated Bundler::Installer to the GemInstaller only to generate
the executable stubs.

Based on this I would continue by extracting two BinstubGenerator
classes and removing GemInstaller's dependency on Bundler::Installer,
possibly allowing for a more concise way to generate binstubs in other
parts of bundler.

I was a bit intimidated by the amount of installers and weary to add
another one to this contested namespace. So I checked in with
@indirect who approved of the general direction of the refactoring and
suggested to rename the old GemInstaller to RubyGemsGemInstaller.

8 files changed, 102 insertions, 67 deletions

diff --git a/lib/bundler.rb b/lib/bundler.rb
index bf3bbb1929..0eea855fe3 100644
--- a/lib/bundler.rb
+++ b/lib/bundler.rb
@@ -27,7 +27,7 @@ module Bundler
   autoload :Fetcher,               "bundler/fetcher"
   autoload :GemHelper,             "bundler/gem_helper"
   autoload :GemHelpers,            "bundler/gem_helpers"
-  autoload :GemInstaller,          "bundler/gem_installer"
+  autoload :RubyGemsGemInstaller,  "bundler/rubygems_gem_installer"
   autoload :Graph,                 "bundler/graph"
   autoload :Index,                 "bundler/index"
   autoload :Installer,             "bundler/installer"
diff --git a/lib/bundler/gem_remote_fetcher.rb b/lib/bundler/gem_remote_fetcher.rb
index 77cab0c4ad..8a823da963 100644
--- a/lib/bundler/gem_remote_fetcher.rb
+++ b/lib/bundler/gem_remote_fetcher.rb
@@ -4,7 +4,7 @@ module Bundler
   # Adds support for setting custom HTTP headers when fetching gems from the
   # server.
   #
-  # TODO Get rid of this when and if gemstash only supports RubyGems versions
+  # TODO: Get rid of this when and if gemstash only supports RubyGems versions
   # that contain https://github.com/rubygems/rubygems/commit/3db265cc20b2f813.
   class GemRemoteFetcher < Gem::RemoteFetcher
     attr_accessor :headers
diff --git a/lib/bundler/installer.rb b/lib/bundler/installer.rb
index 1fd0b85dd9..8257458476 100644
--- a/lib/bundler/installer.rb
+++ b/lib/bundler/installer.rb
@@ -3,6 +3,7 @@ require "rubygems/dependency_installer"
 require "bundler/worker"
 require "bundler/installer/parallel_installer"
 require "bundler/installer/standalone"
+require "bundler/installer/gem_installer"
 
 module Bundler
   class Installer < Environment
@@ -72,51 +73,6 @@ module Bundler
       Standalone.new(options[:standalone], @definition).generate if options[:standalone]
     end
 
-    def install_gem_from_spec(spec, standalone = false, worker = 0, force = false)
-      # Fetch the build settings, if there are any
-      settings = Bundler.settings["build.#{spec.name}"]
-      install_options = { :force => force, :ensure_builtin_gems_cached => standalone }
-
-      post_install_message = nil
-      if settings
-        # Build arguments are global, so this is mutexed
-        Bundler.rubygems.with_build_args [settings] do
-          post_install_message = spec.source.install(spec, install_options)
-        end
-      else
-        post_install_message = spec.source.install(spec, install_options)
-      end
-
-      Bundler.ui.debug "#{worker}:  #{spec.name} (#{spec.version}) from #{spec.loaded_from}"
-
-      if Bundler.settings[:bin] && standalone
-        generate_standalone_bundler_executable_stubs(spec)
-      elsif Bundler.settings[:bin]
-        generate_bundler_executable_stubs(spec, :force => true)
-      end
-
-      post_install_message
-    rescue Errno::ENOSPC
-      raise Bundler::InstallError, "Your disk is out of space. Free some " \
-        "space to be able to install your bundle."
-    rescue Exception => e
-      # if install hook failed or gem signature is bad, just die
-      raise e if e.is_a?(Bundler::InstallHookError) || e.is_a?(Bundler::SecurityError)
-
-      # other failure, likely a native extension build failure
-      Bundler.ui.info ""
-      Bundler.ui.warn "#{e.class}: #{e.message}"
-      msg = "An error occurred while installing #{spec.name} (#{spec.version}),"
-      msg << " and Bundler cannot continue."
-
-      unless spec.source.options["git"]
-        msg << "\nMake sure that `gem install"
-        msg << " #{spec.name} -v '#{spec.version}'` succeeds before bundling."
-      end
-      Bundler.ui.debug e.backtrace.join("\n")
-      raise Bundler::InstallError, msg
-    end
-
     def generate_bundler_executable_stubs(spec, options = {})
       if options[:binstubs_cmd] && spec.executables.empty?
         options = {}
@@ -170,6 +126,22 @@ module Bundler
       end
     end
 
+    def generate_standalone_bundler_executable_stubs(spec)
+      # double-assignment to avoid warnings about variables that will be used by ERB
+      bin_path = Bundler.bin_path
+      template = File.read(File.expand_path("../templates/Executable.standalone", __FILE__))
+      ruby_command = ruby_command = Thor::Util.ruby_command
+
+      spec.executables.each do |executable|
+        next if executable == "bundle"
+        standalone_path = standalone_path = Pathname(Bundler.settings[:path]).expand_path.relative_path_from(bin_path)
+        executable_path = executable_path = Pathname(spec.full_gem_path).join(spec.bindir, executable).relative_path_from(bin_path)
+        File.open "#{bin_path}/#{executable}", "w", 0755 do |f|
+          f.puts ERB.new(template, nil, "-").result(binding)
+        end
+      end
+    end
+
   private
 
     # the order that the resolver provides is significant, since
@@ -194,22 +166,6 @@ module Bundler
       end
     end
 
-    def generate_standalone_bundler_executable_stubs(spec)
-      # double-assignment to avoid warnings about variables that will be used by ERB
-      bin_path = Bundler.bin_path
-      template = File.read(File.expand_path("../templates/Executable.standalone", __FILE__))
-      ruby_command = ruby_command = Thor::Util.ruby_command
-
-      spec.executables.each do |executable|
-        next if executable == "bundle"
-        standalone_path = standalone_path = Pathname(Bundler.settings[:path]).expand_path.relative_path_from(bin_path)
-        executable_path = executable_path = Pathname(spec.full_gem_path).join(spec.bindir, executable).relative_path_from(bin_path)
-        File.open "#{bin_path}/#{executable}", "w", 0755 do |f|
-          f.puts ERB.new(template, nil, "-").result(binding)
-        end
-      end
-    end
-
     def install_in_parallel(size, standalone, force = false)
       ParallelInstaller.call(self, specs, size, standalone, force)
     end
diff --git a/lib/bundler/installer/gem_installer.rb b/lib/bundler/installer/gem_installer.rb
new file mode 100644
index 0000000000..f6f865b2ab
--- /dev/null
+++ b/lib/bundler/installer/gem_installer.rb
@@ -0,0 +1,76 @@
+module Bundler
+  class GemInstaller
+    attr_reader :spec, :standalone, :worker, :force, :installer
+
+    def initialize(spec, installer, standalone = false, worker = 0, force = false)
+      @spec = spec
+      @installer = installer
+      @standalone = standalone
+      @worker = worker
+      @force = force
+    end
+
+    def install_from_spec
+      post_install_message = spec_settings ? install_with_settings : install
+      Bundler.ui.debug "#{worker}:  #{spec.name} (#{spec.version}) from #{spec.loaded_from}"
+      generate_executable_stubs
+      post_install_message
+
+    rescue Errno::ENOSPC
+      raise Bundler::InstallError, out_of_space_message
+    rescue => e
+      handle_exception(e)
+    end
+
+  private
+
+    def failure_message
+      return install_error_message if spec.source.options["git"]
+      "#{install_error_message}\n#{gem_install_message}"
+    end
+
+    def install_error_message
+      "An error occurred while installing #{spec.name} (#{spec.version}), and Bundler cannot continue."
+    end
+
+    def gem_install_message
+      "Make sure that `gem install #{spec.name} -v '#{spec.version}'` succeeds before bundling."
+    end
+
+    def handle_exception(e)
+      # if install hook failed or gem signature is bad, just die
+      raise e if e.is_a?(Bundler::InstallHookError) || e.is_a?(Bundler::SecurityError)
+      # other failure, likely a native extension build failure
+      Bundler.ui.info ""
+      Bundler.ui.warn "#{e.class}: #{e.message}"
+      Bundler.ui.debug e.backtrace.join("\n")
+      raise Bundler::InstallError, failure_message
+    end
+
+    def spec_settings
+      # Fetch the build settings, if there are any
+      Bundler.settings["build.#{spec.name}"]
+    end
+
+    def install
+      spec.source.install(spec, :force => force, :ensure_builtin_gems_cached => standalone)
+    end
+
+    def install_with_settings
+      # Build arguments are global, so this is mutexed
+      Bundler.rubygems.with_build_args([spec_settings]) { install }
+    end
+
+    def out_of_space_message
+      "Your disk is out of space. Free some space to be able to install your bundle."
+    end
+
+    def generate_executable_stubs
+      if Bundler.settings[:bin] && standalone
+        installer.generate_standalone_bundler_executable_stubs(spec)
+      elsif Bundler.settings[:bin]
+        installer.generate_bundler_executable_stubs(spec, :force => true)
+      end
+    end
+  end
+end
diff --git a/lib/bundler/installer/parallel_installer.rb b/lib/bundler/installer/parallel_installer.rb
index c1a8065a09..d32204fab3 100644
--- a/lib/bundler/installer/parallel_installer.rb
+++ b/lib/bundler/installer/parallel_installer.rb
@@ -1,4 +1,5 @@
 require "bundler/worker"
+require "bundler/installer/gem_installer"
 
 class ParallelInstaller
   class SpecInstallation
@@ -84,7 +85,9 @@ class ParallelInstaller
 
   def worker_pool
     @worker_pool ||= Bundler::Worker.new @size, lambda { |spec_install, worker_num|
-      message = @installer.install_gem_from_spec spec_install.spec, @standalone, worker_num, @force
+      message = Bundler::GemInstaller.new(
+        spec_install.spec, @installer, @standalone, worker_num, @force
+      ).install_from_spec
       spec_install.post_install_message = message unless message.nil?
       spec_install
     }
diff --git a/lib/bundler/gem_installer.rb b/lib/bundler/rubygems_gem_installer.rb
index 8124e4bcfc..5d7be0cb7e 100644
--- a/lib/bundler/gem_installer.rb
+++ b/lib/bundler/rubygems_gem_installer.rb
@@ -1,7 +1,7 @@
 require "rubygems/installer"
 
 module Bundler
-  class GemInstaller < Gem::Installer
+  class RubyGemsGemInstaller < Gem::Installer
     def check_executable_overwrite(filename)
       # Bundler needs to install gems regardless of binstub overwriting
     end
diff --git a/lib/bundler/source/path/installer.rb b/lib/bundler/source/path/installer.rb
index 87e9a6e9ca..74542dd913 100644
--- a/lib/bundler/source/path/installer.rb
+++ b/lib/bundler/source/path/installer.rb
@@ -1,7 +1,7 @@
 module Bundler
   class Source
     class Path
-      class Installer < Bundler::GemInstaller
+      class Installer < Bundler::RubyGemsGemInstaller
         attr_reader :spec
 
         def initialize(spec, options = {})
diff --git a/lib/bundler/source/rubygems.rb b/lib/bundler/source/rubygems.rb
index b91f32acd2..8e3173c666 100644
--- a/lib/bundler/source/rubygems.rb
+++ b/lib/bundler/source/rubygems.rb
@@ -132,7 +132,7 @@ module Bundler
 
           installed_spec = nil
           Bundler.rubygems.preserve_paths do
-            installed_spec = Bundler::GemInstaller.new(
+            installed_spec = Bundler::RubyGemsGemInstaller.new(
               path,
               :install_dir         => install_path.to_s,
               :bin_dir             => bin_path.to_s,