Merge pull request #841 from Shopify/vs/use_environment_for_system_calls

Pass environment as a hash to system or exec calls

Author: vinistock

exe/ruby-lsp

@@ -25,9 +25,9 @@ if ENV["BUNDLE_GEMFILE"].nil? && File.exist?("Gemfile.lock")
   # SetupBundler to install the gems
   path = Bundler.settings["path"]
 
-  command = +"BUNDLE_GEMFILE=#{bundle_gemfile} bundle exec ruby-lsp #{ARGV.join(" ")}"
-  command.prepend("BUNDLE_PATH=#{File.expand_path(path, Dir.pwd)} ") if path
-  exit exec(command)
+  env = { "BUNDLE_GEMFILE" => bundle_gemfile }
+  env["BUNDLE_PATH"] = File.expand_path(path, Dir.pwd) if path
+  exit exec(env, "bundle exec ruby-lsp #{ARGV.join(" ")}")
 end
 
 require "sorbet-runtime"

lib/ruby_lsp/setup_bundler.rb

@@ -120,16 +120,18 @@ def run_bundle_install(bundle_gemfile = nil)
       # want to install it in the top level `vendor` and not `.ruby-lsp/vendor`
       path = Bundler.settings["path"]
 
-      command = +""
       # Use the absolute `BUNDLE_PATH` to prevent accidentally creating unwanted folders under `.ruby-lsp`
-      command << "BUNDLE_PATH=#{File.expand_path(path, Dir.pwd)} " if path
-      command << "BUNDLE_GEMFILE=#{bundle_gemfile} " if bundle_gemfile
+      env = {}
+      env["BUNDLE_GEMFILE"] = bundle_gemfile if bundle_gemfile
+      env["BUNDLE_PATH"] = File.expand_path(path, Dir.pwd) if path
 
       # If both `ruby-lsp` and `debug` are already in the Gemfile, then we shouldn't try to upgrade them or else we'll
       # produce undesired source control changes. If the custom bundle was just created and either `ruby-lsp` or `debug`
       # weren't a part of the Gemfile, then we need to run `bundle install` for the first time to generate the
       # Gemfile.lock with them included or else Bundler will complain that they're missing. We can only update if the
       # custom `.ruby-lsp/Gemfile.lock` already exists and includes both gems
+      command = +""
+
       if (@dependencies["ruby-lsp"] && @dependencies["debug"]) ||
           @custom_bundle_dependencies["ruby-lsp"].nil? || @custom_bundle_dependencies["debug"].nil?
         # Install gems using the custom bundle
@@ -147,7 +149,7 @@ def run_bundle_install(bundle_gemfile = nil)
 
       # Add bundle update
       warn("Ruby LSP> Running bundle install for the custom bundle. This may take a while...")
-      system(command)
+      system(env, command)
     end
   end
 end

test/setup_bundler_test.rb

@@ -6,20 +6,20 @@
 
 class SetupBundlerTest < Minitest::Test
   def test_does_nothing_when_running_in_the_ruby_lsp
-    Object.any_instance.expects(:system).with(bundle_install_command)
+    Object.any_instance.expects(:system).with(bundle_env, "bundle install 1>&2")
     run_script("/some/path/ruby-lsp")
     refute_path_exists(".ruby-lsp")
   end
 
   def test_does_nothing_if_both_ruby_lsp_and_debug_are_in_the_bundle
-    Object.any_instance.expects(:system).with(bundle_install_command)
+    Object.any_instance.expects(:system).with(bundle_env, "bundle install 1>&2")
     Bundler::LockfileParser.any_instance.expects(:dependencies).returns({ "ruby-lsp" => true, "debug" => true })
     run_script
     refute_path_exists(".ruby-lsp")
   end
 
   def test_removes_ruby_lsp_folder_if_both_gems_were_added_to_the_bundle
-    Object.any_instance.expects(:system).with(bundle_install_command)
+    Object.any_instance.expects(:system).with(bundle_env, "bundle install 1>&2")
     Bundler::LockfileParser.any_instance.expects(:dependencies).returns({ "ruby-lsp" => true, "debug" => true })
     FileUtils.mkdir(".ruby-lsp")
     run_script
@@ -29,7 +29,7 @@ def test_removes_ruby_lsp_folder_if_both_gems_were_added_to_the_bundle
   end
 
   def test_creates_custom_bundle
-    Object.any_instance.expects(:system).with(bundle_install_command(".ruby-lsp/Gemfile"))
+    Object.any_instance.expects(:system).with(bundle_env(".ruby-lsp/Gemfile"), "bundle install 1>&2")
     Bundler::LockfileParser.any_instance.expects(:dependencies).returns({})
     run_script
 
@@ -43,7 +43,7 @@ def test_creates_custom_bundle
   end
 
   def test_copies_gemfile_lock_when_modified
-    Object.any_instance.expects(:system).with(bundle_install_command(".ruby-lsp/Gemfile"))
+    Object.any_instance.expects(:system).with(bundle_env(".ruby-lsp/Gemfile"), "bundle install 1>&2")
     Bundler::LockfileParser.any_instance.expects(:dependencies).returns({}).twice
     FileUtils.mkdir(".ruby-lsp")
     FileUtils.touch(".ruby-lsp/Gemfile.lock")
@@ -59,7 +59,7 @@ def test_copies_gemfile_lock_when_modified
   end
 
   def test_does_not_copy_gemfile_lock_when_not_modified
-    Object.any_instance.expects(:system).with(bundle_install_command(".ruby-lsp/Gemfile"))
+    Object.any_instance.expects(:system).with(bundle_env(".ruby-lsp/Gemfile"), "bundle install 1>&2")
     Bundler::LockfileParser.any_instance.expects(:dependencies).returns({}).twice
     FileUtils.mkdir(".ruby-lsp")
     FileUtils.cp("Gemfile.lock", ".ruby-lsp/Gemfile.lock")
@@ -71,7 +71,7 @@ def test_does_not_copy_gemfile_lock_when_not_modified
 
   def test_uses_absolute_bundle_path_for_bundle_install
     Bundler.settings.set_global("path", "vendor/bundle")
-    Object.any_instance.expects(:system).with(bundle_install_command(".ruby-lsp/Gemfile"))
+    Object.any_instance.expects(:system).with(bundle_env(".ruby-lsp/Gemfile"), "bundle install 1>&2")
     Bundler::LockfileParser.any_instance.expects(:dependencies).returns({})
     run_script
   ensure
@@ -88,13 +88,12 @@ def run_script(path = "/fake/project/path")
     RubyLsp::SetupBundler.new(path).setup!
   end
 
-  def bundle_install_command(bundle_gemfile = nil)
+  def bundle_env(bundle_gemfile = nil)
     path = Bundler.settings["path"]
 
-    command = +""
-    command << "BUNDLE_PATH=#{File.expand_path(path, Dir.pwd)} " if path
-    command << "BUNDLE_GEMFILE=#{bundle_gemfile} " if bundle_gemfile
-    command << "bundle install "
-    command << "1>&2"
+    env = {}
+    env["BUNDLE_PATH"] = File.expand_path(path, Dir.pwd) if path
+    env["BUNDLE_GEMFILE"] = bundle_gemfile if bundle_gemfile
+    env
   end
 end