search

Shopify / ruby-lsp

An opinionated language server for Ruby
https://shopify.github.io/ruby-lsp/
MIT License
1.33k stars 118 forks source link

Allow configuring extra chruby paths #1976

Closed vinistock closed 2 weeks ago

vinistock commented 3 weeks ago

Motivation

Addressed the first part of #1969

Chruby allows configuring extra directories to search for Ruby installations. We'll need to offer a similar setting to match the behaviour.

Implementation

Added the setting and started pushing the configured URIs into the list of places to search for.

Automated Tests

Added a test.

swrobel commented 2 weeks ago

Is it not possible to just pull this from the environment variable? That seems like a cleaner solution, if possible.

vinistock commented 2 weeks ago

Not in a scalable and clean way. For example:

  1. How would we know where the environment variable is defined? The VS Code process doesn't source your ~/.zshrc, ~/.bashrc or equivalent. Your shell does, but not VS Code. Which means we would need to source those files, which has caused us a number of issues in the past
  2. What if the environment variable is not defined directly in the rc file? For example, what if RUBIES is only defined when there's a prompt or with some conditional behaviour. In many situations, invoking the shell from NodeJS doesn't match the expected conditionals, which may not set the variable

Trying to integrate with shells (our previous approach) basically resulted in a variety of problems. People can configure their environments in unexpected ways that don't integrate well with the extension. As examples, we had reports of users with shell plugins that refused to activate if they weren't in a TTY, breaking our integration.

In other cases, shell plugins hijacked the stderr pipe, redirecting it to something else and only restoring the original file descriptor if the user was about to write a command, which never gets detected when we invoke the shell from NodeJS - again breaking the editor/server communication.

In yet another case, some shell plugin maxed the user's CPU when we invoke their shell from the NodeJS process. Probably because it did not account for the case of having the shell be invoked as a script.

Basically, integrating with shells proved to be extremely brittle and lead to a lot of frustration. We invested significant effort to decouple from shells to prevent all of those. I understand that having to set RUBIES twice is a small inconvenience, but we had to accept that tradeoff in favour of more stable Ruby activation.

swrobel commented 2 weeks ago

@vinistock thanks for all the detail! This seems like a reasonable tradeoff.