-
-
Couldn't load subscription status.
- Fork 35
fix: support Fish shell's space-separated PATH format #172
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
fix: support Fish shell's space-separated PATH format #172
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix! I left a few nitpicky comments but I tested this and it does solve the issue. I can address them and merge this when I get back later today
apps/expert/lib/expert/port.ex
Outdated
| {path, 0} = System.cmd(shell, ["-i", "-l", "-c", "cd #{root_path} && echo $PATH"]) | ||
| # Ideally, it should contain the path to shell (e.g. `/usr/bin/fish`), | ||
| # but it might contain only the name of the shell (e.g. `fish`). | ||
| is_fish? = String.contains?(shell, "fish") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use Path.basename/1 here instead
apps/expert/lib/expert/port.ex
Outdated
| # Fish uses space-separated PATH, so we use the built-in `string join` command | ||
| # to join the entries with colons and have a standard colon-separated PATH output | ||
| # as in bash, which is expected by `:os.find_executable/2`. | ||
| path_command = if is_fish?, do: "string join ':' $PATH", else: "echo $PATH" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we prefer something like fish_shell?, is_x? mixes both naming conventions for guards and booleans
|
Related to alternate shells, I'm not sure if this direction is opening up more issues down the line vs the previous |
2fcd597 to
167d9ea
Compare
|
@rktjmp Unless we find a more reliable way, I'm inclined to special case nushell if it can provide the right path Another approach I dabbled with was to have a script with a shebang |
|
What version manager & platform do you use? I'm unable to reproduce any issues using mise+neovim on linux, which isn't to say the problem doesn't exist.
I don't want to gum up this PR. If you want to open an issue around version managers I can try to look at other configurations, or we can stick with the shell. Note both current releases of I guess to me, if interrogating the version managers explicitly is flakey, then surely interrogating them by proxy (via If it works it works though! |
I've tried with asdf and mise, the one that causes the most issues is asdf
Not really, the issue we had before was that because we didn't have a PATH we'd have to rebuild it, and the apis provided by the version managers were insufficient(for example they'd only give us elixir but not erlang or rust). By getting the PATH in the project directory we can let the os find the asdf shims or the mise installation. It's also a moving target just like shells, asdf in particular changed all its apis in 0.16.
I'm all in for a solution that would let us stop having to worry about this for good |
|
Sounds like this is the best solution 👍 |
|
I see this PR has already landed, but if it helps at all, you can wrap echo "$PATH"
/Users/sbaildon/.local/bin:/nix/var/nix/profiles/default/bin:/Users/sbaildon/.local/share/cargo/binversus echo $PATH
/Users/sbaildon/.local/bin /nix/var/nix/profiles/default/bin /Users/sbaildon/.local/share/cargo/bin |
|
@sbaildon that's a great tip! We can add that in a new PR |
Hey 👋
I spontaneously decided to give it a try to #171 and see if i can fix Fish support. I hope this doesn't bother you, if so, feel free to close the PR.
Fish shell uses space-separated PATH variables internally (as a proper list), while other shells like bash and zsh use colon-separated strings. When Expert tried to detect the Elixir executable by running
echo $PATHin Fish, it received space-separated output like:/opt/homebrew/bin /opt/homebrew/sbin /Users/username/.local/bin ...This caused
:os.find_executable/2to fail since it expects colon-separated paths on Unix systems, preventing Fish users from using Expert properly.Proposed Solution
This PR adds Fish-specific handling to convert the
PATHto the expected colon-separated format:String.contains?(shell, "fish")string join ':' $PATHto convert Fish's list format to colon-separatedstring join ...is built-in in Fish and available since version 2.3.0 (2016)💡 This approach correctly handles paths with spaces in them (e.g.,
/Applications/Visual Studio Code.app/bin) because it works with Fish's native list structure rather than trying to parse space-separated strings.Testing
Tested with Fish shell + Mise on macOS, where Elixir executable is now correctly found 🚀
Let me know if the solution is good enough for you, or if is there anything missing.
Cheers ✌️