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.

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

Rely always on workspace URI instead of Dir.pwd #2424

Merged
merged 1 commit into from
Aug 20, 2024

Conversation

vinistock
Copy link
Member

Motivation

Closes #2382

This PR stops using Dir.pwd and starts relying exclusively on the workspace URI provided by the client. Dir.pwd is not adequate because clients may spawn the language server in a directory that isn't the workspace. In addition to that, Dir.pwd may be a symlink, which can also cause issues in certain situations (as we received reports in the past).

Important note

There's one major caveat here. We are still unable to get rid of this usage of Dir.pwd when setting up the custom bundle.

The reason is because the way Bundler works is a bit incompatible with how language servers work and that prevents us from doing the right thing here. What the LSP spec dictates is that a language server should spawn and immediately respond to the initialize request.

However, for user convenience, we first setup the custom bundle, so that people don't have to add ruby-lsp to their Gemfiles. Ideally, we would

  • Spawn the server
  • Read the initialize request to get the workspace URI
  • Then setup the custom bundle
  • After a successful custom bundle setup, re-invoke Bundler.setup to organize the load paths as necessary
  • And then respond back to the editor

Bundler doesn't allow you to invoke Bundler.setup twice. And even if we only invoke it once, we would not be able to require anything until the Bundler.setup call happens because that may lead to required gem version mismatches.

This is the reason why we setup the custom bundle and then replace the existing process with a bundle exec version of it.

Because of the process replacement thing, we cannot read the initialize request from STDIN while setting up the custom bundle or else the parameters for initialization would be completely lost after re-launching the process with bundle exec.

Finally, since we cannot read the request parameters during custom bundle initialization, we do not know what the workspace URI is.

My hope is to work more closely with the Bundler team and brainstorm some way that dependency setup can be better integrated into the way language servers work. That would not only improve the Dir.pwd thing, but it would allow us to display custom error messages depending on which failures happened during the custom bundle setup. For now, this is the best we can do.

Implementation

Started using the provided workspace URI everywhere and removed all Dir.pwd usages that aren't in tests.

Automated Tests

Adapted existing tests.

@vinistock vinistock added bugfix This PR will fix an existing bug server This pull request should be included in the server gem's release notes labels Aug 9, 2024
@vinistock vinistock self-assigned this Aug 9, 2024
@vinistock vinistock requested a review from a team as a code owner August 9, 2024 19:49
@vinistock vinistock requested review from andyw8 and st0012 August 9, 2024 19:49
Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

Since the workspace_path is the key information for the indexer and now requires external output, we should make it a required argument and change how RubyIndexer::Configuration is initialized, even if we simply pass Dir.pwd in the first and change it later.

lib/ruby_indexer/lib/ruby_indexer/configuration.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/server.rb Outdated Show resolved Hide resolved
st0012 added a commit that referenced this pull request Aug 13, 2024
Since an Index's data is scoped by configuration, we should make the
configuration part of the index. This is especially true with #2424
coming because changing the state of `RubyIndexer.configuration` to
affect the index is surprising.
st0012 added a commit that referenced this pull request Aug 13, 2024
Since an Index's data is scoped by configuration, we should make the
configuration part of the index. This is especially true with #2424
coming because changing the state of `RubyIndexer.configuration` to
affect the index is surprising.
@st0012
Copy link
Member

st0012 commented Aug 13, 2024

I opened #2433 to change the way configuration is initialized.

st0012 added a commit that referenced this pull request Aug 13, 2024
Since an Index's data is scoped by configuration, we should make the
configuration part of the index. This is especially true with #2424
coming because changing the state of `RubyIndexer.configuration` to
affect the index is surprising.
@vinistock vinistock force-pushed the vs-use-workspace-uri-everywhere branch from a84055c to 0f83a1a Compare August 19, 2024 20:38
@vinistock vinistock requested a review from st0012 August 19, 2024 20:39
@vinistock vinistock force-pushed the vs-use-workspace-uri-everywhere branch 5 times, most recently from c678b0b to 766621d Compare August 20, 2024 18:32
@vinistock vinistock force-pushed the vs-use-workspace-uri-everywhere branch from f0f359b to 6a701ea Compare August 20, 2024 19:09
@vinistock vinistock enabled auto-merge (squash) August 20, 2024 19:39
@vinistock vinistock disabled auto-merge August 20, 2024 19:40
@vinistock vinistock merged commit 931f6a5 into main Aug 20, 2024
33 of 36 checks passed
@vinistock vinistock deleted the vs-use-workspace-uri-everywhere branch August 20, 2024 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This PR will fix an existing bug server This pull request should be included in the server gem's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ruby LSP always attempts to index the current working directory
2 participants