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

Allow configuring extra chruby paths #1976

Merged
merged 1 commit into from May 1, 2024

Conversation

vinistock
Copy link
Member

@vinistock vinistock commented Apr 25, 2024

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.

@vinistock vinistock added enhancement New feature or request vscode This pull request should be included in the VS Code extension's release notes labels Apr 25, 2024
@vinistock vinistock self-assigned this Apr 25, 2024
@vinistock vinistock requested a review from a team as a code owner April 25, 2024 19:35
@vinistock vinistock requested review from andyw8 and st0012 April 25, 2024 19:35
@swrobel
Copy link

swrobel commented Apr 30, 2024

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

@vinistock
Copy link
Member Author

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
Copy link

swrobel commented Apr 30, 2024

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

@vinistock vinistock force-pushed the vs/allow_configuring_extra_chruby_paths branch 3 times, most recently from a5ad0d0 to c447b1c Compare April 30, 2024 19:39
@vinistock vinistock force-pushed the vs/allow_configuring_extra_chruby_paths branch from c447b1c to 88631cc Compare April 30, 2024 19:52
@vinistock vinistock merged commit f6f42b1 into main May 1, 2024
39 checks passed
@vinistock vinistock deleted the vs/allow_configuring_extra_chruby_paths branch May 1, 2024 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request vscode This pull request should be included in the VS Code extension's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants