[changelog skip] Prefer String#strip to #chomp (#1085)

When manipulating the results of a shell result, it's common to want to remove the newline. ``` puts `ruby -v`.inspect # => "ruby 2.7.2p137 (2020-10-01 revision 5445e04352) [x86_64-darwin19]\n" ``` This is commonly done by calling `chomp`. However there are some situations that are suspected to sometimes (but not always) contain two extra lines being tracked in heroku/hatchet#120 ## Chomp fails - https://dashboard.heroku.com/pipelines/ac057663-170b-4bdd-99d0-87560eb3a570/tests/1195 - https://dashboard.heroku.com/pipelines/ac057663-170b-4bdd-99d0-87560eb3a570/tests/1197 - Maybe there are multiple newlines in the original string. We can switch to strip instead of chomp. This PR replaces all uses of `chomp` that are intended to strip off trailing newlines with strip ```ruby puts "/hello\n\n".chomp.inspect # => "/hello\n" puts "/hello\n\n".strip.inspect # => "/hello" ``` It's not a perfect replacement, because `strip` also removes other whitespace characters and it modifies the beginning and end of the string. That being said, the ability to remove multiple newlines or any excess whitespace is the behavior we want.
heroku · Oct 7, 2020 · b3983a5 · b3983a5
1 parent 8b313b3
commit b3983a5
Show file tree

Hide file tree

Showing 9 changed files with 30 additions and 30 deletions.
diff --git a/lib/language_pack/helpers/binstub_check.rb b/lib/language_pack/helpers/binstub_check.rb
@@ -51,7 +51,7 @@ def call
       For example bin/#{@bad_binstubs.first.basename} has the shebang line:
 
       ```
-      #{@bad_binstubs.first.open(&:readline).chomp}
+      #{@bad_binstubs.first.open(&:readline).strip}
       ```
 
       It should be:

diff --git a/lib/language_pack/helpers/bundler_wrapper.rb b/lib/language_pack/helpers/bundler_wrapper.rb
@@ -152,7 +152,7 @@ def ruby_version
       if output.match(/No ruby version specified/)
         ""
       else
-        output.chomp.sub('(', '').sub(')', '').sub(/(p-?\d+)/, ' \1').split.join('-')
+        output.strip.sub('(', '').sub(')', '').sub(/(p-?\d+)/, ' \1').split.join('-')
       end
     end
   end

diff --git a/lib/language_pack/metadata.rb b/lib/language_pack/metadata.rb
@@ -21,7 +21,7 @@ def []=(key, value)
 
   def read(key)
     full_key = "#{FOLDER}/#{key}"
-    File.read(full_key).chomp if exists?(key)
+    File.read(full_key).strip if exists?(key)
   end
 
   def exists?(key)

diff --git a/lib/language_pack/rails41.rb b/lib/language_pack/rails41.rb
@@ -34,7 +34,7 @@ def app_secret
 
     @app_secret ||= begin
       if @metadata.exists?(key)
-        @metadata.read(key).chomp
+        @metadata.read(key).strip
       else
         secret = SecureRandom.hex(64)
         @metadata.write(key, secret)

diff --git a/lib/language_pack/ruby.rb b/lib/language_pack/ruby.rb
@@ -160,8 +160,8 @@ def build
       bundler.clean
       gem_layer.metadata[:gems] = Digest::SHA2.hexdigest(File.read("Gemfile.lock"))
       gem_layer.metadata[:stack] = @stack
-      gem_layer.metadata[:ruby_version] = run_stdout(%q(ruby -v)).chomp
-      gem_layer.metadata[:rubygems_version] = run_stdout(%q(gem -v)).chomp
+      gem_layer.metadata[:ruby_version] = run_stdout(%q(ruby -v)).strip
+      gem_layer.metadata[:rubygems_version] = run_stdout(%q(gem -v)).strip
       gem_layer.metadata[:buildpack_version] = BUILDPACK_VERSION
       gem_layer.write
 
@@ -217,7 +217,7 @@ def stack_not_14_not_16?
   end
 
   def warn_bundler_upgrade
-    old_bundler_version  = @metadata.read("bundler_version").chomp if @metadata.exists?("bundler_version")
+    old_bundler_version  = @metadata.read("bundler_version").strip if @metadata.exists?("bundler_version")
 
     if old_bundler_version && old_bundler_version != bundler.version
       warn(<<-WARNING, inline: true)
@@ -235,7 +235,7 @@ def warn_bundler_upgrade
   def self.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
+      out = run_no_pipe(command, user_env: true).strip
       error "Problem detecting bundler vendor directory: #{out}" unless $?.success?
       out
     end
@@ -275,7 +275,7 @@ def ruby_version
       new_app           = !File.exist?("vendor/heroku")
       last_version_file = "buildpack_ruby_version"
       last_version      = nil
-      last_version      = @metadata.read(last_version_file).chomp if @metadata.exists?(last_version_file)
+      last_version      = @metadata.read(last_version_file).strip if @metadata.exists?(last_version_file)
 
       @ruby_version = LanguagePack::RubyVersion.new(bundler.ruby_version,
         is_new:       new_app,
@@ -360,7 +360,7 @@ def setup_language_pack_environment(ruby_layer_path:, gem_layer_path:, bundle_pa
     instrument 'ruby.setup_language_pack_environment' do
       if ruby_version.jruby?
         ENV["PATH"] += ":bin"
-        ENV["JAVA_MEM"] = run(<<~SHELL).chomp
+        ENV["JAVA_MEM"] = run(<<~SHELL).strip
           #{set_jvm_max_heap}
           echo #{default_java_mem}
         SHELL
@@ -798,7 +798,7 @@ def load_default_cache
     instrument "ruby.load_default_cache" do
       if false # load_default_cache?
         puts "New app detected loading default bundler cache"
-        patchlevel = run("ruby -e 'puts RUBY_PATCHLEVEL'").chomp
+        patchlevel = run("ruby -e 'puts RUBY_PATCHLEVEL'").strip
         cache_name  = "#{LanguagePack::RubyVersion::DEFAULT_VERSION}-p#{patchlevel}-default-cache"
         @fetchers[:buildpack].fetch_untar("#{cache_name}.tgz")
       end
@@ -1017,7 +1017,7 @@ def post_bundler
   def syck_hack
     instrument "ruby.syck_hack" do
       syck_hack_file = File.expand_path(File.join(File.dirname(__FILE__), "../../vendor/syck_hack"))
-      rv             = run_stdout('ruby -e "puts RUBY_VERSION"').chomp
+      rv             = run_stdout('ruby -e "puts RUBY_VERSION"').strip
       # < 1.9.3 includes syck, so we need to use the syck hack
       if Gem::Version.new(rv) < Gem::Version.new("1.9.3")
         "-r#{syck_hack_file}"
@@ -1171,7 +1171,7 @@ def node_preinstall_bin_path
     return @node_preinstall_bin_path if defined?(@node_preinstall_bin_path)
 
     legacy_path = "#{Dir.pwd}/#{NODE_BP_PATH}"
-    path        = run("which node").chomp
+    path        = run("which node").strip
     if path && $?.success?
       @node_preinstall_bin_path = path
     elsif run("#{legacy_path}/node -v") && $?.success?
@@ -1195,7 +1195,7 @@ def yarn_preinstall_bin_path
   def yarn_preinstall_binary_path
     return @yarn_preinstall_binary_path if defined?(@yarn_preinstall_binary_path)
 
-    path = run("which yarn").chomp
+    path = run("which yarn").strip
     if path && $?.success?
       @yarn_preinstall_binary_path = path
     else
@@ -1252,8 +1252,8 @@ def bundler_cache
   end
 
   def valid_bundler_cache?(path, metadata)
-    full_ruby_version       = run_stdout(%q(ruby -v)).chomp
-    rubygems_version        = run_stdout(%q(gem -v)).chomp
+    full_ruby_version       = run_stdout(%q(ruby -v)).strip
+    rubygems_version        = run_stdout(%q(gem -v)).strip
     old_rubygems_version    = nil
 
     old_rubygems_version = metadata[:ruby_version]
@@ -1291,7 +1291,7 @@ def valid_bundler_cache?(path, metadata)
     # fix for https://github.com/heroku/heroku-buildpack-ruby/issues/86
     if (!metadata.include?(:rubygems_version) ||
         (old_rubygems_version == "2.0.0" && old_rubygems_version != rubygems_version)) &&
-      metadata.include?(:ruby_version) && metadata[:ruby_version].chomp.include?("ruby 2.0.0p0")
+      metadata.include?(:ruby_version) && metadata[:ruby_version].strip.include?("ruby 2.0.0p0")
       return [false, "Updating to rubygems #{rubygems_version}. Clearing bundler cache."]
     end
 
@@ -1329,8 +1329,8 @@ def load_bundler_cache
     instrument "ruby.load_bundler_cache" do
       cache.load "vendor"
 
-      full_ruby_version       = run_stdout(%q(ruby -v)).chomp
-      rubygems_version        = run_stdout(%q(gem -v)).chomp
+      full_ruby_version       = run_stdout(%q(ruby -v)).strip
+      rubygems_version        = run_stdout(%q(gem -v)).strip
       heroku_metadata         = "vendor/heroku"
       old_rubygems_version    = nil
       ruby_version_cache      = "ruby_version"
@@ -1342,8 +1342,8 @@ def load_bundler_cache
       # bundle clean does not remove binstubs
       FileUtils.rm_rf("vendor/bundler/bin")
 
-      old_rubygems_version = @metadata.read(ruby_version_cache).chomp if @metadata.exists?(ruby_version_cache)
-      old_stack = @metadata.read(stack_cache).chomp if @metadata.exists?(stack_cache)
+      old_rubygems_version = @metadata.read(ruby_version_cache).strip if @metadata.exists?(ruby_version_cache)
+      old_stack = @metadata.read(stack_cache).strip if @metadata.exists?(stack_cache)
       old_stack ||= DEFAULT_LEGACY_STACK
 
       stack_change  = old_stack != @stack
@@ -1366,9 +1366,9 @@ def load_bundler_cache
       elsif !@metadata.include?(buildpack_version_cache) && @metadata.exists?(ruby_version_cache)
         puts "Broken cache detected. Purging build cache."
         purge_bundler_cache
-      elsif (@bundler_cache.exists? || @bundler_cache.old?) && @metadata.exists?(ruby_version_cache) && full_ruby_version != @metadata.read(ruby_version_cache).chomp
+      elsif (@bundler_cache.exists? || @bundler_cache.old?) && @metadata.exists?(ruby_version_cache) && full_ruby_version != @metadata.read(ruby_version_cache).strip
         puts "Ruby version change detected. Clearing bundler cache."
-        puts "Old: #{@metadata.read(ruby_version_cache).chomp}"
+        puts "Old: #{@metadata.read(ruby_version_cache).strip}"
         puts "New: #{full_ruby_version}"
         purge_bundler_cache
       end
@@ -1382,7 +1382,7 @@ def load_bundler_cache
       # fix for https://github.com/heroku/heroku-buildpack-ruby/issues/86
       if (!@metadata.exists?(rubygems_version_cache) ||
           (old_rubygems_version == "2.0.0" && old_rubygems_version != rubygems_version)) &&
-          @metadata.exists?(ruby_version_cache) && @metadata.read(ruby_version_cache).chomp.include?("ruby 2.0.0p0")
+          @metadata.exists?(ruby_version_cache) && @metadata.read(ruby_version_cache).strip.include?("ruby 2.0.0p0")
         puts "Updating to rubygems #{rubygems_version}. Clearing bundler cache."
         purge_bundler_cache
       end
@@ -1403,7 +1403,7 @@ def load_bundler_cache
 
       # recompile gems for libyaml 0.1.7 update
       if @metadata.exists?(buildpack_version_cache) && (bv = @metadata.read(buildpack_version_cache).sub('v', '').to_i) && bv != 0 && bv <= 147 &&
-          (@metadata.exists?(ruby_version_cache) && @metadata.read(ruby_version_cache).chomp.match(/ruby 2\.1\.(9|10)/) ||
+          (@metadata.exists?(ruby_version_cache) && @metadata.read(ruby_version_cache).strip.match(/ruby 2\.1\.(9|10)/) ||
            bundler.has_gem?("psych")
           )
         puts "Need to recompile gems for CVE-2014-2014-9130. Clearing bundler cache."

diff --git a/lib/language_pack/test/rails2.rb b/lib/language_pack/test/rails2.rb
@@ -61,7 +61,7 @@ def db_prepare_test_rake_tasks
 
     if schema_load.not_defined? && structure_load.not_defined?
       result = detect_schema_format
-      case result.lines.last.chomp
+      case result.lines.last.strip
       when "ruby"
         schema_load    = rake.task("db:schema:load")
       when "sql" # currently not a possible edge case, we think

diff --git a/spec/cnb/basic_local_pack_spec.rb b/spec/cnb/basic_local_pack_spec.rb
@@ -27,13 +27,13 @@ def teardown
     return unless image_name
     repo_name, tag_name = image_name.split(":")
 
-    docker_list = `docker images --no-trunc | grep #{repo_name} | grep #{tag_name}`.chomp
+    docker_list = `docker images --no-trunc | grep #{repo_name} | grep #{tag_name}`.strip
     run_local!("docker rmi #{image_name} --force") if !docker_list.empty?
     @image_name = nil
   end
 
   def run(cmd)
-    `docker run #{image_name} '#{cmd}'`.chomp
+    `docker run #{image_name} '#{cmd}'`.strip
   end
 
   def run!(cmd)

diff --git a/spec/hatchet/ruby_spec.rb b/spec/hatchet/ruby_spec.rb
@@ -118,7 +118,7 @@
       Hatchet::Runner.new('cd_ruby', stack: DEFAULT_STACK, buildpacks: buildpacks, config: config).deploy do |app|
         expect(app.output).to match("cd version ruby 2.5.1")
 
-        expect(app.run("which ruby").chomp).to eq("/app/bin/ruby")
+        expect(app.run("which ruby").strip).to eq("/app/bin/ruby")
       end
     end
   end

diff --git a/spec/helpers/binstub_check_spec.rb b/spec/helpers/binstub_check_spec.rb
@@ -2,7 +2,7 @@
 
 describe LanguagePack::Helpers::BinstubCheck do
   def get_ruby_path!
-    out = `which ruby`.chomp
+    out = `which ruby`.strip
     raise "command `which ruby` failed with output: #{out}" unless $?.success?
 
     return Pathname.new(out)