Support fish shell in environment detection

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?

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

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?

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 😃

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

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.

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

@maxnordlund

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.

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.

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.

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.

@wingrunr21

@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.

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.

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

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.

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

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

@maxnordlund

@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

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.

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

That sounds like the best of both worlds 👍

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?

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

@wingrunr21

Working for me! 🎉 Awesome job @wingrunr21 👏

@e1senh0rn

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

@wingrunr21

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.

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.

Navigation Menu

Repository navigation

Support fish shell in environment detection #523

Your environment

Expected behavior

Actual behavior

10 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Support fish shell in environment detection #523

Description

Your environment

Expected behavior

Actual behavior

Activity

wingrunr21 commented on Aug 27, 2019

e1senh0rn commented on Aug 27, 2019

wingrunr21 commented on Aug 27, 2019

e1senh0rn commented on Aug 28, 2019

farcaller commented on Aug 28, 2019

maxnordlund commented on Aug 28, 2019

wingrunr21 commented on Aug 28, 2019

wingrunr21 commented on Aug 28, 2019

maxnordlund commented on Aug 28, 2019

wingrunr21 commented on Aug 29, 2019

e1senh0rn commented on Aug 29, 2019

wingrunr21 commented on Aug 31, 2019

10 remaining items

maxnordlund commented on Sep 18, 2019

maxnordlund commented on Sep 18, 2019

e1senh0rn commented on Sep 18, 2019

maxnordlund commented on Sep 18, 2019

e1senh0rn commented on Sep 19, 2019

maxnordlund commented on Sep 19, 2019

e1senh0rn commented on Sep 19, 2019

maxnordlund commented on Sep 19, 2019

breathe commented on Oct 25, 2019

wingrunr21 commented on Nov 19, 2019

pcriv commented on Nov 19, 2019

wingrunr21 commented on Nov 19, 2019

e1senh0rn commented on Nov 20, 2019

wingrunr21 commented on Nov 23, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions