Merge pull request #822 from Shopify/vs/remove_old_ruby_lsp_folder

Remove old .ruby-lsp folder if `ruby-lsp` and `debug` are in the Gemfile

Author: vinistock

Gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    ruby-lsp (0.7.2)
+    ruby-lsp (0.7.3)
       language_server-protocol (~> 3.17.0)
       sorbet-runtime
       syntax_tree (>= 6.1.1, < 7)

VERSION

@@ -1 +1 @@
-0.7.2
+0.7.3

lib/ruby_lsp/setup_bundler.rb

@@ -17,6 +17,14 @@ class SetupBundler
     def initialize(project_path)
       @project_path = project_path
       @dependencies = T.let(load_dependencies, T::Hash[String, T.untyped])
+      @custom_bundle_dependencies = T.let(
+        if File.exist?(".ruby-lsp/Gemfile.lock")
+          Bundler::LockfileParser.new(Bundler.read_file(".ruby-lsp/Gemfile.lock")).dependencies
+        else
+          {}
+        end,
+        T::Hash[String, T.untyped],
+      )
     end
 
     sig { void }
@@ -31,6 +39,11 @@ def setup!
       # Do not setup a custom bundle if both `ruby-lsp` and `debug` are already in the Gemfile
       if @dependencies["ruby-lsp"] && @dependencies["debug"]
         warn("Ruby LSP> Skipping custom bundle setup since both `ruby-lsp` and `debug` are already in the Gemfile")
+
+        # If the user decided to add the `ruby-lsp` and `debug` to their Gemfile after having already run the Ruby LSP,
+        # then we need to remove the `.ruby-lsp` folder, otherwise we will run `bundle install` for the top level and
+        # try to execute the Ruby LSP using the custom bundle, which will fail since the gems are not installed there
+        FileUtils.rm_r(".ruby-lsp") if Dir.exist?(".ruby-lsp")
         run_bundle_install
         return
       end
@@ -112,7 +125,13 @@ def run_bundle_install(bundle_gemfile = nil)
       command << "BUNDLE_PATH=#{File.expand_path(path, Dir.pwd)} " if path
       command << "BUNDLE_GEMFILE=#{bundle_gemfile} " if bundle_gemfile
 
-      if @dependencies["ruby-lsp"] && @dependencies["debug"]
+      # 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
+      if (@dependencies["ruby-lsp"] && @dependencies["debug"]) ||
+          @custom_bundle_dependencies["ruby-lsp"].nil? || @custom_bundle_dependencies["debug"].nil?
         # Install gems using the custom bundle
         command << "bundle install "
       else

test/setup_bundler_test.rb

@@ -6,18 +6,28 @@
 
 class SetupBundlerTest < Minitest::Test
   def test_does_nothing_when_running_in_the_ruby_lsp
-    Object.any_instance.expects(:system).with(bundle_install_command(update: false))
+    Object.any_instance.expects(:system).with(bundle_install_command)
     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(update: false))
+    Object.any_instance.expects(:system).with(bundle_install_command)
     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)
+    Bundler::LockfileParser.any_instance.expects(:dependencies).returns({ "ruby-lsp" => true, "debug" => true })
+    FileUtils.mkdir(".ruby-lsp")
+    run_script
+    refute_path_exists(".ruby-lsp")
+  ensure
+    FileUtils.rm_r(".ruby-lsp") if Dir.exist?(".ruby-lsp")
+  end
+
   def test_creates_custom_bundle
     Object.any_instance.expects(:system).with(bundle_install_command(".ruby-lsp/Gemfile"))
     Bundler::LockfileParser.any_instance.expects(:dependencies).returns({})
@@ -34,7 +44,7 @@ def test_creates_custom_bundle
 
   def test_copies_gemfile_lock_when_modified
     Object.any_instance.expects(:system).with(bundle_install_command(".ruby-lsp/Gemfile"))
-    Bundler::LockfileParser.any_instance.expects(:dependencies).returns({})
+    Bundler::LockfileParser.any_instance.expects(:dependencies).returns({}).twice
     FileUtils.mkdir(".ruby-lsp")
     FileUtils.touch(".ruby-lsp/Gemfile.lock")
     # Wait a little bit so that the modified timestamps don't match
@@ -50,7 +60,7 @@ def test_copies_gemfile_lock_when_modified
 
   def test_does_not_copy_gemfile_lock_when_not_modified
     Object.any_instance.expects(:system).with(bundle_install_command(".ruby-lsp/Gemfile"))
-    Bundler::LockfileParser.any_instance.expects(:dependencies).returns({})
+    Bundler::LockfileParser.any_instance.expects(:dependencies).returns({}).twice
     FileUtils.mkdir(".ruby-lsp")
     FileUtils.cp("Gemfile.lock", ".ruby-lsp/Gemfile.lock")
 
@@ -78,17 +88,13 @@ def run_script(path = "/fake/project/path")
     RubyLsp::SetupBundler.new(path).setup!
   end
 
-  def bundle_install_command(bundle_gemfile = nil, update: true)
+  def bundle_install_command(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 << if update
-      "bundle update ruby-lsp debug "
-    else
-      "bundle install "
-    end
+    command << "bundle install "
     command << "1>&2"
   end
 end