Merge pull request #8764 from rubygems/deivid-rodriguez/fix-local-ext-reinstall

Fix `gem install` sometimes compiling the wrong source files

Author: deivid-rodriguez

lib/rubygems/request_set.rb

@@ -181,13 +181,10 @@ def install(options, &block) # :yields: request, installer
 
     # Install requested gems after they have been downloaded
     sorted_requests.each do |req|
-      if req.installed?
+      if req.installed? && @always_install.none? {|spec| spec == req.spec.spec }
         req.spec.spec.build_extensions
-
-        if @always_install.none? {|spec| spec == req.spec.spec }
-          yield req, nil if block_given?
-          next
-        end
+        yield req, nil if block_given?
+        next
       end
 
       spec =

test/rubygems/helper.rb

@@ -683,6 +683,14 @@ def write_file(path)
     path
   end
 
+  def write_dummy_extconf(gem_name)
+    write_file File.join(@tempdir, "extconf.rb") do |io|
+      io.puts "require 'mkmf'"
+      yield io if block_given?
+      io.puts "create_makefile '#{gem_name}'"
+    end
+  end
+
   ##
   # Load a YAML string, the psych 3 way
 

test/rubygems/test_gem_commands_install_command.rb

@@ -647,17 +647,10 @@ def test_execute_rdoc
     @cmd.options[:args] = %w[a]
 
     use_ui @ui do
-      # Don't use Dir.chdir with a block, it warnings a lot because
-      # of a downstream Dir.chdir with a block
-      old = Dir.getwd
-
-      begin
-        Dir.chdir @tempdir
+      Dir.chdir @tempdir do
         assert_raise Gem::MockGemUi::SystemExitException, @ui.error do
           @cmd.execute
         end
-      ensure
-        Dir.chdir old
       end
     end
 
@@ -684,17 +677,10 @@ def test_execute_rdoc_with_path
     @cmd.options[:args] = %w[a]
 
     use_ui @ui do
-      # Don't use Dir.chdir with a block, it warnings a lot because
-      # of a downstream Dir.chdir with a block
-      old = Dir.getwd
-
-      begin
-        Dir.chdir @tempdir
+      Dir.chdir @tempdir do
         assert_raise Gem::MockGemUi::SystemExitException, @ui.error do
           @cmd.execute
         end
-      ensure
-        Dir.chdir old
       end
     end
 
@@ -720,17 +706,10 @@ def test_execute_saves_build_args
     @cmd.options[:args] = %w[a]
 
     use_ui @ui do
-      # Don't use Dir.chdir with a block, it warnings a lot because
-      # of a downstream Dir.chdir with a block
-      old = Dir.getwd
-
-      begin
-        Dir.chdir @tempdir
+      Dir.chdir @tempdir do
         assert_raise Gem::MockGemUi::SystemExitException, @ui.error do
           @cmd.execute
         end
-      ensure
-        Dir.chdir old
       end
     end
 

test/rubygems/test_gem_dependency_installer.rb

@@ -382,13 +382,9 @@ def test_install_dependency_existing_extension
     FileUtils.mv f1_gem, @tempdir
     inst = nil
 
-    pwd = Dir.getwd
-    Dir.chdir @tempdir
-    begin
+    Dir.chdir @tempdir do
       inst = Gem::DependencyInstaller.new
       inst.install "f"
-    ensure
-      Dir.chdir pwd
     end
 
     assert_equal %w[f-1], inst.installed_gems.map(&:full_name)
@@ -523,6 +519,58 @@ def test_install_local_subdir
     assert_equal %w[a-1], inst.installed_gems.map(&:full_name)
   end
 
+  def test_install_local_with_extensions_already_installed
+    pend "needs investigation" if Gem.java_platform?
+    pend "ruby.h is not provided by ruby repo" if ruby_repo?
+
+    @spec = quick_gem "a" do |s|
+      s.extensions << "extconf.rb"
+      s.files += %w[extconf.rb a.c]
+    end
+
+    write_dummy_extconf "a"
+
+    c_source_path = File.join(@tempdir, "a.c")
+
+    write_file c_source_path do |io|
+      io.write <<-C
+        #include <ruby.h>
+        void Init_a() { }
+      C
+    end
+
+    package_path = Gem::Package.build @spec
+    installer = Gem::Installer.at(package_path)
+
+    # Make sure the gem is installed and backup the correct package
+
+    installer.install
+
+    package_bkp_path = "#{package_path}.bkp"
+    FileUtils.cp package_path, package_bkp_path
+
+    # Break the extension, rebuild it, and try to install it
+
+    write_file c_source_path do |io|
+      io.write "typo"
+    end
+
+    Gem::Package.build @spec
+
+    assert_raise Gem::Ext::BuildError do
+      installer.install
+    end
+
+    # Make sure installing the good package again still works
+
+    FileUtils.cp "#{package_path}.bkp", package_path
+
+    Dir.chdir @tempdir do
+      inst = Gem::DependencyInstaller.new domain: :local
+      inst.install package_path
+    end
+  end
+
   def test_install_minimal_deps
     util_setup_gems
 

test/rubygems/test_gem_installer.rb

@@ -1478,12 +1478,7 @@ def test_install_extension_dir
 
     @spec = setup_base_spec
     @spec.extensions << "extconf.rb"
-    write_file File.join(@tempdir, "extconf.rb") do |io|
-      io.write <<-RUBY
-        require "mkmf"
-        create_makefile("#{@spec.name}")
-      RUBY
-    end
+    write_dummy_extconf @spec.name
 
     @spec.files += %w[extconf.rb]
 
@@ -1503,12 +1498,7 @@ def test_install_extension_dir_is_removed_on_reinstall
     @spec = setup_base_spec
 
     @spec.extensions << "extconf.rb"
-    write_file File.join(@tempdir, "extconf.rb") do |io|
-      io.write <<-RUBY
-        require "mkmf"
-        create_makefile("#{@spec.name}")
-      RUBY
-    end
+    write_dummy_extconf @spec.name
 
     @spec.files += %w[extconf.rb]
 
@@ -1539,12 +1529,7 @@ def test_install_extension_dir_is_removed_on_reinstall
   def test_install_user_extension_dir
     @spec = setup_base_spec
     @spec.extensions << "extconf.rb"
-    write_file File.join(@tempdir, "extconf.rb") do |io|
-      io.write <<-RUBY
-        require "mkmf"
-        create_makefile("#{@spec.name}")
-      RUBY
-    end
+    write_dummy_extconf @spec.name
 
     @spec.files += %w[extconf.rb]
 
@@ -1571,15 +1556,13 @@ def test_find_lib_file_after_install
 
     @spec = setup_base_spec
     @spec.extensions << "extconf.rb"
-    write_file File.join(@tempdir, "extconf.rb") do |io|
+    write_dummy_extconf @spec.name do |io|
       io.write <<-RUBY
-        require "mkmf"
 
         CONFIG['CC'] = '$(TOUCH) $@ ||'
         CONFIG['LDSHARED'] = '$(TOUCH) $@ ||'
         $ruby = '#{Gem.ruby}'
 
-        create_makefile("#{@spec.name}")
       RUBY
     end
 
@@ -1618,12 +1601,7 @@ def test_install_extension_and_script
 
     @spec = setup_base_spec
     @spec.extensions << "extconf.rb"
-    write_file File.join(@tempdir, "extconf.rb") do |io|
-      io.write <<-RUBY
-        require "mkmf"
-        create_makefile("#{@spec.name}")
-      RUBY
-    end
+    write_dummy_extconf @spec.name
 
     rb = File.join("lib", "#{@spec.name}.rb")
     @spec.files += [rb]
@@ -1663,15 +1641,13 @@ def test_install_extension_flat
 
       @spec.extensions << "extconf.rb"
 
-      write_file File.join(@tempdir, "extconf.rb") do |io|
+      write_dummy_extconf @spec.name do |io|
         io.write <<-RUBY
-          require "mkmf"
 
           CONFIG['CC'] = '$(TOUCH) $@ ||'
           CONFIG['LDSHARED'] = '$(TOUCH) $@ ||'
           $ruby = '#{Gem.ruby}'
 
-          create_makefile("#{@spec.name}")
         RUBY
       end
 
@@ -1698,13 +1674,13 @@ def test_install_extension_clean_intermediate_files
     @spec.require_paths = ["."]
     @spec.extensions << "extconf.rb"
 
-    File.write File.join(@tempdir, "extconf.rb"), <<-RUBY
-      require "mkmf"
-      CONFIG['CC'] = '$(TOUCH) $@ ||'
-      CONFIG['LDSHARED'] = '$(TOUCH) $@ ||'
-      $ruby = '#{Gem.ruby}'
-      create_makefile("#{@spec.name}")
-    RUBY
+    write_dummy_extconf @spec.name do |io|
+      io.write <<~RUBY
+        CONFIG['CC'] = '$(TOUCH) $@ ||'
+        CONFIG['LDSHARED'] = '$(TOUCH) $@ ||'
+        $ruby = '#{Gem.ruby}'
+      RUBY
+    end
 
     # empty depend file for no auto dependencies
     @spec.files += %W[depend #{@spec.name}.c].each do |file|