-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Fix fzf with no cmdline arguments on Cygwin #4383
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
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any environment variable that shows you're on Cygwin? Something like $CYGWIN
or $CYG*
?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like your idea of having users set up their own envs. In my opinion they should also be reminded to set this value separately in each environment where they want to run fzf
.
FWIW, my research inclines me to think there really isn't a "grand unified" approach through canonical environment or shell variables. See for instance this discussion on Stack Overflow.
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.
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-winpty
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
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 -s
for Cygwin and callingcygpath
to reformat if needed. Very basic test runs now looking good: