Skip to content

Commit

Permalink
Account for missing bundle or lockfile in compose bundle request (#3104)
Browse files Browse the repository at this point in the history
### Motivation

When implementing the new `rubyLsp/composeBundle` request, I had the use case of automated restarts on lockfile changes in mind.

However, you can still trigger a restart manually through the commands and restarts are also triggered when a lockfile is deleted. In those two cases, the request was crashing.

### Implementation

Since we're already running the LSP, `Bundler.default_lockfile` may return the path to a lockfile that no longer exists (it is based on the `BUNDLE_GEMFILE` environment variable, which is already set at that point). We need to check if the file exists before trying to parse it.

Additionally, if the restart command is invoked manually and there's no bundle, we still shouldn't fail because we can compose the bundle regardless.

### Automated Tests

Added tests.
  • Loading branch information
vinistock committed Jan 29, 2025
1 parent f1e9ffe commit e4f6a84
Show file tree
Hide file tree
Showing 6 changed files with 87 additions and 16 deletions.
4 changes: 2 additions & 2 deletions exe/ruby-lsp-launcher
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ end

begin
# Wait until the composed Bundle is finished
Process.wait(pid)
_, status = Process.wait2(pid)
rescue Errno::ECHILD
# In theory, the child process can finish before we even get to the wait call, but that is not an error
end
Expand Down Expand Up @@ -105,7 +105,7 @@ end
# flow, we are not booting the LSP yet, just checking if the bundle is valid before rebooting
if reboot
# Use the exit status to signal to the server if composing the bundle succeeded
exit(install_error || setup_error ? 1 : 0)
exit(install_error || setup_error ? 1 : status&.exitstatus || 0)
end

# Now that the bundle is set up, we can begin actually launching the server. Note that `Bundler.setup` will have already
Expand Down
1 change: 1 addition & 0 deletions lib/ruby_lsp/internal.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
require "language_server-protocol"
require "rbs"
require "fileutils"
require "open3"

require "ruby-lsp"
require "ruby_lsp/base_server"
Expand Down
29 changes: 23 additions & 6 deletions lib/ruby_lsp/server.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1290,25 +1290,40 @@ def window_show_message_request(message)
addon.handle_window_show_message_response(result[:title])
end

sig { params(message: T::Hash[Symbol, T.untyped]).void }
# NOTE: all servers methods are void because they can produce several messages for the client. The only reason this
# method returns the created thread is to that we can join it in tests and avoid flakiness. The implementation is
# not supposed to rely on the return of this method
sig { params(message: T::Hash[Symbol, T.untyped]).returns(T.nilable(Thread)) }
def compose_bundle(message)
already_composed_path = File.join(@global_state.workspace_path, ".ruby-lsp", "bundle_is_composed")
command = "#{Gem.ruby} #{File.expand_path("../../exe/ruby-lsp-launcher", __dir__)} #{@global_state.workspace_uri}"
id = message[:id]

begin
Bundler::LockfileParser.new(Bundler.default_lockfile.read)
Bundler.with_original_env do
Bundler::LockfileParser.new(Bundler.default_lockfile.read)
end
rescue Bundler::LockfileError => e
send_message(Error.new(id: id, code: BUNDLE_COMPOSE_FAILED_CODE, message: e.message))
return
rescue Bundler::GemfileNotFound, Errno::ENOENT
# We still compose the bundle if there's no Gemfile or if the lockfile got deleted
end

# We compose the bundle in a thread so that the LSP continues to work while we're checking for its validity. Once
# we return the response back to the editor, then the restart is triggered
Thread.new do
send_log_message("Recomposing the bundle ahead of restart")
pid = Process.spawn(command)
_, status = Process.wait2(pid)

_stdout, stderr, status = Bundler.with_unbundled_env do
Open3.capture3(
Gem.ruby,
"-I",
File.dirname(T.must(__dir__)),
File.expand_path("../../exe/ruby-lsp-launcher", __dir__),
@global_state.workspace_uri.to_s,
chdir: @global_state.workspace_path,
)
end

if status&.exitstatus == 0
# Create a signal for the restart that it can skip composing the bundle and launch directly
Expand All @@ -1317,7 +1332,9 @@ def compose_bundle(message)
else
# This special error code makes the extension avoid restarting in case we already know that the composed
# bundle is not valid
send_message(Error.new(id: id, code: BUNDLE_COMPOSE_FAILED_CODE, message: "Failed to compose bundle"))
send_message(
Error.new(id: id, code: BUNDLE_COMPOSE_FAILED_CODE, message: "Failed to compose bundle\n#{stderr}"),
)
end
end
end
Expand Down
3 changes: 1 addition & 2 deletions lib/ruby_lsp/setup_bundler.rb
Original file line number Diff line number Diff line change
Expand Up @@ -241,8 +241,7 @@ def run_bundle_install(bundle_gemfile = @gemfile)

# If either the Gemfile or the lockfile have been modified during the process of setting up the bundle, retry
# composing the bundle from scratch

if @gemfile && @lockfile
if @gemfile&.exist? && @lockfile&.exist?
current_gemfile_hash = Digest::SHA256.hexdigest(@gemfile.read)
current_lockfile_hash = Digest::SHA256.hexdigest(@lockfile.read)

Expand Down
1 change: 0 additions & 1 deletion test/integration_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
# frozen_string_literal: true

require "test_helper"
require "open3"

class IntegrationTest < Minitest::Test
def setup
Expand Down
65 changes: 60 additions & 5 deletions test/server_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1068,7 +1068,7 @@ def test_compose_bundle_creates_file_to_skip_next_compose
})

capture_subprocess_io do
@server.process_message({ id: 2, method: "rubyLsp/composeBundle" })
@server.send(:compose_bundle, { id: 2, method: "rubyLsp/composeBundle" })&.join
end
result = find_message(RubyLsp::Result, id: 2)
assert(result.response[:success])
Expand Down Expand Up @@ -1116,12 +1116,63 @@ def test_compose_bundle_detects_syntax_errors_in_lockfile
LOCKFILE
File.write(File.join(dir, "Gemfile.lock"), lockfile_contents)

Bundler.with_unbundled_env do
@server.process_message({ id: 2, method: "rubyLsp/composeBundle" })
capture_subprocess_io do
@server.send(:compose_bundle, { id: 2, method: "rubyLsp/composeBundle" })&.join
end
end

error = find_message(RubyLsp::Error)
assert_match("Your Gemfile.lock contains merge conflicts.", error.message)
end
end

def test_compose_bundle_does_not_fail_if_bundle_is_missing
Dir.mktmpdir do |dir|
Dir.chdir(dir) do
@server.process_message({
id: 1,
method: "initialize",
params: {
initializationOptions: {},
capabilities: { general: { positionEncodings: ["utf-8"] } },
workspaceFolders: [{ uri: URI::Generic.from_path(path: dir).to_s }],
},
})

capture_subprocess_io do
@server.send(:compose_bundle, { id: 2, method: "rubyLsp/composeBundle" })&.join
end

result = find_message(RubyLsp::Result, id: 2)
assert(result.response[:success])
end
end
end

def test_compose_bundle_does_not_fail_if_restarting_on_lockfile_deletion
Dir.mktmpdir do |dir|
Dir.chdir(dir) do
@server.process_message({
id: 1,
method: "initialize",
params: {
initializationOptions: {},
capabilities: { general: { positionEncodings: ["utf-8"] } },
workspaceFolders: [{ uri: URI::Generic.from_path(path: dir).to_s }],
},
})

File.write(File.join(dir, "Gemfile"), <<~GEMFILE)
source "https://rubygems.org"
gem "stringio"
GEMFILE

capture_subprocess_io do
@server.send(:compose_bundle, { id: 2, method: "rubyLsp/composeBundle" })&.join
end

error = find_message(RubyLsp::Error)
assert_match("Your Gemfile.lock contains merge conflicts.", error.message)
result = find_message(RubyLsp::Result, id: 2)
assert(result.response[:success])
end
end
end
Expand Down Expand Up @@ -1201,6 +1252,10 @@ def find_message(desired_class, desired_method = nil, id: nil)
until message.is_a?(desired_class) && (!desired_method || T.unsafe(message).method == desired_method) &&
(!id || T.unsafe(message).id == id)
message = @server.pop_response

if message.is_a?(RubyLsp::Error) && desired_class != RubyLsp::Error
flunk("Unexpected error: #{message.message}")
end
end

message
Expand Down

0 comments on commit e4f6a84

Please sign in to comment.