Closed e1senh0rn closed 4 years ago
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
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 toexport
, 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
orprintenv
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 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.
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.
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:
/bin/bash -l
)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.
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.
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.
Ok, #529 is the tracking issue for that
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
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 https://github.com/rubyide/vscode-ruby/issues/523#issuecomment-525957465):
for name in (set -nx)
echo $name=(string join : $$name)
end
@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.
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 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 https://github.com/rubyide/vscode-ruby/issues/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!
Working for me! 🎉 Awesome job @wingrunr21 👏
Great! Thanks to @e1senh0rn and @maxnordlund for working through the requirements on the shim.
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.
Your environment
vscode-ruby
version: 0.24.2Expected behavior
Enabling ruby linter via rubocop (using bundler) should check file without errors.
Actual behavior
Language server throws following error:
Running same command from terminal works just fine.
I investigated an error, and it really has nothing to do with bundler itself.
~/.gem/ruby/<ABI-version>
. I have non-bundled gems installed in ~/.gem/ruby/2.6.3, which prevents it from finding bundler.GEM_PATH
environment variable.export