Merge #6843

6843: Improve `clean_env` deprecation path r=deivid-rodriguez a=deivid-rodriguez

### What was the end-user problem that led to this PR?

The problem was that I noticed that there's still some use cases where the current implementation of `Bundler.with_clean_env` can be useful. Whereas the most common use case is to go back to the original environment before bundler was loaded, sometimes you actually want a fully "unbundled" environment. For example, I wanted to test a rails template in isolation (`rails new my_app -m my_template.rb`) in a library where we specifically set `BUNDLE_GEMFILE` in CI. In that context, `with_original_env` won't do the trick for me, because to properly test my template i need to make sure no bundler environment is set at all.

### What was your diagnosis of the problem?

My diagnosis was that we should probably undeprecate `(with_)clean_env`. But @indirect suggested that we should still deprecate the method name, because it's confusing what each developer understands by "clean". That's a very good point.

### What is your fix for the problem, implemented in this PR?

My fix is to move the functionality of `(with_)clean_env` to `(with_)unbundled_env`, and deprecate `(with_)clean_env`.

In addition, I noticed that the current deprecation message for `with_clean_env` actually mentioned the method `clean_env`, which is not a method the end user is using directly. So I changed the deprecation messages to suggest the proper alternative for each case (suggest `unbundled_env` as a replacement for `clean_env`; and `with_unbundled_env` as a replacement for `with_clean_env`).

### Why did you choose this fix out of the possible options?

I chose this fix because it undeprecates the functionality previously provided by `clean_env`, while still deprecating the `clean_env` name because of being confusing.


Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>

3 files changed, 114 insertions, 30 deletions

diff --git a/lib/bundler.rb b/lib/bundler.rb
index 1cb3b4fb21..137d916cc6 100644
--- a/lib/bundler.rb
+++ b/lib/bundler.rb
@@ -280,10 +280,19 @@ EOF
       ORIGINAL_ENV.clone
     end
 
-    # @deprecated Use `original_env` instead
-    # @return [Hash] Environment with all bundler-related variables removed
+    # @deprecated Use `unbundled_env` instead
     def clean_env
-      Bundler::SharedHelpers.major_deprecation(2, "`Bundler.clean_env` has weird edge cases, use `.original_env` instead")
+      Bundler::SharedHelpers.major_deprecation(
+        2,
+        "`Bundler.clean_env` has been deprecated in favor of `Bundler.unbundled_env`. " \
+        "If you instead want the environment before bundler was originally loaded, use `Bundler.original_env`"
+      )
+
+      unbundled_env
+    end
+
+    # @return [Hash] Environment with all bundler-related variables removed
+    def unbundled_env
       env = original_env
 
       if env.key?("BUNDLER_ORIG_MANPATH")
@@ -305,12 +314,25 @@ EOF
       env
     end
 
+    # Run block with environment present before Bundler was activated
     def with_original_env
       with_env(original_env) { yield }
     end
 
+    # @deprecated Use `with_unbundled_env` instead
     def with_clean_env
-      with_env(clean_env) { yield }
+      Bundler::SharedHelpers.major_deprecation(
+        2,
+        "`Bundler.with_clean_env` has been deprecated in favor of `Bundler.with_unbundled_env`. " \
+        "If you instead want the environment before bundler was originally loaded, use `Bundler.with_original_env`"
+      )
+
+      with_env(unbundled_env) { yield }
+    end
+
+    # Run block with all bundler-related variables removed
+    def with_unbundled_env
+      with_env(unbundled_env) { yield }
     end
 
     def clean_system(*args)
diff --git a/spec/other/major_deprecation_spec.rb b/spec/other/major_deprecation_spec.rb
index fba177b497..ce2735ba94 100644
--- a/spec/other/major_deprecation_spec.rb
+++ b/spec/other/major_deprecation_spec.rb
@@ -41,10 +41,12 @@ RSpec.describe "major deprecations", :bundler => "< 2" do
 
   describe "Bundler" do
     describe ".clean_env" do
-      it "is deprecated in favor of .original_env" do
+      it "is deprecated in favor of .unbundled_env" do
         source = "Bundler.clean_env"
         bundle "exec ruby -e #{source.dump}"
-        expect(warnings).to have_major_deprecation "`Bundler.clean_env` has weird edge cases, use `.original_env` instead"
+        expect(warnings).to have_major_deprecation \
+          "`Bundler.clean_env` has been deprecated in favor of `Bundler.unbundled_env`. " \
+          "If you instead want the environment before bundler was originally loaded, use `Bundler.original_env`"
       end
     end
 
diff --git a/spec/runtime/with_clean_env_spec.rb b/spec/runtime/with_unbundled_env_spec.rb
index 321f5b6415..83eb1eac13 100644
--- a/spec/runtime/with_clean_env_spec.rb
+++ b/spec/runtime/with_unbundled_env_spec.rb
@@ -2,6 +2,7 @@
 
 RSpec.describe "Bundler.with_env helpers" do
   def bundle_exec_ruby!(code, *args)
+    build_bundler_context
     opts = args.last.is_a?(Hash) ? args.pop : {}
     env = opts[:env] ||= {}
     env[:RUBYOPT] ||= "-r#{spec_dir.join("support/hax")}"
@@ -9,13 +10,13 @@ RSpec.describe "Bundler.with_env helpers" do
     bundle! "exec '#{Gem.ruby}' -e #{code}", *args
   end
 
-  describe "Bundler.original_env" do
-    before do
-      bundle "config path vendor/bundle"
-      gemfile ""
-      bundle "install"
-    end
+  def build_bundler_context
+    bundle "config path vendor/bundle"
+    gemfile ""
+    bundle "install"
+  end
 
+  describe "Bundler.original_env" do
     it "should return the PATH present before bundle was activated" do
       code = "print Bundler.original_env['PATH']"
       path = `getconf PATH`.strip + "#{File::PATH_SEPARATOR}/foo"
@@ -46,6 +47,7 @@ RSpec.describe "Bundler.with_env helpers" do
       RB
       path = `getconf PATH`.strip + File::PATH_SEPARATOR + File.dirname(Gem.ruby)
       with_path_as(path) do
+        build_bundler_context
         bundle! "exec '#{Gem.ruby}' #{bundled_app("exe.rb")} 2", :env => { :RUBYOPT => "-r#{spec_dir.join("support/hax")}" }
       end
       expect(err).to eq <<-EOS.strip
@@ -58,45 +60,69 @@ RSpec.describe "Bundler.with_env helpers" do
     it "removes variables that bundler added", :ruby_repo do
       original = ruby!('puts ENV.to_a.map {|e| e.join("=") }.sort.join("\n")', :env => { :RUBYOPT => "-r#{spec_dir.join("support/hax")}" })
       code = 'puts Bundler.original_env.to_a.map {|e| e.join("=") }.sort.join("\n")'
-      bundle! "exec '#{Gem.ruby}' -e #{code.dump}", :env => { :RUBYOPT => "-r#{spec_dir.join("support/hax")}" }
+      bundle_exec_ruby! code.dump
       expect(out).to eq original
     end
   end
 
-  describe "Bundler.clean_env", :bundler => "< 2" do
-    before do
-      bundle "config path vendor/bundle"
-      gemfile ""
-      bundle "install"
-    end
-
+  shared_examples_for "an unbundling helper" do
     it "should delete BUNDLE_PATH" do
-      code = "print Bundler.clean_env.has_key?('BUNDLE_PATH')"
+      code = "print #{modified_env}.has_key?('BUNDLE_PATH')"
       ENV["BUNDLE_PATH"] = "./foo"
       bundle_exec_ruby! code.dump
-      expect(last_command.stdboth).to eq "false"
+      expect(last_command.stdboth).to include "false"
     end
 
     it "should remove '-rbundler/setup' from RUBYOPT" do
-      code = "print Bundler.clean_env['RUBYOPT']"
+      code = "print #{modified_env}['RUBYOPT']"
       ENV["RUBYOPT"] = "-W2 -rbundler/setup"
       bundle_exec_ruby! code.dump
       expect(last_command.stdboth).not_to include("-rbundler/setup")
     end
 
     it "should clean up RUBYLIB", :ruby_repo do
-      code = "print Bundler.clean_env['RUBYLIB']"
+      code = "print #{modified_env}['RUBYLIB']"
       ENV["RUBYLIB"] = root.join("lib").to_s + File::PATH_SEPARATOR + "/foo"
       bundle_exec_ruby! code.dump
-      expect(last_command.stdboth).to eq("/foo")
+      expect(last_command.stdboth).to include("/foo")
     end
 
     it "should restore the original MANPATH" do
-      code = "print Bundler.clean_env['MANPATH']"
+      code = "print #{modified_env}['MANPATH']"
       ENV["MANPATH"] = "/foo"
       ENV["BUNDLER_ORIG_MANPATH"] = "/foo-original"
       bundle_exec_ruby! code.dump
-      expect(last_command.stdboth).to eq("/foo-original")
+      expect(last_command.stdboth).to include("/foo-original")
+    end
+  end
+
+  describe "Bundler.unbundled_env" do
+    let(:modified_env) { "Bundler.unbundled_env" }
+
+    it_behaves_like "an unbundling helper"
+  end
+
+  describe "Bundler.clean_env" do
+    let(:modified_env) { "Bundler.clean_env" }
+
+    it_behaves_like "an unbundling helper"
+
+    it "prints a deprecation", :bundler => 2 do
+      code = "Bundler.clean_env"
+      bundle_exec_ruby! code.dump
+      expect(last_command.stdboth).to include(
+        "[DEPRECATED FOR 2.0] `Bundler.clean_env` has been deprecated in favor of `Bundler.unbundled_env`. " \
+        "If you instead want the environment before bundler was originally loaded, use `Bundler.original_env`"
+      )
+    end
+
+    it "does not print a deprecation", :bundler => "< 2" do
+      code = "Bundler.clean_env"
+      bundle_exec_ruby! code.dump
+      expect(last_command.stdboth).not_to include(
+        "[DEPRECATED FOR 2.0] `Bundler.clean_env` has been deprecated in favor of `Bundler.unbundled_env`. " \
+        "If you instead want the environment before bundler was originally loaded, use `Bundler.original_env`"
+      )
     end
   end
 
@@ -116,9 +142,9 @@ RSpec.describe "Bundler.with_env helpers" do
     end
   end
 
-  describe "Bundler.with_clean_env", :bundler => "< 2" do
-    it "should set ENV to clean_env in the block" do
-      expected = Bundler.clean_env
+  describe "Bundler.with_clean_env" do
+    it "should set ENV to unbundled_env in the block" do
+      expected = Bundler.unbundled_env
       actual = Bundler.with_clean_env { ENV.to_hash }
       expect(actual).to eq(expected)
     end
@@ -130,6 +156,40 @@ RSpec.describe "Bundler.with_env helpers" do
 
       expect(ENV).not_to have_key("FOO")
     end
+
+    it "prints a deprecation", :bundler => 2 do
+      code = "Bundler.with_clean_env {}"
+      bundle_exec_ruby! code.dump
+      expect(last_command.stdboth).to include(
+        "[DEPRECATED FOR 2.0] `Bundler.with_clean_env` has been deprecated in favor of `Bundler.with_unbundled_env`. " \
+        "If you instead want the environment before bundler was originally loaded, use `Bundler.with_original_env`"
+      )
+    end
+
+    it "does not print a deprecation", :bundler => "< 2" do
+      code = "Bundler.with_clean_env {}"
+      bundle_exec_ruby! code.dump
+      expect(last_command.stdboth).not_to include(
+        "[DEPRECATED FOR 2.0] `Bundler.with_clean_env` has been deprecated in favor of `Bundler.with_unbundled_env`. " \
+        "If you instead want the environment before bundler was originally loaded, use `Bundler.with_original_env`"
+      )
+    end
+  end
+
+  describe "Bundler.with_unbundled_env" do
+    it "should set ENV to unbundled_env in the block" do
+      expected = Bundler.unbundled_env
+      actual = Bundler.with_unbundled_env { ENV.to_hash }
+      expect(actual).to eq(expected)
+    end
+
+    it "should restore the environment after execution" do
+      Bundler.with_unbundled_env do
+        ENV["FOO"] = "hello"
+      end
+
+      expect(ENV).not_to have_key("FOO")
+    end
   end
 
   describe "Bundler.clean_system", :ruby => ">= 1.9", :bundler => "< 2" do