Auto merge of #4298 - njam:preserve-path, r=indirect

Refactor path/environment preserver, make it always preserve the PATH

Closes #4251

@segiddins @indirect @RochesterinNYC wdyt?

lib/bundler.rb

@@ -3,7 +3,7 @@
 require "pathname"
 require "rbconfig"
 require "thread"
-require "bundler/path_preserver"
+require "bundler/environment_preserver"
 require "bundler/gem_remote_fetcher"
 require "bundler/rubygems_ext"
 require "bundler/rubygems_integration"
@@ -13,9 +13,9 @@
 require "bundler/errors"
 
 module Bundler
-  PathPreserver.preserve_path_in_environment("PATH", ENV)
-  PathPreserver.preserve_path_in_environment("GEM_PATH", ENV)
-  ORIGINAL_ENV = ENV.to_hash
+  environment_preserver = EnvironmentPreserver.new(ENV, %w(PATH GEM_PATH))
+  ORIGINAL_ENV = environment_preserver.restore
+  ENV.replace(environment_preserver.backup)
   SUDO_MUTEX = Mutex.new
 
   autoload :Definition,             "bundler/definition"

lib/bundler/environment_preserver.rb

@@ -0,0 +1,35 @@
+# frozen_string_literal: true
+module Bundler
+  class EnvironmentPreserver
+    # @param env [ENV]
+    # @param keys [Array<String>]
+    def initialize(env, keys)
+      @original = env.to_hash
+      @keys = keys
+      @prefix = "BUNDLE_ORIG_"
+    end
+
+    # @return [Hash]
+    def backup
+      env = restore.clone
+      @keys.each do |key|
+        value = env[key]
+        env[@prefix + key] = value unless value.nil? || value.empty?
+      end
+      env
+    end
+
+    # @return [Hash]
+    def restore
+      env = @original.clone
+      @keys.each do |key|
+        value_original = env[@prefix + key]
+        unless value_original.nil? || value_original.empty?
+          env[key] = value_original
+          env.delete(@prefix + key)
+        end
+      end
+      env
+    end
+  end
+end

spec/bundler/cli_spec.rb

@@ -18,7 +18,7 @@
       f.puts "#!/usr/bin/env ruby\nputs 'Hello, world'\n"
     end
 
-    with_path_as(tmp) do
+    with_path_added(tmp) do
       bundle "testtasks"
     end
 

spec/bundler/environment_preserver_spec.rb

@@ -0,0 +1,68 @@
+# frozen_string_literal: true
+require "spec_helper"
+
+describe Bundler::EnvironmentPreserver do
+  let(:preserver) { described_class.new(env, ["foo"]) }
+
+  describe "#backup" do
+    let(:env) { { "foo" => "my-foo", "bar" => "my-bar" } }
+    subject { preserver.backup }
+
+    it "should create backup entries" do
+      expect(subject["BUNDLE_ORIG_foo"]).to eq("my-foo")
+    end
+
+    it "should keep the original entry" do
+      expect(subject["foo"]).to eq("my-foo")
+    end
+
+    it "should not create backup entries for unspecified keys" do
+      expect(subject.key?("BUNDLE_ORIG_bar")).to eq(false)
+    end
+
+    it "should not affect the original env" do
+      subject
+      expect(env.keys.sort).to eq(%w(bar foo))
+    end
+
+    context "when a key is empty" do
+      let(:env) { { "foo" => "" } }
+
+      it "should not create backup entries" do
+        expect(subject.key?("BUNDLE_ORIG_foo")).to eq(false)
+      end
+    end
+  end
+
+  describe "#restore" do
+    subject { preserver.restore }
+
+    context "when an original key is set" do
+      let(:env) { { "foo" => "my-foo", "BUNDLE_ORIG_foo" => "orig-foo" } }
+
+      it "should restore the original value" do
+        expect(subject["foo"]).to eq("orig-foo")
+      end
+
+      it "should delete the backup value" do
+        expect(subject.key?("BUNDLE_ORIG_foo")).to eq(false)
+      end
+    end
+
+    context "when no original key is set" do
+      let(:env) { { "foo" => "my-foo" } }
+
+      it "should keep the current value" do
+        expect(subject["foo"]).to eq("my-foo")
+      end
+    end
+
+    context "when the original key is empty" do
+      let(:env) { { "foo" => "my-foo", "BUNDLE_ORIG_foo" => "" } }
+
+      it "should keep the current value" do
+        expect(subject["foo"]).to eq("my-foo")
+      end
+    end
+  end
+end

spec/commands/exec_spec.rb

@@ -103,8 +103,9 @@
       f.puts "echo foobar"
     end
     File.chmod(0744, "--verbose")
-    ENV["PATH"] = "."
-    bundle "exec -- --verbose"
+    with_path_as(".") do
+      bundle "exec -- --verbose"
+    end
     expect(out).to eq("foobar")
   end
 
@@ -358,7 +359,7 @@ def bin_path(a,b,c)
       Bundler.rubygems.extend(Monkey)
       G
       bundle "install --deployment"
-      bundle "exec ruby -e '`bundler -v`; puts $?.success?'"
+      bundle "exec ruby -e '`../../exe/bundler -v`; puts $?.success?'"
       expect(out).to match("true")
     end
   end

spec/commands/help_spec.rb

@@ -13,23 +13,23 @@
   end
 
   it "uses mann when available" do
-    fake_man!
-
-    bundle "help gemfile"
+    with_fake_man do
+      bundle "help gemfile"
+    end
     expect(out).to eq(%(["#{root}/lib/bundler/man/gemfile.5"]))
   end
 
   it "prefixes bundle commands with bundle- when finding the groff files" do
-    fake_man!
-
-    bundle "help install"
+    with_fake_man do
+      bundle "help install"
+    end
     expect(out).to eq(%(["#{root}/lib/bundler/man/bundle-install"]))
   end
 
   it "simply outputs the txt file when there is no man on the path" do
-    kill_path!
-
-    bundle "help install", :expect_err => true
+    with_path_as("") do
+      bundle "help install", :expect_err => true
+    end
     expect(out).to match(/BUNDLE-INSTALL/)
   end
 
@@ -43,7 +43,7 @@
       f.puts "#!/usr/bin/env ruby\nputs ARGV.join(' ')\n"
     end
 
-    with_path_as(tmp) do
+    with_path_added(tmp) do
       bundle "help testtasks"
     end
 
@@ -52,37 +52,37 @@
   end
 
   it "is called when the --help flag is used after the command" do
-    fake_man!
-
-    bundle "install --help"
+    with_fake_man do
+      bundle "install --help"
+    end
     expect(out).to eq(%(["#{root}/lib/bundler/man/bundle-install"]))
   end
 
   it "is called when the --help flag is used before the command" do
-    fake_man!
-
-    bundle "--help install"
+    with_fake_man do
+      bundle "--help install"
+    end
     expect(out).to eq(%(["#{root}/lib/bundler/man/bundle-install"]))
   end
 
   it "is called when the -h flag is used before the command" do
-    fake_man!
-
-    bundle "-h install"
+    with_fake_man do
+      bundle "-h install"
+    end
     expect(out).to eq(%(["#{root}/lib/bundler/man/bundle-install"]))
   end
 
   it "is called when the -h flag is used after the command" do
-    fake_man!
-
-    bundle "install -h"
+    with_fake_man do
+      bundle "install -h"
+    end
     expect(out).to eq(%(["#{root}/lib/bundler/man/bundle-install"]))
   end
 
   it "has helpful output when using --help flag for a non-existent command" do
-    fake_man!
-
-    bundle "instill -h", :expect_err => true
+    with_fake_man do
+      bundle "instill -h", :expect_err => true
+    end
     expect(err).to include('Could not find command "instill -h --no-color".')
   end
 end

spec/install/gemfile/git_spec.rb

@@ -977,7 +977,9 @@
         end
       G
 
-      bundle "update", :env => { "PATH" => "", "_ORIGINAL_PATH" => "" }
+      with_path_as("") do
+        bundle "update"
+      end
       expect(out).to include("You need to install git to be able to use gems from git repositories. For help installing git, please refer to GitHub's tutorial at https://help.github.com/articles/set-up-git")
     end
 

spec/runtime/with_clean_env_spec.rb

@@ -89,6 +89,21 @@
         expect(`echo $BUNDLE_PATH`.strip).to eq("./Gemfile")
       end
     end
+
+    it "should preserve the PATH environment variable" do
+      gemfile ""
+      bundle "install --path vendor/bundle"
+
+      code = "Bundler.with_original_env do;" \
+             "  print ENV['PATH'];" \
+             "end"
+
+      path = `getconf PATH`.strip
+      with_path_as(path) do
+        result = bundle("exec ruby -e #{code.inspect}")
+        expect(result).to eq(path)
+      end
+    end
   end
 
   describe "Bundler.clean_system" do

spec/spec_helper.rb

@@ -80,13 +80,8 @@
   config.filter_run :focused => true unless ENV["CI"]
   config.run_all_when_everything_filtered = true
 
-  original_wd            = Dir.pwd
-  original_path          = ENV["PATH"]
-  original_gem_home      = ENV["GEM_HOME"]
-  original_ruby_opt      = ENV["RUBYOPT"]
-  original_ruby_lib      = ENV["RUBYLIB"]
-  original_git_dir       = ENV["GIT_DIR"]
-  original_git_work_tree = ENV["GIT_WORK_TREE"]
+  original_wd  = Dir.pwd
+  original_env = ENV.to_hash
 
   def pending_jruby_shebang_fix
     pending "JRuby executables do not have a proper shebang" if RUBY_PLATFORM == "java"
@@ -110,20 +105,6 @@ def pending_jruby_shebang_fix
     puts @out if defined?(@out) && example.exception
 
     Dir.chdir(original_wd)
-    # Reset ENV
-    ENV["PATH"]                  = original_path
-    ENV["GEM_HOME"]              = original_gem_home
-    ENV["GEM_PATH"]              = original_gem_home
-    ENV["RUBYOPT"]               = original_ruby_opt
-    ENV["RUBYLIB"]               = original_ruby_lib
-    ENV["GIT_DIR"]               = original_git_dir
-    ENV["GIT_WORK_TREE"]         = original_git_work_tree
-    ENV["BUNDLE_PATH"]           = nil
-    ENV["BUNDLE_GEMFILE"]        = nil
-    ENV["BUNDLE_FROZEN"]         = nil
-    ENV["BUNDLE_APP_CONFIG"]     = nil
-    ENV["BUNDLER_TEST"]          = nil
-    ENV["BUNDLER_SPEC_PLATFORM"] = nil
-    ENV["BUNDLER_SPEC_VERSION"]  = nil
+    ENV.replace(original_env)
   end
 end