Support fish shell in environment detection

[e1senh0rn]

Your environment

• vscode-ruby version: 0.24.2
• Ruby version: 2.6.3
• Ruby version manager (if any): chruby
• VS Code version: 1.37.1
• Operating System: macOS 10.14.6
• Using language server? Yes
• Login shell: /usr/local/bin/fish

Expected behavior

Enabling ruby linter via rubocop (using bundler) should check file without errors.

Actual behavior

Language server throws following error:

[Info  - 2:33:48 PM] Initializing Ruby language server...
[Info  - 2:33:48 PM] Rebuilding tree-sitter for local Electron version
[Info  - 2:33:48 PM] Rebuild succeeded!
Lint: executing bundle exec rubocop -s /Users/e1senh0rn/Projects/test/app.rb -f json...
/Users/e1senh0rn/.rubies/ruby-2.6.3/lib/ruby/2.6.0/rubygems.rb:283:in `find_spec_for_exe': Could not find 'bundler' (2.0.1) required by your /Users/e1senh0rn/Projects/test/Gemfile.lock. (Gem::GemNotFoundException)
To update to the latest version installed on your system, run `bundle update --bundler`.
To install the missing version, run `gem install bundler:2.0.1`

	from /Users/e1senh0rn/.rubies/ruby-2.6.3/lib/ruby/2.6.0/rubygems.rb:302:in `activate_bin_path'
	from /Users/e1senh0rn/.gem/ruby/2.6.3/bin/bundle:23:in `<main>'

Lint: Received invalid JSON from rubocop:

Running same command from terminal works just fine.

I investigated an error, and it really has nothing to do with bundler itself.

• Ruby version is 2.6.3. ABI version is 2.6.0. Rubygems by default tries to lookup gems in ~/.gem/ruby/<ABI-version>. I have non-bundled gems installed in ~/.gem/ruby/2.6.3, which prevents it from finding bundler.
• This can be adjusted with GEM_PATH environment variable.
• chruby actually sets variables correctly within shell, hence no errors while running it from terminal.
• vscode-ruby launches login shell in workspace directory to figure out environment variables.
• To launch shell it creates shell shim to list variables.
• I'm using fish shell, and resulting shim looks like this ( ~/.vscode/extensions/rebornix.ruby-0.24.2/client/out/util/shims/env.fish.sh):

#!/usr/local/bin/fish -i

export

• In fish export lists variables as VARIABLE value.
• processExportLine expects variables and values to be separated by =.
• This produces completely blank environment ({}) when launching ruby command.

This behavior will cause error with non-bash shells (fish, elvish, csh, tcsh, ...).

Affected code: https://github.com/rubyide/vscode-ruby/blob/master/packages/vscode-ruby/src/util/env.ts#L17

Could it be better to invoke /usr/bin/env instead of export?

[wingrunr21]

Nope. export is a shell built-in vs env which requires another process to start.

The shim looks that way because I haven't tested with the fish shell. Most likely I'll need to have different shim templates depending on the shell I'm using (similar to what I do for Windows).

There's a ticket for v0.26.0 to move the environment detection to the language server (as part of remote environment support) so I will look into supporting fish in that ticket

[e1senh0rn]

Builtin functionality is different by shell, and it might produce different formatting.
To me it seems that running env should produce an output in a portable way.
What is the reason to explicitly use builtin?

[wingrunr21]

I am not relying on functionality that differs by shell. export is POSIX compliant. I should probably be using export -p to fully conform with the spec, but also haven't had an issue with it until now 😃

[e1senh0rn]

Thing is, except for bash/zsh/ksh/dash, they aren't POSIX compliant. This is why env or printenv could be a common denominator.

[farcaller]

Not all the shells are POSIX compliant, though (fish isn't and it's a popular shell with a non-trivial user base).

Granted, /usr/bin/env isn't technically part of POSIX standard, but it's known to exist in the overwhelming majority of *NIX systems (see also this SO discussion).

I'd think this behavior is actually a bug. If you don't want to depend on the existence of /usr/bin/env you can check if the binary is there and fallback to export, but I think calling /usr/bin/env is good enough for all the systems.

[maxnordlund]

For reference, export is actually a function in fish. You can see it's implementation with type export or functions export:

function export --description 'Set env variable. Alias for `set -gx` for bash compatibility.'
    if not set -q argv[1]
        set -x
        return 0
    end
    for arg in $argv
        set -l v (string split -m 1 "=" -- $arg)
        switch (count $v)
            case 1
                set -gx $v $$v
            case 2
                if contains -- $v[1] PATH CDPATH MANPATH
                    set -l colonized_path (string replace -- "$$v[1]" (string join ":" -- $$v[1]) $v[2])
                    set -gx $v[1] (string split ":" -- $colonized_path)
                else
                    # status is 1 from the contains check, and `set` does not change the status on success: reset it.
                    true
                    set -gx $v[1] $v[2]
                end
        end
    end
end

Here you can see that it boils down to set -x which lists all exported fish variables. One thing to note here is that array variables, like PATH, are printed with two spaces between each single quote wrapped element:

$ set -Lx # Without -L the list gets cut off with an … at the end
PATH  '/usr/local/opt/curl/bin'  '/usr/local/bin'  '/usr/local/sbin'  '/usr/bin'  '/bin'  '/usr/sbin'  '/sbin'
# Omitted other environmental variables for brevity

This does what you're looking for (and only uses shell builtins):

for name in (set -nx)
    echo $name=(string join : $$name)
end

EDITOR=nvim
PATH=/usr/local/opt/curl/bin:/usr/local/bin:/usr/local/sbin:/usr/bin:/bin:/usr/sbin:/sbin
# etc

[wingrunr21]

Not all the shells are POSIX compliant, though (fish isn't and it's a popular shell with a non-trivial user base).

Yes, I realize this. POSIX is normally highly portable. I'm not saying I'm not going to support the fish shell, I'm saying I'm not going to do it the way you think is right.

I'd think this behavior is actually a bug. If you don't want to depend on the existence of /usr/bin/env you can check if the binary is there and fallback to export, but I think calling /usr/bin/env is good enough for all the systems.

Thanks for the input. This isn't a bug and I don't agree on using env. For example, users executing in remote environments (eg a docker container) may not have coreutils installed. If I have a working "fallback" from env, why not just use that in the first place and not rely on a program external to the shell.

env would also require spawning another process just to run a linter or formatter. People already talk about RuboCop being too slow.

@maxnordlund that's an interesting approach and I'll look into it more. However, if I can't rely on a POSIX standard to be portable, I'm going to end up in a world of supporting individual shell use cases anyway. Any shell that doesn't have a consistent set -nx implementation (if that exists) would be the same use case as here.

[wingrunr21]

Thing is, except for bash/zsh/ksh/dash, they aren't POSIX compliant. This is why env or printenv could be a common denominator.

bash/zsh/ksh/dash are a giant portion of users. I have my reasons for not wanting to use coreutils programs in the shims (a few of which I talked about above). The fish shell is popular, yes, but not nearly as much as those other ones. In choosing things to work on for this project, dealing with a non-POSIX shell wasn't high on my list. I said I'll support it but I do not think using env is the way to do it.

[maxnordlund]

I guess you would need to check what shell you are running first, then run either the fish specific stuff, or POSIX (like) shell.

I mentioned it briefly, but in fish export, aka set -x, cuts off long array variables with an ellipsis …. Which means you can't get away with trying to interpret the output after the fact.

[wingrunr21]

That’s essentially how it works today. Each shell has its own shim written to the file system. The POSIX template is shared but can be overridden. That’s how Windows support works.

[e1senh0rn]

@wingrunr21 Don't get me wrong, I'm not trying to push towards env or printenv in particular.

Having preset shims for major shells will cover most of the cases. Although there always will appear a random person with unsupported shell :)
One more detail. vscode-ruby uses login shell. Actually, it might be quite different from what user normally employs. For example, on mac in iterm you can set up zsh/fish as your shell, but leave login shell intact.
If shims are way to go, then some documentation will be of a great help. I could write some!
Also, it might be good to give user some control over env discovery process (like, shell selection, or using script wrapper instead of shell+shim).

Regarding 3rd-party utility and introduced delay - I see that you cache env discovery results, so I doubt it will add noticeable delay.

I see some reasons why you don't want to rely on coreutils, but could you elaborate a bit? I personally never encountered systems without env/printenv. Well, except some docker containers, but those usually don't have any shell either. But I definitely want to understand your reasoning and, may be, provide some other solution.

[wingrunr21]

If you have suggestions on how to detect the shell that someone has configured in iTerm I'm all ears. Remember that people don't always launch VSCode from the terminal. They can use the app dock, Spotlight, Alfred, whatever.

[minkir014]

I think when launching the debugger you can get the setting terminal.integrated.shell.{OSNAME} from the client-side so it means you need to detect the operating system before that.

[e1senh0rn]

Honestly I don't believe there is really 100% reliable way of detecting desired shell.
I think current behavior is the best one.

Adding documentation and customization can cover all the exception cases:

• pick login shell as default
  • add documentation describing this behavior
  • add configuration option for shell command line (for example, /bin/bash -l)
• use shims for different shells
  • add documentation regarding shims
  • add configuration option to allow custom shim. This way user will have an option to customize environment variables listing.

[e1senh0rn]

Custom shims might be a good idea.
I just updated plugin, and it generated new shim. vscode-ruby does not contain fish-specific shim yet, so it ended up broken.

[wingrunr21]

The way VSCode ships extensions + the changes to the directory structure will cause new shims to be written to the file system whenever they are needed.

Adding a config option to specify a custom shell is something I've thought about but I have reservations against. I don't want to make every small decision this extension makes configurable. It creates a ton of permutations to support/test against. For instance: it makes your configuration less portable. You would be unable to share that configuration between Windows and *nix machines or even across *nix machines if you don't install your choice of shell in the same location. In this instance I'd ask why your login shell differs from the shell you use all the time.

I'm not going to add documentation or the option to support custom shims. The shims are not intended to be user modified or exposed in any way. They are wrappers to get around limitations with spawn and getting an interactive login shell. They are supposed to be an internal construct.

[maxnordlund]

I've set SHELL to bash even though I use fish since some stuff broke. But at the same time I've set RBENV_SHELL and NODENV_SHELL to fish since they support it. It would therefore be nice if I could tell this package the same thing, that I use fish, even though I've set SHELL to bash.

I also agree that the user shouldn't be able to set their own shim, as that would leak implementation details. Letting the user choose which shim to use is something else though.

[wingrunr21]

Ok, #529 is the tracking issue for that

[e1senh0rn]

I attempted to put a script in fish to list variables using only builtins.

#!/usr/local/bin/fish -i

for var in (set -xgL)
  set -l pair (string split -m 1 ' ' $var)
  set -l var_key $pair[1]
  set -l var_val $pair[2]

  if string match -q '*PATH' $var_key
    set var_val (string join ':' $$var_key)
  end

  printf "%s=%s\n" $var_key $var_val
end

or with long keys:

#!/usr/local/bin/fish -i

for var in (set --global --export --long)
  set --local pair (string split --max 1 ' ' $var)
  set --local var_key $pair[1]
  set --local var_val $pair[2]

  if string match --quiet '*PATH' $var_key
    set var_val (string join ':' $$var_key)
  end

  printf "%s=%s\n" $var_key $var_val
end

[maxnordlund]

That doesn't respect local variables, like the ones set for the current shell only. I wonder why you filter out only variables ending in PATH? And why must it be run in interactive mode (fish -i)?

A simple solution, also only using builtins (from my comment above #523 (comment)):

for name in (set -nx)
    echo $name=(string join : $$name)
end

[e1senh0rn]

@maxnordlund I took fish -i from existing shim. Might not be necessary.

Sorry, completely missed your code! I didn't know about -n flag, and I really like your approach.

I agree, --global might not be needed.

Though it fails on some of the variables for me:

> echo $LDFLAGS
-L/usr/local/opt/readline/lib -L/usr/local/opt/mysql@5.7/lib
> echo (string join : $LDFLAGS)
string join: Unknown option '-L/usr/local/opt/readline/lib -L/usr/local/opt/mysql@5.7/lib'
in command substitution

Regarding PATH:

fish automatically creates arrays from all environment variables whose name ends in PATH, by splitting them on colons. Other variables are not automatically split.

[maxnordlund]

Aha, I forgot echo (string join : -- $$name), without -- fish tries to interpret the -L option. Good catch

[maxnordlund]

Regarding PATH:

fish automatically creates arrays from all environment variables whose name ends in PATH, by splitting them on colons. Other variables are not automatically split.

Yes indeed, the string join only outputs colons between elements. Which means that only split environmental variables, aka PATHs, will have them. If it's only a single element string join does nothing.

[e1senh0rn]

I was getting weird results for LDFLAGS, but figured it out.
In my config.fish I had

...
set -x LDFLAGS "-L/usr/local/opt/mysql@5.7/lib" $LDFLAGS
set -x LDFLAGS "-L/usr/local/opt/readline/lib" $LDFLAGS
...

This apparently set it as array, messing up the output.

But then I found another issue. Snippet put into file vs running in CLI outputs different results:

> ./env_list.fish | grep LDFLAGS
LDFLAGS=-L/usr/local/opt/readline/lib:-L/usr/local/opt/mysql@5.7/lib:-L/usr/local/opt/readline/lib -L/usr/local/opt/mysql@5.7/lib

> echo (string join : -- $LDFLAGS)
-L/usr/local/opt/readline/lib:-L/usr/local/opt/mysql@5.7/lib

[maxnordlund]

That is so weird, why would they be different? Hm, needs investigating, but I need to go now.

[e1senh0rn]

@maxnordlund I found the reason for different behavior, it was my fault.

Apparently fish always executes its startup files, and it is up to user to put if status --is-interactive or if status --is-login guards.

In my config I did prepend LDFLAGS with new values (if you search on github - I'm not the only one). Interactive shell starts with LDFLAGS being blank, so there was no issue.
On running fish script it was starting new fish session with LDFLAGS env var being non-empty, prepending it again.

This won't be an issue with running fish shim.

Regarding joining via :. Apparently in fish v3 *PATH variables were enhanced, and are implicitly joined or split via colons (https://fishshell.com/docs/current/index.html#variables-path).

#!/usr/local/bin/fish

for name in (set -nx)
  echo $name="$$name"
  # alternatively, via printf
  # printf "%s=%s\n" $name "$$name"
end

[maxnordlund]

Ah, that makes more sense even though it might be a bit annoying. Yeah, fish 3 makes it much easier, but I figured the shim must support fish 2 as well.

[e1senh0rn]

I would go with safe option then: join *PATH variables (as per documentation), and just "stringify" the rest (like LDFLAGS).

for name in (set -nx)
  if string match --quiet '*PATH' $name
    echo $name=(string join : -- $$name)
  else
    echo $name="$$name"
  end
end

[maxnordlund]

That sounds like the best of both worlds 👍

[breathe]

Hi -- I filed this issue #552 and see reference there that the cause of my problem is my usage of fish shell. Is there a way to work around this issue that doesn't involve changing my user shell?

[wingrunr21]

Fish shell support for the environment detection landed in v0.26.0. Please give it a try!

[pcriv]

Working for me! 🎉 Awesome job @wingrunr21 👏

[wingrunr21]

Great! Thanks to @e1senh0rn and @maxnordlund for working through the requirements on the shim.

[e1senh0rn]

Many thanks @wingrunr21 ! Works perfectly.

Just checked and noticed that shim file is actually named env.usr.local.bin.fish.fish. Not sure if it is intentional or not.

[wingrunr21]

That was intentional. When I moved the detection I decided to normalize around the path to the shell for the shim in case other use cases pop up later.