Fix fzf with no cmdline arguments on Cygwin#4383
Conversation
|
This PR makes fzf work in my Cygwin64 installation on Windows 10. |
| argStr := escapeSingleQuote(args[0]) | ||
| _argStr := "" | ||
|
|
||
| osname, _ := exec.Command("uname", "-s").Output() |
There was a problem hiding this comment.
So we would have to start a process to determine if we're running Cygwin, but this has a cost. Can there be a lighter way to do it, like checking if an environment variable exists?
There was a problem hiding this comment.
You have a good point. Based on their respective man pages, in bash and zsh we can check that os.Getenv("OSTYPE") == "cygwin", but fish makes no mention of OSTYPE and I don't see a better option than uname in that case. Perhaps that little complication can be ironed out by looking at opts.{Bash,Zsh,Fish}?
There was a problem hiding this comment.
Is there any environment variable that shows you're on Cygwin? Something like $CYGWIN or $CYG*?
There was a problem hiding this comment.
os.Getenv() picks up on OSTYPE but again that leaves the fish shell in the lurch.
However, bash also has a BASH_VERSINFO and I see that BASH_VERSINFO[5]="x86_64-pc-cygwin" in my setup. I am looking for something analogous on fish. If I find it I will keep you updated.
There was a problem hiding this comment.
And telling folks to setup an environment variable on their system would not be an option? In case we cannot find a common pre-existing environment variable, I mean.
There was a problem hiding this comment.
So there is no special environment variable that is guaranteed to be present in Cygwin, right? That's unfortunate.
Just to be clear, IIUC, this issue is caused by the use of winpty, so what happens when you tell fzf not to use it?
fzf --no-winptyThere was a problem hiding this comment.
Maybe this can help. In windows it is almost warrantee that the OS environment variable will exist with the value Windows_NT. It seems to have been there since windows XP and from my experience it is very useful to detect windows https://stackoverflow.com/questions/2788720/environment-variable-to-determine-the-os-type-windows-xp-windows-7.
Then you can check if BASH_VERSION, ZSH_VERSION or FISH_VERSION exists. Each one of them should be available when running the respective shell in interactive mode.
Now, I do not now of any cygwin specific variable but you can check for the existence of MSYSTEM which is set by msys or git bash. If it is not present, it should be very likely that you are running cygwin.
There was a problem hiding this comment.
You can simply check PATH.
When fzf is called from Cygwin, PATH should contain the bin directory of Cygwin for (almost) 100%. I think you also want to check MSYS2 paths because there should be the same problem in MSYS2, which is a fork of Cygwin. MSYS2 is the base system of MinGW and Git for Windows, so its user base isn't negligible. If you care about the cases where Cygwin/MSYS2 is installed in a strange path, you can instead check if cygwin1.dll / msys-2.0.dll / msys-1.0.dll exists in the PATH.
There was a problem hiding this comment.
fzf already has cygwin/msys detection and cygpath for the shell when running other commands, based on checking SHELL (or the first word of --with-shell) for a /. Using the same approach for proxy execution would fix the problem for bash, and for any other shells if the user explicitly exports a SHELL. (It would also be consistent in the sense that if you're having problems with fzf detecting your shell, you can just export SHELL.)
There was a problem hiding this comment.
A further update - it turns out you can just substitute all \\ with / in args[0] before quoting, and it works fine without needing cygpath. This is a reasonable thing to do on Windows since 1) Windows APIs allow forward slash, and 2) on this code path we're invoking sh or bash anyhow, so it's absolutely gonna be cygwin or msys and they want forward slashes. (That is, Cygwin's bash will invoke C:/whatever/fzf.exe just fine, it's C:\whatever\fzf.exe it has a problem with.)
This addresses issue #4382.
In module
src/winpty_windows.go, functionrunWinpty()is handing off the executable's full pathname torunProxy()as the first argument. The path was coming over Windows-style and causing Cygwin to choke. So I'm checkinguname -sfor Cygwin and callingcygpathto reformat if needed. Very basic test runs now looking good: