Merge pull request #847 from Shopify/vs/improve_custom_bundle

Use pathnames and bundler methods in custom bundle logic

Author: vinistock

exe/ruby-lsp

@@ -1,32 +1,21 @@
 #!/usr/bin/env ruby
 # frozen_string_literal: true
 
-# We should make sure that, if we're running on a bundler project, that it has a Gemfile.lock
-if File.exist?("Gemfile") && !File.exist?("Gemfile.lock")
-  warn("Project contains a Gemfile, but no Gemfile.lock. Run `bundle install` to lock gems and restart the server")
-  exit(78)
-end
-
 # When we're running without bundler, then we need to make sure the custom bundle is fully configured and re-execute
 # using `BUNDLE_GEMFILE=.ruby-lsp/Gemfile bundle exec ruby-lsp` so that we have access to the gems that are a part of
 # the application's bundle
-if ENV["BUNDLE_GEMFILE"].nil? && File.exist?("Gemfile.lock")
+if ENV["BUNDLE_GEMFILE"].nil?
   require_relative "../lib/ruby_lsp/setup_bundler"
-  RubyLsp::SetupBundler.new(Dir.pwd).setup!
-
-  # In some cases, like when the `ruby-lsp` is already a part of the bundle, we don't generate `.ruby-lsp/Gemfile`.
-  # However, we still want to run the server with `bundle exec`. We need to make sure we're pointing to the right
-  # `Gemfile`
-  bundle_gemfile = File.exist?(".ruby-lsp/Gemfile") ? ".ruby-lsp/Gemfile" : "Gemfile"
 
-  # In addition to BUNDLE_GEMFILE, we also need to make sure that BUNDLE_PATH is absolute and not relative. For example,
-  # if BUNDLE_PATH is `vendor/bundle`, we want the top level `vendor/bundle` and not `.ruby-lsp/vendor/bundle`.
-  # Expanding to get the absolute path ensures we're pointing to the correct folder, which is the same one we use in
-  # SetupBundler to install the gems
-  path = Bundler.settings["path"]
+  begin
+    bundle_gemfile, bundle_path = RubyLsp::SetupBundler.new(Dir.pwd).setup!
+  rescue RubyLsp::SetupBundler::BundleNotLocked
+    warn("Project contains a Gemfile, but no Gemfile.lock. Run `bundle install` to lock gems and restart the server")
+    exit(78)
+  end
 
   env = { "BUNDLE_GEMFILE" => bundle_gemfile }
-  env["BUNDLE_PATH"] = File.expand_path(path, Dir.pwd) if path
+  env["BUNDLE_PATH"] = bundle_path if bundle_path
   exit exec(env, "bundle exec ruby-lsp #{ARGV.join(" ")}")
 end
 

lib/ruby_lsp/setup_bundler.rb

@@ -4,6 +4,7 @@
 require "sorbet-runtime"
 require "bundler"
 require "fileutils"
+require "pathname"
 
 # This file is a script that will configure a custom bundle for the Ruby LSP. The custom bundle allows developers to use
 # the Ruby LSP without including the gem in their application's Gemfile while at the same time giving us access to the
@@ -13,107 +14,133 @@ module RubyLsp
   class SetupBundler
     extend T::Sig
 
+    class BundleNotLocked < StandardError; end
+
     sig { params(project_path: String).void }
     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
-          {}
+
+      # Custom bundle paths
+      @custom_dir = T.let(Pathname.new(".ruby-lsp").expand_path(Dir.pwd), Pathname)
+      @custom_gemfile = T.let(@custom_dir + "Gemfile", Pathname)
+      @custom_lockfile = T.let(@custom_dir + "Gemfile.lock", Pathname)
+
+      # Regular bundle paths
+      @gemfile = T.let(
+        begin
+          Bundler.default_gemfile
+        rescue Bundler::GemfileNotFound
+          nil
         end,
-        T::Hash[String, T.untyped],
+        T.nilable(Pathname),
       )
+      @lockfile = T.let(@gemfile ? Bundler.default_lockfile : nil, T.nilable(Pathname))
+
+      @dependencies = T.let(load_dependencies, T::Hash[String, T.untyped])
+      @custom_bundle_dependencies = T.let(custom_bundle_dependencies, T::Hash[String, T.untyped])
     end
 
-    sig { void }
+    # Setups up the custom bundle and returns the `BUNDLE_GEMFILE` and `BUNDLE_PATH` that should be used for running the
+    # server
+    sig { returns([String, T.nilable(String)]) }
     def setup!
-      # Do not setup a custom bundle if we're working on the Ruby LSP, since it's already included by default
-      if File.basename(@project_path) == "ruby-lsp"
-        warn("Ruby LSP> Skipping custom bundle setup since we're working on the Ruby LSP itself")
-        run_bundle_install
-        return
-      end
+      raise BundleNotLocked if @gemfile&.exist? && !@lockfile&.exist?
 
       # 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")
+        warn("Ruby LSP> Skipping custom bundle setup since both `ruby-lsp` and `debug` are already in #{@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
+        @custom_dir.rmtree if @custom_dir.exist?
+        return run_bundle_install
       end
 
       # Automatically create and ignore the .ruby-lsp folder for users
-      FileUtils.mkdir(".ruby-lsp") unless Dir.exist?(".ruby-lsp")
-      File.write(".ruby-lsp/.gitignore", "*") unless File.exist?(".ruby-lsp/.gitignore")
+      @custom_dir.mkpath unless @custom_dir.exist?
+      ignore_file = @custom_dir + ".gitignore"
+      ignore_file.write("*") unless ignore_file.exist?
 
-      # Write the custom `.ruby-lsp/Gemfile` if it doesn't exist or if the content doesn't match
-      content = custom_gemfile_content
+      write_custom_gemfile
 
-      unless File.exist?(".ruby-lsp/Gemfile") && File.read(".ruby-lsp/Gemfile") == content
-        File.write(".ruby-lsp/Gemfile", content)
+      unless @gemfile&.exist? && @lockfile&.exist?
+        warn("Ruby LSP> Skipping lockfile copies because there's no top level bundle")
+        return run_bundle_install(@custom_gemfile)
       end
 
       # If .ruby-lsp/Gemfile.lock already exists and the top level Gemfile.lock hasn't been modified since it was last
       # updated, then we're ready to boot the server
-      if File.exist?(".ruby-lsp/Gemfile.lock") &&
-          File.stat(".ruby-lsp/Gemfile.lock").mtime > File.stat("Gemfile.lock").mtime
-        warn("Ruby LSP> Skipping custom bundle setup since .ruby-lsp/Gemfile.lock already exists and is up to date")
-        run_bundle_install(".ruby-lsp/Gemfile")
-        return
+      if @custom_lockfile.exist? && @custom_lockfile.stat.mtime > @lockfile.stat.mtime
+        warn("Ruby LSP> Skipping custom bundle setup since #{@custom_lockfile} already exists and is up to date")
+        return run_bundle_install(@custom_gemfile)
       end
 
-      FileUtils.cp("Gemfile.lock", ".ruby-lsp/Gemfile.lock")
-      run_bundle_install(".ruby-lsp/Gemfile")
+      FileUtils.cp(@lockfile.to_s, @custom_lockfile.to_s)
+      run_bundle_install(@custom_gemfile)
     end
 
     private
 
-    sig { returns(String) }
-    def custom_gemfile_content
+    sig { returns(T::Hash[String, T.untyped]) }
+    def custom_bundle_dependencies
+      return {} unless @custom_lockfile.exist?
+
+      ENV["BUNDLE_GEMFILE"] = @custom_gemfile.to_s
+      Bundler::LockfileParser.new(@custom_lockfile.read).dependencies
+    ensure
+      ENV.delete("BUNDLE_GEMFILE")
+    end
+
+    sig { void }
+    def write_custom_gemfile
       parts = [
         "# This custom gemfile is automatically generated by the Ruby LSP.",
         "# It should be automatically git ignored, but in any case: do not commit it to your repository.",
         "",
-        "eval_gemfile(File.expand_path(\"../Gemfile\", __dir__))",
       ]
 
+      # If there's a top level Gemfile, we want to evaluate from the custom bundle. We get the source from the top level
+      # Gemfile, so if there isn't one we need to add a default source
+      if @gemfile&.exist?
+        parts << "eval_gemfile(File.expand_path(\"../Gemfile\", __dir__))"
+      else
+        parts.unshift('source "https://rubygems.org"')
+      end
+
       unless @dependencies["ruby-lsp"]
-        parts << 'gem "ruby-lsp", require: false, group: :development, source: "https://rubygems.org"'
+        parts << 'gem "ruby-lsp", require: false, group: :development'
       end
 
       unless @dependencies["debug"]
-        parts << 'gem "debug", require: false, group: :development, platforms: :mri, source: "https://rubygems.org"'
+        parts << 'gem "debug", require: false, group: :development, platforms: :mri'
       end
 
-      parts.join("\n")
+      content = parts.join("\n")
+      @custom_gemfile.write(content) unless @custom_gemfile.exist? && @custom_gemfile.read == content
     end
 
     sig { returns(T::Hash[String, T.untyped]) }
     def load_dependencies
+      return {} unless @lockfile&.exist?
+
       # We need to parse the Gemfile.lock manually here. If we try to do `bundler/setup` to use something more
       # convenient, we may end up with issues when the globally installed `ruby-lsp` version mismatches the one included
       # in the `Gemfile`
-      dependencies = Bundler::LockfileParser.new(Bundler.read_file("Gemfile.lock")).dependencies
+      dependencies = Bundler::LockfileParser.new(@lockfile.read).dependencies
 
       # When working on a gem, the `ruby-lsp` might be listed as a dependency in the gemspec. We need to make sure we
-      # check those as well or else we may get version mismatch errors
-      gemspec_path = Dir.glob("*.gemspec").first
-      if gemspec_path
-        gemspec_dependencies = Bundler.load_gemspec(gemspec_path).dependencies.to_h { |dep| [dep.name, dep] }
-        dependencies.merge!(gemspec_dependencies)
+      # check those as well or else we may get version mismatch errors. Notice that bundler allows more than one
+      # gemspec, so we need to make sure we go through all of them
+      Dir.glob("{,*}.gemspec").each do |path|
+        dependencies.merge!(Bundler.load_gemspec(path).dependencies.to_h { |dep| [dep.name, dep] })
       end
 
       dependencies
     end
 
-    sig { params(bundle_gemfile: T.untyped).void }
-    def run_bundle_install(bundle_gemfile = nil)
+    sig { params(bundle_gemfile: T.nilable(Pathname)).returns([String, T.nilable(String)]) }
+    def run_bundle_install(bundle_gemfile = @gemfile)
       # If the user has a custom bundle path configured, we need to ensure that we will use the absolute and not
       # relative version of it when running `bundle install`. This is necessary to avoid installing the gems under the
       # `.ruby-lsp` folder, which is not the user's intention. For example, if the path is configured as `vendor`, we
@@ -122,7 +149,7 @@ def run_bundle_install(bundle_gemfile = nil)
 
       # Use the absolute `BUNDLE_PATH` to prevent accidentally creating unwanted folders under `.ruby-lsp`
       env = {}
-      env["BUNDLE_GEMFILE"] = bundle_gemfile if bundle_gemfile
+      env["BUNDLE_GEMFILE"] = bundle_gemfile.to_s
       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
@@ -150,6 +177,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(env, command)
+      [bundle_gemfile.to_s, path]
     end
   end
 end

test/setup_bundler_test.rb

@@ -5,12 +5,6 @@
 require "ruby_lsp/setup_bundler"
 
 class SetupBundlerTest < Minitest::Test
-  def test_does_nothing_when_running_in_the_ruby_lsp
-    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_env, "bundle install 1>&2")
     Bundler::LockfileParser.any_instance.expects(:dependencies).returns({ "ruby-lsp" => true, "debug" => true })
@@ -51,7 +45,10 @@ def test_copies_gemfile_lock_when_modified
     sleep(0.05)
     FileUtils.touch("Gemfile.lock")
 
-    FileUtils.expects(:cp).with("Gemfile.lock", ".ruby-lsp/Gemfile.lock")
+    FileUtils.expects(:cp).with(
+      File.expand_path("Gemfile.lock", Dir.pwd),
+      File.expand_path(".ruby-lsp/Gemfile.lock", Dir.pwd),
+    )
 
     run_script
   ensure
@@ -80,6 +77,83 @@ def test_uses_absolute_bundle_path_for_bundle_install
     FileUtils.rm_r(".ruby-lsp")
   end
 
+  def test_creates_custom_bundle_if_no_gemfile
+    # Create a temporary directory with no Gemfile or Gemfile.lock
+    Dir.mktmpdir do |dir|
+      Dir.chdir(dir) do
+        bundle_gemfile = Pathname.new(".ruby-lsp").expand_path(Dir.pwd) + "Gemfile"
+        Object.any_instance.expects(:system).with(bundle_env(bundle_gemfile.to_s), "bundle install 1>&2")
+
+        Bundler.with_unbundled_env do
+          run_script
+        end
+
+        assert_path_exists(".ruby-lsp")
+        assert_path_exists(".ruby-lsp/Gemfile")
+        assert_match("ruby-lsp", File.read(".ruby-lsp/Gemfile"))
+        assert_match("debug", File.read(".ruby-lsp/Gemfile"))
+      end
+    end
+  end
+
+  def test_raises_if_bundle_is_not_locked
+    # Create a temporary directory with no Gemfile or Gemfile.lock
+    Dir.mktmpdir do |dir|
+      Dir.chdir(dir) do
+        FileUtils.touch("Gemfile")
+
+        Bundler.with_unbundled_env do
+          assert_raises(RubyLsp::SetupBundler::BundleNotLocked) do
+            run_script
+          end
+        end
+      end
+    end
+  end
+
+  def test_does_nothing_if_both_ruby_lsp_and_debug_are_gemspec_dependencies
+    Dir.mktmpdir do |dir|
+      Dir.chdir(dir) do
+        # Write a fake Gemfile and gemspec
+        File.write(File.join(dir, "Gemfile"), <<~GEMFILE)
+          source "https://rubygems.org"
+          gemspec
+        GEMFILE
+
+        File.write(File.join(dir, "fake.gemspec"), <<~GEMSPEC)
+          Gem::Specification.new do |s|
+            s.name = "fake"
+            s.version = "0.1.0"
+            s.authors = ["Dev"]
+            s.email = ["[email protected]"]
+            s.metadata["allowed_push_host"] = "https://rubygems.org"
+
+            s.summary = "A fake gem"
+            s.description = "A fake gem"
+            s.homepage = "https://github.com/fake/gem"
+            s.license = "MIT"
+            s.files = Dir.glob("lib/**/*.rb") + ["README.md", "VERSION", "LICENSE.txt"]
+            s.bindir = "exe"
+            s.require_paths = ["lib"]
+
+            s.add_dependency("ruby-lsp")
+            s.add_dependency("debug")
+          end
+        GEMSPEC
+
+        FileUtils.touch(File.join(dir, "Gemfile.lock"))
+
+        Bundler.with_unbundled_env do
+          Object.any_instance.expects(:system).with(bundle_env, "bundle install 1>&2")
+          Bundler::LockfileParser.any_instance.expects(:dependencies).returns({})
+          run_script
+        end
+
+        refute_path_exists(".ruby-lsp")
+      end
+    end
+  end
+
   private
 
   # This method runs the script and then immediately unloads it. This allows us to make assertions against the effects
@@ -88,12 +162,14 @@ def run_script(path = "/fake/project/path")
     RubyLsp::SetupBundler.new(path).setup!
   end
 
-  def bundle_env(bundle_gemfile = nil)
+  def bundle_env(bundle_gemfile = "Gemfile")
+    bundle_gemfile_path = Pathname.new(bundle_gemfile)
     path = Bundler.settings["path"]
 
     env = {}
     env["BUNDLE_PATH"] = File.expand_path(path, Dir.pwd) if path
-    env["BUNDLE_GEMFILE"] = bundle_gemfile if bundle_gemfile
+    env["BUNDLE_GEMFILE"] =
+      bundle_gemfile_path.absolute? ? bundle_gemfile_path.to_s : bundle_gemfile_path.expand_path(Dir.pwd).to_s
     env
   end
 end