Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix chdir issue for LSP #2171

Merged
merged 1 commit into from
Jan 29, 2025
Merged

Fix chdir issue for LSP #2171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/ruby_lsp/tapioca/addon.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ def file_updated?(change, path)

sig { void }
def run_gem_rbi_check
gem_rbi_check = RunGemRbiCheck.new
gem_rbi_check = RunGemRbiCheck.new(T.must(@global_state).workspace_path)
gem_rbi_check.run

T.must(@outgoing_queue) << Notification.window_log_message(
Expand Down
48 changes: 32 additions & 16 deletions lib/ruby_lsp/tapioca/run_gem_rbi_check.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,28 +13,28 @@ class RunGemRbiCheck
attr_reader :stderr
attr_reader :status

sig { void }
def initialize
sig { params(project_path: String).void }
def initialize(project_path)
@project_path = project_path
@stdout = T.let("", String)
@stderr = T.let("", String)
@status = T.let(nil, T.nilable(Process::Status))
end

sig { params(project_path: String).void }
def run(project_path = ".")
FileUtils.chdir(project_path) do
return log_message("Not a git repository") unless git_repo?
sig { void }
def run
return log_message("Not a git repository") unless git_repo?

lockfile_changed? ? generate_gem_rbis : cleanup_orphaned_rbis
end
lockfile_changed? ? generate_gem_rbis : cleanup_orphaned_rbis
andyw8 marked this conversation as resolved.
Show resolved Hide resolved
end

private

attr_reader :project_path

sig { returns(T::Boolean) }
def git_repo?
_, status = Open3.capture2e("git rev-parse --is-inside-work-tree")
T.must(status.success?)
!!system("git rev-parse --is-inside-work-tree", chdir: project_path)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get why we are doing a !! here, but it is totally fine for a ? method to return T.nilable(T::Boolean), which would remove the need for this operation.

Predicate methods in Ruby return truthy or falsy values, not booleans.

end

sig { returns(T::Boolean) }
Expand All @@ -45,7 +45,11 @@ def lockfile_changed?

sig { returns(String) }
def fetch_lockfile_diff
@lockfile_diff = File.exist?("Gemfile.lock") ? %x(git diff Gemfile.lock).strip : ""
@lockfile_diff = if File.exist?(File.join(project_path, "Gemfile.lock"))
execute_in_project_path("git", "diff", "Gemfile.lock").strip
else
""
end
end

sig { void }
Expand All @@ -72,6 +76,7 @@ def execute_tapioca_gem_command(gems)
"gem",
"--lsp_addon",
*gems,
chdir: project_path,
andyw8 marked this conversation as resolved.
Show resolved Hide resolved
)

log_message(stdout) unless stdout.empty?
Expand All @@ -82,35 +87,46 @@ def execute_tapioca_gem_command(gems)

sig { params(gems: T::Array[String]).void }
def remove_rbis(gems)
FileUtils.rm_f(Dir.glob("sorbet/rbi/gems/{#{gems.join(",")}}@*.rbi"))
FileUtils.rm_f(Dir.glob("sorbet/rbi/gems/{#{gems.join(",")}}@*.rbi", base: project_path))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is correct. From the docs:

If optional keyword argument base is given, its value specifies the base directory. Each pattern string specifies entries relative to the base directory; the default is '.'. The base directory is not prepended to the entry names in the result:

Dir.glob(pattern, base: 'lib').take(5)
# => ["abbrev.gemspec", "abbrev.rb", "base64.gemspec", "base64.rb", "benchmark.gemspec"]

You will need to then File.join all the glob results with the project_path before passing them to FileUtils.rm_f.

In any case, for readability, it would be great to pull out the glob result into a local variable.

log_message("Removed RBIs for: #{gems.join(", ")}")
end

sig { void }
def cleanup_orphaned_rbis
untracked_files = %x(git ls-files --others --exclude-standard sorbet/rbi/gems/).lines.map(&:strip)
deleted_files = %x(git ls-files --deleted sorbet/rbi/gems/).lines.map(&:strip)
untracked_files = execute_in_project_path(
"git",
"ls-files",
"--others",
"--exclude-standard",
"sorbet/rbi/gems/",
).lines.map(&:strip)
deleted_files = execute_in_project_path("git", "ls-files", "--deleted", "sorbet/rbi/gems/").lines.map(&:strip)
Comment on lines +96 to +103
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like this is missing a git_ls_gem_rbis abstraction:

Suggested change
untracked_files = execute_in_project_path(
"git",
"ls-files",
"--others",
"--exclude-standard",
"sorbet/rbi/gems/",
).lines.map(&:strip)
deleted_files = execute_in_project_path("git", "ls-files", "--deleted", "sorbet/rbi/gems/").lines.map(&:strip)
untracked_files = git_ls_gem_rbis("--others", "--exclude-standard")
deleted_files = git_ls_gem_rbis("--deleted")


delete_files(untracked_files, "Deleted untracked RBIs")
restore_files(deleted_files, "Restored deleted RBIs")
end

sig { params(files: T::Array[String], message: String).void }
def delete_files(files, message)
files.each { |file| File.delete(file) }
FileUtils.rm(files.map { |file| File.join(project_path, file) })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, we are doing this operation in two places, remove_rbis and here in this method. We seem to be missing a common abstraction that will take relative file paths and remove them after mapping them to absolute paths.

log_message("#{message}: #{files.join(", ")}") unless files.empty?
end

sig { params(files: T::Array[String], message: String).void }
def restore_files(files, message)
files.each { |file| %x(git checkout -- #{file}) }
files.each { |file| execute_in_project_path("git", "checkout", "--", file) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to not need to execute multiple commands. If we join all the filenames, it could be too long, but we can stream them into the stdin of the execution, and use the --paths-from-file=- flag of git checkout to do all of this in one operation.

Something like:

Suggested change
files.each { |file| execute_in_project_path("git", "checkout", "--", file) }
execute_in_project_path("git", "checkout", "--pathspec-from-file=-", stdin: files.join("\n"))

log_message("#{message}: #{files.join(", ")}") unless files.empty?
end

sig { params(message: String).void }
def log_message(message)
@stdout += "#{message}\n"
end

def execute_in_project_path(*parts)
stdout_and_stderr, _status = T.unsafe(Open3).capture2e(*parts, chdir: project_path)
stdout_and_stderr
end
Comment on lines +126 to +129
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As part of my suggestion above:

Suggested change
def execute_in_project_path(*parts)
stdout_and_stderr, _status = T.unsafe(Open3).capture2e(*parts, chdir: project_path)
stdout_and_stderr
end
def execute_in_project_path(*parts, stdin: nil)
options = {}
options[:chdir] = project_path
options[:stdin_data] = stdin if stdin
stdout_and_stderr, _status = T.unsafe(Open3).capture2e(*parts, options)
stdout_and_stderr
end

end
end
end
30 changes: 15 additions & 15 deletions spec/tapioca/ruby_lsp/run_gem_rbi_check_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ class RunGemRbiCheckSpec < SpecWithProject
@project.require_mock_gem(foo)

@project.bundle_install!
check = ::RubyLsp::Tapioca::RunGemRbiCheck.new
check.run(@project.absolute_path)
check = ::RubyLsp::Tapioca::RunGemRbiCheck.new(@project.absolute_path)
check.run

assert check.stdout.include?("Not a git repository")
end
Expand Down Expand Up @@ -55,8 +55,8 @@ class RunGemRbiCheckSpec < SpecWithProject
@project.require_mock_gem(foo)
@project.bundle_install!

check = ::RubyLsp::Tapioca::RunGemRbiCheck.new
check.run(@project.absolute_path)
check = ::RubyLsp::Tapioca::RunGemRbiCheck.new(@project.absolute_path)
check.run

assert_project_file_exist("sorbet/rbi/gems/[email protected]")
end
Expand All @@ -68,16 +68,16 @@ class RunGemRbiCheckSpec < SpecWithProject
@project.require_mock_gem(foo)
@project.bundle_install!

check = ::RubyLsp::Tapioca::RunGemRbiCheck.new
check.run(@project.absolute_path)
check = ::RubyLsp::Tapioca::RunGemRbiCheck.new(@project.absolute_path)
check.run

assert_project_file_exist("sorbet/rbi/gems/[email protected]")

# Modify the gem
foo.update("0.0.2")
@project.bundle_install!

check.run(@project.absolute_path)
check.run

assert_project_file_exist("sorbet/rbi/gems/[email protected]")
end
Expand All @@ -89,15 +89,15 @@ class RunGemRbiCheckSpec < SpecWithProject
@project.require_mock_gem(foo)
@project.bundle_install!

check1 = ::RubyLsp::Tapioca::RunGemRbiCheck.new
check1.run(@project.absolute_path)
check1 = ::RubyLsp::Tapioca::RunGemRbiCheck.new(@project.absolute_path)
check1.run

assert_project_file_exist("sorbet/rbi/gems/[email protected]")

@project.exec("git restore Gemfile Gemfile.lock")

check2 = ::RubyLsp::Tapioca::RunGemRbiCheck.new
check2.run(@project.absolute_path)
check2 = ::RubyLsp::Tapioca::RunGemRbiCheck.new(@project.absolute_path)
check2.run

refute_project_file_exist("sorbet/rbi/gems/[email protected]")
end
Expand All @@ -110,8 +110,8 @@ class RunGemRbiCheckSpec < SpecWithProject

assert_project_file_exist("/sorbet/rbi/gems/[email protected]")

check = ::RubyLsp::Tapioca::RunGemRbiCheck.new
check.run(@project.absolute_path)
check = ::RubyLsp::Tapioca::RunGemRbiCheck.new(@project.absolute_path)
check.run

refute_project_file_exist("sorbet/rbi/gems/[email protected]")
end
Expand All @@ -127,8 +127,8 @@ class RunGemRbiCheckSpec < SpecWithProject

refute_project_file_exist("sorbet/rbi/gems/[email protected]")

check = ::RubyLsp::Tapioca::RunGemRbiCheck.new
check.run(@project.absolute_path)
check = ::RubyLsp::Tapioca::RunGemRbiCheck.new(@project.absolute_path)
check.run

assert_project_file_exist("sorbet/rbi/gems/[email protected]")

Expand Down
Loading