diff --git a/lib/rubygems/commands/install_command.rb b/lib/rubygems/commands/install_command.rb index 6d3beec0b432..5e7a5a410670 100644 --- a/lib/rubygems/commands/install_command.rb +++ b/lib/rubygems/commands/install_command.rb @@ -93,7 +93,7 @@ def description # :nodoc: [build fails] Gem files will remain installed in \\ /path/to/gems/some_extension_gem-1.0 for inspection. - Results logged to /path/to/gems/some_extension_gem-1.0/gem_make.out + Results logged to /path/to/build_info/some_extension_gem-1.0.gem_make.out $ gem install some_extension_gem -- --with-extension-lib=/path/to/lib [build succeeds] $ gem list some_extension_gem @@ -110,7 +110,7 @@ def description # :nodoc: [build fails] Gem files will remain installed in \\ /path/to/gems/some_extension_gem-1.0 for inspection. - Results logged to /path/to/gems/some_extension_gem-1.0/gem_make.out + Results logged to /path/to/build_info/some_extension_gem-1.0.gem_make.out $ [cd /path/to/gems/some_extension_gem-1.0] $ [edit files or what-have-you and run make] $ gem spec ../../cache/some_extension_gem-1.0.gem --ruby > \\ diff --git a/lib/rubygems/ext/builder.rb b/lib/rubygems/ext/builder.rb index 53a02f61f31b..ca1b4c2db615 100644 --- a/lib/rubygems/ext/builder.rb +++ b/lib/rubygems/ext/builder.rb @@ -222,9 +222,25 @@ def build_extension(extension, dest_path) # :nodoc: verbose { results.join("\n") } - write_gem_make_out results.join "\n" + # mkmf.log is a noisy, non-reproducible build artifact that is not meant + # to be installed. Drop the one left behind by a successful build instead + # of leaving it in the installation tree. + FileUtils.rm_f File.join(extension_dir, "mkmf.log") rescue StandardError => e results << e.message + + # On failure keep the mkmf.log for inspection, but move it out of the + # installation tree into the build_info directory. + mkmf_log = File.join extension_dir, "mkmf.log" + if File.exist?(mkmf_log) + mkmf_log_dest = File.join @spec.build_info_dir, "#{@spec.full_name}.mkmf.log" + FileUtils.mkdir_p @spec.build_info_dir + FileUtils.mv mkmf_log, mkmf_log_dest + + results << "To see why this extension failed to compile, please check the mkmf.log which can be found here:" + results << " #{mkmf_log_dest}" + end + build_error(results.join("\n"), $@) end end @@ -256,12 +272,14 @@ def build_extensions end ## - # Writes +output+ to gem_make.out in the extension install directory. + # Writes +output+ to gem_make.out in the build_info directory. Only called + # on failure (via #build_error), to keep build logs out of the installation + # tree. def write_gem_make_out(output) # :nodoc: - destination = File.join @spec.extension_dir, "gem_make.out" + destination = File.join @spec.build_info_dir, "#{@spec.full_name}.gem_make.out" - FileUtils.mkdir_p @spec.extension_dir + FileUtils.mkdir_p @spec.build_info_dir File.open destination, "wb" do |io| io.puts output diff --git a/lib/rubygems/ext/ext_conf_builder.rb b/lib/rubygems/ext/ext_conf_builder.rb index 822454355d10..a658e0b0eb59 100644 --- a/lib/rubygems/ext/ext_conf_builder.rb +++ b/lib/rubygems/ext/ext_conf_builder.rb @@ -27,17 +27,10 @@ def self.build(extension, dest_path, results, args = [], lib_dir = nil, extensio cmd << "--target-rbconfig=#{target_rbconfig.path}" if target_rbconfig.path cmd.push(*args) - run(cmd, results, class_name, extension_dir) do |s, r| - mkmf_log = File.join(extension_dir, "mkmf.log") - if File.exist? mkmf_log - unless s.success? - r << "To see why this extension failed to compile, please check" \ - " the mkmf.log which can be found here:\n" - r << " " + File.join(dest_path, "mkmf.log") + "\n" - end - FileUtils.mv mkmf_log, dest_path - end - end + # Leave mkmf.log in the extension directory. The final placement (dropped + # on success, moved to build_info on failure) is decided by + # Gem::Ext::Builder#build_extension. + run(cmd, results, class_name, extension_dir) ENV["DESTDIR"] = nil diff --git a/lib/rubygems/uninstaller.rb b/lib/rubygems/uninstaller.rb index fe4c3a80cf92..ff4982d9945f 100644 --- a/lib/rubygems/uninstaller.rb +++ b/lib/rubygems/uninstaller.rb @@ -271,6 +271,8 @@ def remove(spec) safe_delete { rm_r full_gem_path, exclusions: exclusions } safe_delete { FileUtils.rm_r spec.extension_dir } + safe_delete { FileUtils.rm_f File.join(spec.build_info_dir, "#{spec.full_name}.mkmf.log") } + safe_delete { FileUtils.rm_f File.join(spec.build_info_dir, "#{spec.full_name}.gem_make.out") } old_platform_name = spec.original_name diff --git a/spec/bundler/installer/parallel_installer_spec.rb b/spec/bundler/installer/parallel_installer_spec.rb index 528dc1ae93e5..be62ae59d0a6 100644 --- a/spec/bundler/installer/parallel_installer_spec.rb +++ b/spec/bundler/installer/parallel_installer_spec.rb @@ -147,34 +147,37 @@ let(:gem_two) { definition.specs.find {|spec| spec.name == "two" } } it "takes all available slots" do - redefine_build_jobs do + acquired = track_build_jobs(rendezvous: true) do Bundler::ParallelInstaller.call(installer, definition.specs, 5, false, true) end - # Take 3 slots out of the 5 available. - expect(File.read(File.join(gem_one.extension_dir, "gem_make.out"))).to include("make -j3") + # Take 3 slots (capped per gem) out of the 5 available. + expect(acquired["one"]).to eq(3) # Take the remaining 2 slots. - expect(File.read(File.join(gem_two.extension_dir, "gem_make.out"))).to include("make -j2") + expect(acquired["two"]).to eq(2) end it "fallback to non parallel when no slots are available" do - redefine_build_jobs do + acquired = track_build_jobs(rendezvous: true) do Bundler::ParallelInstaller.call(installer, definition.specs, 3, false, true) end # Take 3 slots out of the 3 available. - expect(File.read(File.join(gem_one.extension_dir, "gem_make.out"))).to include("make -j3") + expect(acquired["one"]).to eq(3) # Fallback to one slot (non parallel). - expect(File.read(File.join(gem_two.extension_dir, "gem_make.out"))).to_not include("make -j") + expect(acquired["two"]).to eq(1) end it "uses one jobs when installing serially" do + acquired = nil Bundler.settings.temporary(jobs: 1) do - Bundler::ParallelInstaller.call(installer, definition.specs, 1, false, true) + acquired = track_build_jobs do + Bundler::ParallelInstaller.call(installer, definition.specs, 1, false, true) + end end - expect(File.read(File.join(gem_one.extension_dir, "gem_make.out"))).to_not include("make -j") - expect(File.read(File.join(gem_two.extension_dir, "gem_make.out"))).to_not include("make -j") + expect(acquired["one"]).to eq(1) + expect(acquired["two"]).to eq(1) end it "release the job slots" do @@ -186,39 +189,52 @@ end end - Bundler::ParallelInstaller.call(installer, definition.specs, 3, false, true) + acquired = track_build_jobs do + Bundler::ParallelInstaller.call(installer, definition.specs, 3, false, true) + end # Take 3 slots out of the 3 available. - expect(File.read(File.join(gem_one.extension_dir, "gem_make.out"))).to include("make -j3") - # Take 3 slots that were released. - expect(File.read(File.join(gem_two.extension_dir, "gem_make.out"))).to include("make -j3") + expect(acquired["one"]).to eq(3) + # Take 3 slots that were released by `one`. + expect(acquired["two"]).to eq(3) end - def redefine_build_jobs + # Records how many jobserver slots each gem's build acquired. RubyGems turns + # that count directly into `make -jN`, so asserting on it verifies slot + # allocation and release without reading a build log, which a successful + # build no longer writes. With +rendezvous+, "one" grabs its slots first and + # holds them until "two" has grabbed the rest, making the split deterministic. + def track_build_jobs(rendezvous: false) + acquired = {} old_method = Bundler::RubyGemsGemInstaller.instance_method(:build_jobs) Bundler::RubyGemsGemInstaller.remove_method(:build_jobs) - # Rendezvous so that "one" grabs its slots first and keeps holding them - # until "two" has grabbed the rest. Blocking on a queue avoids the - # busy-wait and makes the ordering deterministic. one_acquired = Thread::Queue.new two_acquired = Thread::Queue.new Bundler::RubyGemsGemInstaller.define_method(:build_jobs) do - if spec.name == "one" - value = old_method.bind(self).call - one_acquired << true - two_acquired.pop - elsif spec.name == "two" - one_acquired.pop - value = old_method.bind(self).call - two_acquired << true - end + value = + if rendezvous && spec.name == "one" + v = old_method.bind(self).call + one_acquired << true + two_acquired.pop + v + elsif rendezvous && spec.name == "two" + one_acquired.pop + v = old_method.bind(self).call + two_acquired << true + v + else + old_method.bind(self).call + end + acquired[spec.name] = value value end yield + + acquired ensure Bundler::RubyGemsGemInstaller.remove_method(:build_jobs) Bundler::RubyGemsGemInstaller.define_method(:build_jobs, old_method) diff --git a/spec/commands/install_spec.rb b/spec/commands/install_spec.rb index f8a134f23108..7048c0f4d253 100644 --- a/spec/commands/install_spec.rb +++ b/spec/commands/install_spec.rb @@ -1369,10 +1369,18 @@ def run s.extensions = extension s.write(extension, extconf_code) + # A successful build no longer leaves gem_make.out behind. Force the + # build to fail at the make stage so the make command line, including + # the jobserver `-j`, is recorded in build_info for these assertions. + s.write("ext/mypsych/mypsych.c", "#error forced build failure for test") end end end + def gem_make_out + File.read(File.join(@gemspec.build_info_dir, "#{@gemspec.full_name}.gem_make.out")) + end + after do if @old_makeflags ENV["MAKEFLAGS"] = @old_makeflags @@ -1384,39 +1392,33 @@ def run it "doesn't pass down -j to make when MAKEFLAGS is set" do ENV["MAKEFLAGS"] = "-j1" - install_gemfile(<<~G, env: { "BUNDLE_JOBS" => "8" }) + install_gemfile(<<~G, env: { "BUNDLE_JOBS" => "8" }, raise_on_error: false) source "https://gem.repo4" gem "mypsych" G - gem_make_out = File.read(File.join(@gemspec.extension_dir, "gem_make.out")) - expect(gem_make_out).not_to include("make -j8") end it "uses 3 slots from the available pool when running the compilation of an extension" do ENV.delete("MAKEFLAGS") - install_gemfile(<<~G, env: { "BUNDLE_JOBS" => "8" }) + install_gemfile(<<~G, env: { "BUNDLE_JOBS" => "8" }, raise_on_error: false) source "https://gem.repo4" gem "mypsych" G - gem_make_out = File.read(File.join(@gemspec.extension_dir, "gem_make.out")) - expect(gem_make_out).to include("make -j3") end it "consumes 3 slots from the pool when BUNDLE_JOBS isn't set" do ENV.delete("MAKEFLAGS") - install_gemfile(<<~G) + install_gemfile(<<~G, raise_on_error: false) source "https://gem.repo4" gem "mypsych" G - gem_make_out = File.read(File.join(@gemspec.extension_dir, "gem_make.out")) - expect(gem_make_out).to include("make -j3") end end diff --git a/test/rubygems/test_gem_commands_install_command.rb b/test/rubygems/test_gem_commands_install_command.rb index d75ba349f96a..39b6e5951df7 100644 --- a/test/rubygems/test_gem_commands_install_command.rb +++ b/test/rubygems/test_gem_commands_install_command.rb @@ -1587,6 +1587,9 @@ def test_pass_down_the_job_option_to_make write_file(extconf_path) do |io| io.puts "require 'mkmf'" + # Force the build to fail at the make stage so the build log is + # written. The make command line (including -j) is recorded there. + io.puts "File.write('a.c', '#error forced build failure for test')" io.puts "create_makefile '#{spec.name}'" end @@ -1595,12 +1598,12 @@ def test_pass_down_the_job_option_to_make end use_ui @ui do - assert_raise Gem::MockGemUi::SystemExitException, @ui.error do + assert_raise Gem::MockGemUi::TermError, @ui.error do @cmd.invoke "a", "-j4" end end - gem_make_out = File.read(File.join(gemspec.extension_dir, "gem_make.out")) + gem_make_out = File.read(File.join(gemspec.build_info_dir, "#{gemspec.full_name}.gem_make.out")) if vc_windows? && nmake_found? refute_includes(gem_make_out, " -j4") else diff --git a/test/rubygems/test_gem_commands_update_command.rb b/test/rubygems/test_gem_commands_update_command.rb index 5bc36d61035e..ec5c762b89dd 100644 --- a/test/rubygems/test_gem_commands_update_command.rb +++ b/test/rubygems/test_gem_commands_update_command.rb @@ -735,6 +735,9 @@ def test_pass_down_the_job_option_to_make write_file(extconf_path) do |io| io.puts "require 'mkmf'" + # Force the build to fail at the make stage so the build log is + # written. The make command line (including -j) is recorded there. + io.puts "File.write('a.c', '#error forced build failure for test')" io.puts "create_makefile '#{spec.name}'" end @@ -748,7 +751,7 @@ def test_pass_down_the_job_option_to_make @cmd.invoke("a", "-j2") end - gem_make_out = File.read(File.join(gemspec.extension_dir, "gem_make.out")) + gem_make_out = File.read(File.join(gemspec.build_info_dir, "#{gemspec.full_name}.gem_make.out")) if vc_windows? && nmake_found? refute_includes(gem_make_out, " -j2") else diff --git a/test/rubygems/test_gem_ext_builder.rb b/test/rubygems/test_gem_ext_builder.rb index 37204f3c472a..5285d7af1308 100644 --- a/test/rubygems/test_gem_ext_builder.rb +++ b/test/rubygems/test_gem_ext_builder.rb @@ -154,7 +154,9 @@ def test_build_extensions assert_path_exist @spec.extension_dir assert_path_exist @spec.gem_build_complete_path - assert_path_exist File.join @spec.extension_dir, "gem_make.out" + assert_path_not_exist File.join @spec.extension_dir, "gem_make.out" + assert_path_not_exist File.join @spec.extension_dir, "mkmf.log" + assert_path_not_exist File.join @spec.gem_dir, "ext", "mkmf.log" assert_path_exist File.join @spec.extension_dir, "a.rb" assert_path_exist File.join @spec.gem_dir, "lib", "a.rb" assert_path_exist File.join @spec.gem_dir, "lib", "a", "b.rb" @@ -208,7 +210,9 @@ def test_build_extensions_install_ext_only assert_path_exist @spec.extension_dir assert_path_exist @spec.gem_build_complete_path - assert_path_exist File.join @spec.extension_dir, "gem_make.out" + assert_path_not_exist File.join @spec.extension_dir, "gem_make.out" + assert_path_not_exist File.join @spec.extension_dir, "mkmf.log" + assert_path_not_exist File.join @spec.gem_dir, "ext", "mkmf.log" assert_path_exist File.join @spec.extension_dir, "a.rb" assert_path_not_exist File.join @spec.gem_dir, "lib", "a.rb" assert_path_not_exist File.join @spec.gem_dir, "lib", "a", "b.rb" @@ -261,13 +265,88 @@ def test_build_multiple_extensions assert_path_exist @spec.extension_dir assert_path_exist @spec.gem_build_complete_path assert_path_exist File.join @spec.gem_dir, "ext", "foo" - assert_path_exist File.join @spec.extension_dir, "gem_make.out" + assert_path_not_exist File.join @spec.extension_dir, "gem_make.out" + assert_path_not_exist File.join @spec.extension_dir, "mkmf.log" + assert_path_not_exist File.join @spec.gem_dir, "ext", "mkmf.log" assert_path_exist File.join @spec.extension_dir, "a.rb" assert_path_exist File.join @spec.gem_dir, "lib", "a.rb" assert_path_exist File.join @spec.gem_dir, "lib", "a", "b.rb" end end + def test_build_extensions_does_not_install_logs_on_success + pend "terminates on mswin" if vc_windows? && ruby_repo? + + @spec.extensions << "ext/extconf.rb" + + ext_dir = File.join @spec.gem_dir, "ext" + FileUtils.mkdir_p ext_dir + + File.open File.join(ext_dir, "extconf.rb"), "w" do |f| + f.write <<-'RUBY' + require 'mkmf' + + create_makefile 'a' + RUBY + end + + use_ui @ui do + @builder.build_extensions + end + + assert_path_exist @spec.gem_build_complete_path + + # No build logs are left anywhere in the installation tree. + assert_path_not_exist File.join @spec.extension_dir, "gem_make.out" + assert_path_not_exist File.join @spec.extension_dir, "mkmf.log" + assert_path_not_exist File.join ext_dir, "mkmf.log" + assert_path_not_exist File.join ext_dir, "gem_make.out" + assert_path_not_exist File.join @spec.build_info_dir, "#{@spec.full_name}.mkmf.log" + assert_path_not_exist File.join @spec.build_info_dir, "#{@spec.full_name}.gem_make.out" + end + + def test_build_extensions_logs_to_build_info_on_failure + pend "terminates on mswin" if vc_windows? && ruby_repo? + + @spec.extensions << "ext/extconf.rb" + + ext_dir = File.join @spec.gem_dir, "ext" + FileUtils.mkdir_p ext_dir + + File.open File.join(ext_dir, "extconf.rb"), "w" do |f| + f.write <<-'RUBY' + require 'mkmf' + + have_library 'nonexistent' or abort 'need libnonexistent' + + create_makefile 'a' + RUBY + end + + e = assert_raise Gem::Ext::BuildError do + use_ui @ui do + @builder.build_extensions + end + end + + mkmf_log = File.join @spec.build_info_dir, "#{@spec.full_name}.mkmf.log" + gem_make_out = File.join @spec.build_info_dir, "#{@spec.full_name}.gem_make.out" + + assert_path_exist mkmf_log + assert_path_exist gem_make_out + + # Logs are not left in the installation tree. + assert_path_not_exist File.join @spec.extension_dir, "mkmf.log" + assert_path_not_exist File.join @spec.extension_dir, "gem_make.out" + assert_path_not_exist File.join ext_dir, "mkmf.log" + + # The error message points at the new build_info paths. + assert_includes e.message, gem_make_out + assert_includes e.message, mkmf_log + + assert_path_not_exist @spec.gem_build_complete_path + end + def test_build_extensions_none use_ui @ui do @builder.build_extensions @@ -311,12 +390,14 @@ def test_build_extensions_extconf_bad assert_equal "Building native extensions. This could take a while...\n", @ui.output assert_equal "", @ui.error - gem_make_out = File.join @spec.extension_dir, "gem_make.out" + gem_make_out = File.join @spec.build_info_dir, "#{@spec.full_name}.gem_make.out" cmd_make_out = File.read(gem_make_out) assert_match %r{#{Regexp.escape Gem.ruby} .* extconf\.rb}, cmd_make_out assert_match(/: No such file/, cmd_make_out) + assert_path_not_exist File.join @spec.extension_dir, "gem_make.out" + assert_path_not_exist @spec.gem_build_complete_path assert_equal cwd, Dir.pwd @@ -324,7 +405,7 @@ def test_build_extensions_extconf_bad def test_build_extensions_unsupported FileUtils.mkdir_p @spec.gem_dir - gem_make_out = File.join @spec.extension_dir, "gem_make.out" + gem_make_out = File.join @spec.build_info_dir, "#{@spec.full_name}.gem_make.out" @spec.extensions << nil e = assert_raise Gem::Ext::BuildError do diff --git a/test/rubygems/test_gem_ext_ext_conf_builder.rb b/test/rubygems/test_gem_ext_ext_conf_builder.rb index bc383e5540a9..e14013899559 100644 --- a/test/rubygems/test_gem_ext_ext_conf_builder.rb +++ b/test/rubygems/test_gem_ext_ext_conf_builder.rb @@ -110,10 +110,12 @@ def test_class_build_extconf_fail assert_equal "extconf failed, exit code 1", error.message assert_match(/^#{Regexp.quote(Gem.ruby)}.* extconf.rb/, output[1]) - assert_match(File.join(@dest_path, "mkmf.log"), output[4]) - assert_includes(output, "To see why this extension failed to compile, please check the mkmf.log which can be found here:\n") + refute_includes(output, "To see why this extension failed to compile, please check the mkmf.log which can be found here:\n") - assert_path_exist File.join @dest_path, "mkmf.log" + # mkmf.log is left in the extension directory; deciding where it ends up is + # left to Gem::Ext::Builder#build_extension. + assert_path_exist File.join @ext, "mkmf.log" + assert_path_not_exist File.join @dest_path, "mkmf.log" end def test_class_build_extconf_success_without_warning @@ -133,7 +135,10 @@ def test_class_build_extconf_success_without_warning refute_includes(output, "To see why this extension failed to compile, please check the mkmf.log which can be found here:\n") - assert_path_exist File.join @dest_path, "mkmf.log" + # On a successful build, mkmf.log is cleaned up by "make clean" and is never + # copied into the install destination. + assert_path_not_exist File.join @ext, "mkmf.log" + assert_path_not_exist File.join @dest_path, "mkmf.log" end def test_class_build_unconventional diff --git a/test/rubygems/test_gem_specification.rb b/test/rubygems/test_gem_specification.rb index 79be0c996d23..a82abe5cb30f 100644 --- a/test/rubygems/test_gem_specification.rb +++ b/test/rubygems/test_gem_specification.rb @@ -1562,8 +1562,9 @@ def test_build_extensions_preview @ext.build_extensions + # A successful build no longer leaves gem_make.out in the install tree. gem_make_out = File.join @ext.extension_dir, "gem_make.out" - assert_path_exist gem_make_out + assert_path_not_exist gem_make_out end def test_contains_requirable_file_eh diff --git a/test/rubygems/test_gem_uninstaller.rb b/test/rubygems/test_gem_uninstaller.rb index 92ea01a3bca2..f4ce4e0e04e3 100644 --- a/test/rubygems/test_gem_uninstaller.rb +++ b/test/rubygems/test_gem_uninstaller.rb @@ -391,6 +391,39 @@ def test_uninstall_extension assert_path_not_exist @spec.extension_dir end + def test_uninstall_removes_build_info_logs + @spec.extensions << "extconf.rb" + write_file File.join(@tempdir, "extconf.rb") do |io| + io.write <<-RUBY +require 'mkmf' +create_makefile '#{@spec.name}' + RUBY + end + + @spec.files += %w[extconf.rb] + + use_ui @ui do + path = Gem::Package.build @spec + + installer = Gem::Installer.at path, force: true + installer.install + end + + # Build logs left behind in build_info by a previous failed build. + FileUtils.mkdir_p @spec.build_info_dir + mkmf_log = File.join @spec.build_info_dir, "#{@spec.full_name}.mkmf.log" + gem_make_out = File.join @spec.build_info_dir, "#{@spec.full_name}.gem_make.out" + FileUtils.touch mkmf_log + FileUtils.touch gem_make_out + + uninstaller = Gem::Uninstaller.new @spec.name, executables: true + uninstaller.uninstall + + assert_path_not_exist @spec.extension_dir + assert_path_not_exist mkmf_log + assert_path_not_exist gem_make_out + end + def test_uninstall_nonexistent uninstaller = Gem::Uninstaller.new "bogus", executables: true