From e8e686fae4d63b2cae5a863040975242d89d6e56 Mon Sep 17 00:00:00 2001 From: schneems Date: Tue, 29 Sep 2020 13:57:59 -0500 Subject: [PATCH 1/6] Upgrade Hatchet --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 8480cd130..133bca93c 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -11,7 +11,7 @@ GEM excon moneta multi_json (>= 1.9.2) - heroku_hatchet (7.1.3) + heroku_hatchet (7.3.0) excon (~> 0) platform-api (~> 3) rrrretry (~> 1) From 2860b74b0d98520ee67053aa4533e7c71df9fc6b Mon Sep 17 00:00:00 2001 From: schneems Date: Tue, 29 Sep 2020 13:58:04 -0500 Subject: [PATCH 2/6] Failing test --- spec/hatchet/ci_spec.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/spec/hatchet/ci_spec.rb b/spec/hatchet/ci_spec.rb index 775e7fb40..473ab270b 100644 --- a/spec/hatchet/ci_spec.rb +++ b/spec/hatchet/ci_spec.rb @@ -4,6 +4,9 @@ it "Does not cause the double ruby rainbow bug" do Hatchet::Runner.new("heroku-ci-json-example").run_ci do |test_run| expect(test_run.status).to eq(:succeeded) + + install_bundler_count = test_run.output.scan("Installing bundler").count + expect(install_bundler_count).to eq(1), "Expected output to only install bundler once but was found #{install_bundler_count} times. output:\n#{test_run.output}" end end From 4769014b98ef4a777b6827948089f19a0fa85b61 Mon Sep 17 00:00:00 2001 From: schneems Date: Tue, 29 Sep 2020 14:01:51 -0500 Subject: [PATCH 3/6] Memoize slug_vendor_base class method call Shelling out is comparatively expensive and this gets called several times. It's already memoized at the instance level (which calls the class). This commit memoizes the class call as well. --- lib/language_pack/ruby.rb | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/language_pack/ruby.rb b/lib/language_pack/ruby.rb index 0583c0a0d..7551c970b 100644 --- a/lib/language_pack/ruby.rb +++ b/lib/language_pack/ruby.rb @@ -231,13 +231,14 @@ def warn_bundler_upgrade end end - # For example "vendor/bundle/ruby/2.6.0" def self.slug_vendor_base - command = %q(ruby -e "require 'rbconfig';puts \"vendor/bundle/#{RUBY_ENGINE}/#{RbConfig::CONFIG['ruby_version']}\"") - slug_vendor_base = run_no_pipe(command, user_env: true).chomp - error "Problem detecting bundler vendor directory: #{@slug_vendor_base}" unless $?.success? - return slug_vendor_base + @slug_vendor_base ||= begin + command = %q(ruby -e "require 'rbconfig';puts \"vendor/bundle/#{RUBY_ENGINE}/#{RbConfig::CONFIG['ruby_version']}\"") + out = run_no_pipe(command, user_env: true).chomp + error "Problem detecting bundler vendor directory: #{out}" unless $?.success? + out + end end # the relative path to the bundler directory of gems From 5050ed7f9b21f4e33a79904ed93a0dea44412ec9 Mon Sep 17 00:00:00 2001 From: schneems Date: Tue, 29 Sep 2020 15:34:33 -0500 Subject: [PATCH 4/6] Allow calling puts on non-strings When debugging its common to do something like: ```ruby puts variable ``` And unless it's a string this method will trigger an exception: ``` NoMethodError (undefined method `each_line' for []:Array) ``` This commit ensures all variables passed in to puts will be strings before calling `each_line` on them. --- lib/language_pack/shell_helpers.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/language_pack/shell_helpers.rb b/lib/language_pack/shell_helpers.rb index a7c4f2c09..00fe81dac 100644 --- a/lib/language_pack/shell_helpers.rb +++ b/lib/language_pack/shell_helpers.rb @@ -243,7 +243,7 @@ def topic(message) # (indented by 6 spaces) # @param [String] message to be displayed def puts(message) - message.each_line do |line| + message.to_s.each_line do |line| if line.end_with?("\n".freeze) print " #{line}" else From 08fb614f16b0f4fba07d7a7731920688350a39f0 Mon Sep 17 00:00:00 2001 From: schneems Date: Tue, 29 Sep 2020 16:02:24 -0500 Subject: [PATCH 5/6] [close #1072] Don't install bundler twice on CI When we compile an app for CI, if the user does not specify a test command than the bin/support/ruby_test gets called. This file automatically detects the test framework the user is likely using and calls the appropriate command (such as `bin/rspec`). The problem is that the logic in this file relies on determining what gems the customer's application has. To detect gems we need access to the bundler's source code because we use bundler internals for parsing the Gemfile.lock. A copy of bundler is already installed on the system from the prior `bin/test-compile` call, but...it's located on disk at the "slug_vendor_base" which is based off of the customer's ruby version, for example: ``` vendor/bundle/ruby/2.6.0 ``` So in order to detect gems, we need access to bundler, to access bundler we need to put the customer's ruby binary on the PATH. This was previously being done here: ``` # The `ruby_test-compile` program installs a version of Ruby for the # user's application. It needs the propper `PATH`, where ever Ruby is installed # we always add a symlink to the `bin/ruby` file so that is always valid. # We calculate the gem path the same way we do when compiling. LanguagePack::ShellHelpers.user_env_hash["PATH"] = "#{build_dir}/bin:#{bundler.bundler_path}/bin:#{ENV["PATH"]}" ``` I moved adding the users `app/bin` to the PATH up to the top of the script. Now that requirement is set, I worked on getting rid of the double bundler installation. In this line: ``` bundler.install ``` It will check to see if a copy of bundler is already installed, the problem is: due to the circular dependency on using bundler to find a customer's Ruby version and then using Ruby to determine where in the customer's app bundler will live, this defaults to a tmp directory. In regular `bin/compile` this is great, we then later move the bundler to the right spot. In this case, we can just tell it the location of where we previously installed bundler in the app: ```ruby bundler = LanguagePack::Helpers::BundlerWrapper.new( gemfile_path: "#{build_dir}/Gemfile", bundler_path: LanguagePack::Ruby.slug_vendor_base # This was previously installed by bin/support/ruby_test-compile ) ``` > In the `slug_vendor_base` With this change bundler checks that directory, sees its non-empty and then skips downloading bundler for a second time. ## Retro This logic is very difficult to reason about. We could make it simpler by removing the requirement that the customer's Ruby version is used to determine the directory of where bundler lives. I don't know what tight coupling might exist between this file location and other actions. For instance when we bundle install we specify a path of `vendor/bundle` and the path we install bundler into is in `vendor/bundle/ruby/` For a first attempt I would put it in a completely separate directory. Other buildpacks use `.heroku` we could put it in `.heroku/ruby/bundler` and then we wouldn't need to calculate the location. If that raises coupling issues with other parts of this project (likely) then I could iterate again. --- bin/support/ruby_test | 30 +++++++++++++------- lib/language_pack/helpers/bundler_wrapper.rb | 8 ++++++ 2 files changed, 28 insertions(+), 10 deletions(-) diff --git a/bin/support/ruby_test b/bin/support/ruby_test index 977e9b8e1..6ecdab8f5 100755 --- a/bin/support/ruby_test +++ b/bin/support/ruby_test @@ -29,26 +29,36 @@ def execute_command(command) pipe(command, :user_env => true, :output_object => Kernel) end +def user_env_hash + LanguagePack::ShellHelpers.user_env_hash +end + # $ bin/test BUILD_DIR ENV_DIR ARTIFACT_DIR build_dir, env_dir, _ = ARGV LanguagePack::ShellHelpers.initialize_env(env_dir) +Dir.chdir(build_dir) + +# The `ruby_test-compile` program installs a version of Ruby for the +# user's application. It needs the propper `PATH`, where ever Ruby is installed +# otherwise we end up using the buildpack's version of Ruby +# +# This is needed here because LanguagePack::Ruby.slug_vendor_base shells out to the user's ruby binary +user_env_hash["PATH"] = "#{build_dir}/bin:#{ENV["PATH"]}" bundler = LanguagePack::Helpers::BundlerWrapper.new( - gemfile_path: "#{build_dir}/Gemfile" + gemfile_path: "#{build_dir}/Gemfile", + bundler_path: LanguagePack::Ruby.slug_vendor_base # This was previously installed by bin/support/ruby_test-compile ) -# The `ruby_test-compile` program installs a version of Ruby for the -# user's application. It needs the propper `PATH`, where ever Ruby is installed -# we always add a symlink to the `bin/ruby` file so that is always valid. -# We calculate the gem path the same way we do when compiling. -LanguagePack::ShellHelpers.user_env_hash["PATH"] = "#{build_dir}/bin:#{bundler.bundler_path}/bin:#{ENV["PATH"]}" -LanguagePack::ShellHelpers.user_env_hash["GEM_PATH"] = LanguagePack::Ruby.slug_vendor_base +# - Add bundler's bin directory to the PATH +# - Always make sure `$HOME/bin` is first on the path +user_env_hash["PATH"] = "#{build_dir}/bin:#{bundler.bundler_path}/bin:#{user_env_hash["PATH"]}" +user_env_hash["GEM_PATH"] = LanguagePack::Ruby.slug_vendor_base -# load bundler +# - Sets BUNDLE_GEMFILE +# - Loads bundler's internal Gemfile.lock parser so we can use `bundler.has_gem?` bundler.install -Dir.chdir(build_dir) - execute_test( if bundler.has_gem?("rspec-core") if File.exist?("bin/rspec") diff --git a/lib/language_pack/helpers/bundler_wrapper.rb b/lib/language_pack/helpers/bundler_wrapper.rb index 9ca42c9ba..364f7c2c9 100644 --- a/lib/language_pack/helpers/bundler_wrapper.rb +++ b/lib/language_pack/helpers/bundler_wrapper.rb @@ -176,6 +176,14 @@ def fetch_bundler topic("Installing bundler #{@version}") bundler_version_escape_valve! + # Install directory structure (as of Bundler 2.1.4): + # - cache + # - bin + # - gems + # - specifications + # - build_info + # - extensions + # - doc FileUtils.mkdir_p(bundler_path) Dir.chdir(bundler_path) do @fetcher.fetch_untar(@bundler_tar) From 609ff762187c30a37ce51f23febfe3259ea271b9 Mon Sep 17 00:00:00 2001 From: schneems Date: Tue, 29 Sep 2020 16:03:42 -0500 Subject: [PATCH 6/6] Fix hatchet deprecation ``` ENV HATCHET_BUILDPACK_BASE is not set. It currently defaults to the ruby buildpack. In the future this env var will be required ``` --- CHANGELOG.md | 1 + spec/spec_helper.rb | 2 ++ 2 files changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e794a512c..5cf9ae712 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ # Main (unreleased) +* Fix double installation of bundler on CI runs when no test script is specified (https://github.com/heroku/heroku-buildpack-ruby/pull/1073) * Bundler 2.x is now 2.1.4 (https://github.com/heroku/heroku-buildpack-ruby/pull/1052) * Persistent bundler config is now being set using the `BUNDLE_*` env vars (https://github.com/heroku/heroku-buildpack-ruby/pull/1039) * Rake task "assets:clean" will not get called if it does not exist (https://github.com/heroku/heroku-buildpack-ruby/pull/1018) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 22688d3ac..21007b31b 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -7,6 +7,8 @@ require 'language_pack' require 'language_pack/shell_helpers' +ENV["HATCHET_BUILDPACK_BASE"] = "https://github.com/heroku/heroku-buildpack-ruby" + ENV['RACK_ENV'] = 'test' DEFAULT_STACK = 'heroku-18'