Auto merge of #4272 - bundler:git-extension-rebuild, r=segiddins

Skip building git gem extensions if they're already built

None

10 files changed, 136 insertions, 45 deletions

diff --git a/lib/bundler.rb b/lib/bundler.rb
index 5cfff3d573..2958e64cee 100644
--- a/lib/bundler.rb
+++ b/lib/bundler.rb
@@ -355,18 +355,16 @@ module Bundler
       # depend on "./" relative paths.
       SharedHelpers.chdir(path.dirname.to_s) do
         contents = path.read
-        if contents[0..2] == "---" # YAML header
-          spec = eval_yaml_gemspec(path, contents)
+        spec = if contents[0..2] == "---" # YAML header
+          eval_yaml_gemspec(path, contents)
         else
-          spec = eval_gemspec(path, contents)
+          eval_gemspec(path, contents)
         end
-        Bundler.rubygems.validate(spec) if spec && validate
+        return unless spec
+        spec.loaded_from = path.expand_path.to_s
+        Bundler.rubygems.validate(spec) if validate
         spec
       end
-    rescue Gem::InvalidSpecificationException => e
-      error_message = "The gemspec at #{file} is not valid. Please fix this gemspec.\n" \
-        "The validation error was '#{e.message}'\n"
-      raise Gem::InvalidSpecificationException.new(error_message)
     end
 
     def clear_gemspec_cache
diff --git a/lib/bundler/rubygems_ext.rb b/lib/bundler/rubygems_ext.rb
index 22c22a2dd0..2f36c29cd9 100644
--- a/lib/bundler/rubygems_ext.rb
+++ b/lib/bundler/rubygems_ext.rb
@@ -53,7 +53,7 @@ module Gem
     if method_defined?(:extension_dir)
       alias_method :rg_extension_dir, :extension_dir
       def extension_dir
-        @extension_dir ||= if source.respond_to?(:extension_dir_name)
+        @bundler_extension_dir ||= if source.respond_to?(:extension_dir_name)
           File.expand_path(File.join(extensions_dir, source.extension_dir_name))
         else
           rg_extension_dir
diff --git a/lib/bundler/rubygems_integration.rb b/lib/bundler/rubygems_integration.rb
index 0f567db12a..4e5ca7d1f8 100644
--- a/lib/bundler/rubygems_integration.rb
+++ b/lib/bundler/rubygems_integration.rb
@@ -54,10 +54,23 @@ module Bundler
 
     def validate(spec)
       Bundler.ui.silence { spec.validate(false) }
+    rescue Gem::InvalidSpecificationException => e
+      error_message = "The gemspec at #{spec.loaded_from} is not valid. Please fix this gemspec.\n" \
+        "The validation error was '#{e.message}'\n"
+      raise Gem::InvalidSpecificationException.new(error_message)
     rescue Errno::ENOENT
       nil
     end
 
+    def set_installed_by_version(spec, installed_by_version = Gem::VERSION)
+      return unless spec.respond_to?(:installed_by_version=)
+      spec.installed_by_version = Gem::Version.create(installed_by_version)
+    end
+
+    def spec_missing_extensions?(spec)
+      !spec.respond_to?(:missing_extensions?) || spec.missing_extensions?
+    end
+
     def path(obj)
       obj.to_s
     end
@@ -503,9 +516,7 @@ module Bundler
         # Missing summary is downgraded to a warning in later versions,
         # so we set it to an empty string to prevent an exception here.
         spec.summary ||= ""
-        Bundler.ui.silence { spec.validate(false) }
-      rescue Errno::ENOENT
-        nil
+        RubygemsIntegration.instance_method(:validate).bind(self).call(spec)
       end
     end
 
diff --git a/lib/bundler/source/git.rb b/lib/bundler/source/git.rb
index 90bbd13d34..228ab61a67 100644
--- a/lib/bundler/source/git.rb
+++ b/lib/bundler/source/git.rb
@@ -222,6 +222,10 @@ module Bundler
 
     private
 
+      def build_extensions(installer)
+        super if Bundler.rubygems.spec_missing_extensions?(installer.spec)
+      end
+
       def serialize_gemspecs_in(destination)
         expanded_path = destination.expand_path(Bundler.root)
         Dir["#{expanded_path}/#{@glob}"].each do |spec_path|
diff --git a/lib/bundler/source/path.rb b/lib/bundler/source/path.rb
index 6bfd2e88c7..9790ba5164 100644
--- a/lib/bundler/source/path.rb
+++ b/lib/bundler/source/path.rb
@@ -135,9 +135,12 @@ module Bundler
         if File.directory?(expanded_path)
           # We sort depth-first since `<<` will override the earlier-found specs
           Dir["#{expanded_path}/#{@glob}"].sort_by {|p| -p.split(File::SEPARATOR).size }.each do |file|
-            next unless spec = Bundler.load_gemspec(file, :validate)
-            spec.loaded_from = file.to_s
+            next unless spec = Bundler.load_gemspec(file)
             spec.source = self
+            Bundler.rubygems.set_installed_by_version(spec)
+            # Validation causes extension_dir to be calculated, which depends
+            # on #source, so we validate here instead of load_gemspec
+            Bundler.rubygems.validate(spec)
             index << spec
           end
 
@@ -194,8 +197,7 @@ module Bundler
         SharedHelpers.chdir(gem_dir) do
           installer = Path::Installer.new(spec, :env_shebang => false)
           run_hooks(:pre_install, installer)
-          installer.build_extensions unless disable_extensions
-          run_hooks(:post_build, installer)
+          build_extensions(installer) unless disable_extensions
           installer.generate_bin
           run_hooks(:post_install, installer)
         end
@@ -213,6 +215,11 @@ module Bundler
         Bundler.ui.warn "The validation message from Rubygems was:\n  #{e.message}"
       end
 
+      def build_extensions(installer)
+        installer.build_extensions
+        run_hooks(:post_build, installer)
+      end
+
       def run_hooks(type, installer)
         hooks_meth = "#{type}_hooks"
         return unless Gem.respond_to?(hooks_meth)
diff --git a/spec/bundler/bundler_spec.rb b/spec/bundler/bundler_spec.rb
index 9d4353f718..84d2922f37 100644
--- a/spec/bundler/bundler_spec.rb
+++ b/spec/bundler/bundler_spec.rb
@@ -75,32 +75,31 @@ describe Bundler do
       end
     end
 
-    context "validate is true" do
-      subject { Bundler.load_gemspec_uncached(app_gemspec_path, true) }
-
-      context "and there are gemspec validation errors" do
-        let(:ui_shell) { double(:ui_shell) }
+    it "sets loaded_from" do
+      app_gemspec_path.open("w") do |f|
+        f.puts <<-GEMSPEC
+          Gem::Specification.new do |gem|
+            gem.name = "validated"
+          end
+        GEMSPEC
+      end
 
-        before do
-          allow(Bundler::UI::Shell).to receive(:new).and_return(ui_shell)
-          allow(Bundler.rubygems).to receive(:validate) {
-            raise Gem::InvalidSpecificationException.new("TODO is not an author")
-          }
-        end
+      expect(subject.loaded_from).to eq(app_gemspec_path.expand_path.to_s)
+    end
 
-        it "should raise a Gem::InvalidSpecificationException and produce a helpful warning message" do
-          File.open(app_gemspec_path, "wb") do |file|
-            file.puts <<-GEMSPEC.gsub(/^\s+/, "")
-              Gem::Specification.new do |gem|
-                gem.author = "TODO"
-              end
-            GEMSPEC
-          end
+    context "validate is true" do
+      subject { Bundler.load_gemspec_uncached(app_gemspec_path, true) }
 
-          expect { subject }.to raise_error(Gem::InvalidSpecificationException,
-            "The gemspec at #{app_gemspec_path} is not valid. "\
-            "Please fix this gemspec.\nThe validation error was 'TODO is not an author'\n")
+      it "validates the specification" do
+        app_gemspec_path.open("w") do |f|
+          f.puts <<-GEMSPEC
+            Gem::Specification.new do |gem|
+              gem.name = "validated"
+            end
+          GEMSPEC
         end
+        expect(Bundler.rubygems).to receive(:validate).with have_attributes(:name => "validated")
+        subject
       end
     end
   end
diff --git a/spec/bundler/rubygems_integration_spec.rb b/spec/bundler/rubygems_integration_spec.rb
index 917d673b51..fbc49c414c 100644
--- a/spec/bundler/rubygems_integration_spec.rb
+++ b/spec/bundler/rubygems_integration_spec.rb
@@ -7,16 +7,43 @@ describe Bundler::RubygemsIntegration do
   end
 
   context "#validate" do
-    let(:spec) { double("spec", :summary => "") }
+    let(:spec) do
+      Gem::Specification.new do |s|
+        s.name = "to-validate"
+        s.version = "1.0.0"
+        s.loaded_from = __FILE__
+      end
+    end
+    subject { Bundler.rubygems.validate(spec) }
 
     it "skips overly-strict gemspec validation", :rubygems => "< 1.7" do
       expect(spec).to_not receive(:validate)
-      Bundler.rubygems.validate(spec)
+      subject
     end
 
     it "validates with packaging mode disabled", :rubygems => "1.7" do
       expect(spec).to receive(:validate).with(false)
-      Bundler.rubygems.validate(spec)
+      subject
+    end
+
+    it "should set a summary to avoid an overly-strict error", :rubygems => "~> 1.7.0" do
+      spec.summary = nil
+      expect { subject }.not_to raise_error
+      expect(spec.summary).to eq("")
+    end
+
+    context "with an invalid spec" do
+      before do
+        expect(spec).to receive(:validate).with(false).
+          and_raise(Gem::InvalidSpecificationException.new("TODO is not an author"))
+      end
+
+      it "should raise a Gem::InvalidSpecificationException and produce a helpful warning message",
+        :rubygems => "1.7" do
+        expect { subject }.to raise_error(Gem::InvalidSpecificationException,
+          "The gemspec at #{__FILE__} is not valid. "\
+          "Please fix this gemspec.\nThe validation error was 'TODO is not an author'\n")
+      end
     end
   end
 
diff --git a/spec/commands/clean_spec.rb b/spec/commands/clean_spec.rb
index 95035555d4..37f569b267 100644
--- a/spec/commands/clean_spec.rb
+++ b/spec/commands/clean_spec.rb
@@ -660,9 +660,11 @@ describe "bundle clean" do
       gem "very_simple_git_binary", :git => "#{lib_path("very_simple_git_binary-1.0")}", :ref => "#{revision}"
     G
 
-    bundle "install --path vendor/bundle"
-    bundle :clean
+    bundle! "install --path vendor/bundle"
+    expect(vendored_gems("bundler/gems/extensions")).to exist
+    expect(vendored_gems("bundler/gems/very_simple_git_binary-1.0-#{revision[0..11]}")).to exist
 
+    bundle! :clean
     expect(out).to eq("")
 
     expect(vendored_gems("bundler/gems/extensions")).to exist
diff --git a/spec/commands/package_spec.rb b/spec/commands/package_spec.rb
index 0ebb65b1a9..9d88e4467c 100644
--- a/spec/commands/package_spec.rb
+++ b/spec/commands/package_spec.rb
@@ -38,6 +38,8 @@ describe "bundle package" do
             Gem::Specification.new do |s|
               s.name = "mygem"
               s.version = "0.1.1"
+              s.summary = ""
+              s.authors = ["gem author"]
               s.add_development_dependency "nokogiri", "=1.4.2"
             end
           G
@@ -50,8 +52,8 @@ describe "bundle package" do
           gemspec
         D
 
-        bundle "package --all"
-        sleep 20
+        bundle! "package --all"
+
         expect(bundled_app("vendor/cache/rack-1.0.0.gem")).to exist
         expect(bundled_app("vendor/cache/nokogiri-1.4.2.gem")).to exist
         expect(bundled_app("vendor/cache/mygem-0.1.1.gem")).to_not exist
diff --git a/spec/install/gemfile/git_spec.rb b/spec/install/gemfile/git_spec.rb
index 66a721521e..5081d35fa7 100644
--- a/spec/install/gemfile/git_spec.rb
+++ b/spec/install/gemfile/git_spec.rb
@@ -980,6 +980,47 @@ describe "bundle install with git sources" do
       expect(out).to include("An error occurred while installing foo (1.0)")
       expect(out).not_to include("gem install foo")
     end
+
+    it "does not reinstall the extension", :rubygems => ">= 2.3.0" do
+      build_git "foo" do |s|
+        s.add_dependency "rake"
+        s.extensions << "Rakefile"
+        s.write "Rakefile", <<-RUBY
+          task :default do
+            path = File.expand_path("../lib", __FILE__)
+            FileUtils.mkdir_p(path)
+            cur_time = Time.now.to_f.to_s
+            File.open("\#{path}/foo.rb", "w") do |f|
+              f.puts "FOO = \#{cur_time}"
+            end
+          end
+        RUBY
+      end
+
+      install_gemfile <<-G
+        source "file://#{gem_repo1}"
+        gem "foo", :git => "#{lib_path("foo-1.0")}"
+      G
+
+      run <<-R
+        require 'foo'
+        puts FOO
+      R
+
+      installed_time = out
+      expect(installed_time).to match(/\d+\.\d+/)
+
+      install_gemfile <<-G
+        source "file://#{gem_repo1}"
+        gem "foo", :git => "#{lib_path("foo-1.0")}"
+      G
+
+      run <<-R
+        require 'foo'
+        puts FOO
+      R
+      expect(out).to eq(installed_time)
+    end
   end
 
   it "ignores git environment variables" do