Shopify / ruby-lsp

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

"Automatic Ruby environment activation with none failed" (fish shell) #2278

Closed brodock closed 3 weeks ago

brodock commented 1 month ago

Description

Reproduction steps

In a machine using Fish with Oh My Fish:

  1. Open VSCode/VSCodium
  2. Get the error below

Code snippet or error message

Automatic Ruby environment activation with none failed: Command failed: ruby -W0 -rjson -e 'STDERR.print({ env: ENV.to_h, yjit: !!defined?(RubyVM::YJIT), version: RUBY_VERSION }.to_json)' ~/.local/share/omf/pkg/bundler/functions/__execute_as_bundler.fish (line 1): command substitutions not allowed here { env: ENV.to_h, yjit: !!defined?(RubyVM::YJIT), version: RUBY_VERSION }.to_json ^~~~~~~~~~~~~^ in command substitution called on line 1 of file ~/.local/share/omf/pkg/bundler/functions/__execute_as_bundler.fish in function '__execute_as_bundler' with arguments 'ruby -W0 -rjson -e STDERR.print\(\{\ env:\ ENV.to_h,\ yjit:\ !!defined\?\(RubyVM::YJIT\),\ version:\ RUBY_VERSION\ \}.to_json\)' called on line 1 of file ~/.local/share/omf/pkg/bundler/conf.d/bundler.fish in function 'ruby' with arguments '-W0 -rjson -e STDERR.print\(\{\ env:\ ENV.to_h,\ yjit:\ !!defined\?\(RubyVM::YJIT\),\ version:\ RUBY_VERSION\ \}.to_json\)' ~/.local/share/omf/pkg/bundler/fun...

The following is the output of running the failed command in a fish shell:

ruby -W0 -rjson -e "STDERR.print({ env: ENV.to_h, yjit: !!defined?(RubyVM::YJIT), version: RUBY_VERSION }.to_json)"
~/.local/share/omf/pkg/bundler/functions/__execute_as_bundler.fish (line 1): command substitutions not allowed here
{ env: ENV.to_h, yjit: !!defined?(RubyVM::YJIT), version: RUBY_VERSION }.to_json
                                 ^~~~~~~~~~~~~^
in command substitution
    called on line 1 of file ~/.local/share/omf/pkg/bundler/functions/__execute_as_bundler.fish
in function '__execute_as_bundler' with arguments 'ruby -W0 -rjson -e STDERR.print\(\{\ env:\ ENV.to_h,\ yjit:\ !!defined\?\(RubyVM::YJIT\),\ version:\ RUBY_VERSION\ \}.to_json\)'
    called on line 1 of file ~/.local/share/omf/pkg/bundler/conf.d/bundler.fish
in function 'ruby' with arguments '-W0 -rjson -e STDERR.print\(\{\ env:\ ENV.to_h,\ yjit:\ !!defined\?\(RubyVM::YJIT\),\ version:\ RUBY_VERSION\ \}.to_json\)'
~/.local/share/omf/pkg/bundler/functions/__execute_as_bundler.fish (line 1): Commandname was invalid
command ruby -W0 -rjson -e STDERR.print({ env: ENV.to_h, yjit: !!defined?(RubyVM::YJIT), version: RUBY_VERSION }.to_json)
                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
in function '__execute_as_bundler' with arguments 'ruby -W0 -rjson -e STDERR.print\(\{\ env:\ ENV.to_h,\ yjit:\ !!defined\?\(RubyVM::YJIT\),\ version:\ RUBY_VERSION\ \}.to_json\)'
    called on line 1 of file ~/.local/share/omf/pkg/bundler/conf.d/bundler.fish
in function 'ruby' with arguments '-W0 -rjson -e STDERR.print\(\{\ env:\ ENV.to_h,\ yjit:\ !!defined\?\(RubyVM::YJIT\),\ version:\ RUBY_VERSION\ \}.to_json\)'

Proposal

If the snippet above doesn't work well when using Fish shell, it should be executed with a compatible shell instead, ignoring whatever is in $SHELL.

vinistock commented 1 month ago

Thank you for the bug report! I'm a fish user myself and haven't had any issues so far.

I'm suspicious of whatever ~/.local/share/omf/pkg/bundler/functions/__execute_as_bundler.fish is doing. It seems that running ruby will run that script instead of the actual executable and that triggers the escaping issue.

Can you explain what that script does and how it modifies the ruby command?

louim commented 1 month ago

I assume that it's this file, based on the path and name.

brodock commented 1 month ago

Correct, it's one of the "plugins/extensions" from oh my fish

vinistock commented 1 month ago

I'm not really sure how to fix this. If we were to further escape the command, what escaping makes it work for that shell function?

And will the escaped version work for people who don't have that plugin?

brodock commented 1 month ago

@vinistock my proposal, as per in the issue description is to avoid doing a "per shell flavour" solution. We can implement a minimal shell detection trying to find sh or bash on each folder present on PATH (from the editor/typescript) and try to use that instead. We will never get something that supports all weird shell variations folks like me may be using, so stick to one reliable version. If you can't detect bash, use existing approach as fallback

vinistock commented 1 month ago

I'm not sure that will always work. For some version managers, the only reason it works at all is because we use the configured shell. We did try standardizing on /bin/sh and that lead to issues, especially with ASDF.

In addition to that, some users put sourcing of version manager specific things (or configure environment variables) in their profile/rc files, which we would also miss if we attempted to invoke their version manager through a shell they don't have configured.

Based on my experience trying to improve the reliability of Ruby runtime activation, making these sort of decisions might fix issues for a certain group of users, but will lead to problems with a different group.

Finally, if we automatically picked a shell that isn't the one the user has configured and activation failed, it would be extra confusing because it wouldn't be obvious to them why we aren't using the shell they have configured.

I would rather we do something like exposing a setting so that users can override what shell to use only for activation and make these cases the exception instead of the norm.

andyw8 commented 3 weeks ago

Closing for now as no action is planned. We'll continue to listen for more feedback, but it seems the current implementation is working well for most users.