Skip to content

Fix fzf with no cmdline arguments on Cygwin#4383

Open
clarity20 wants to merge 1 commit into
junegunn:masterfrom
clarity20:cygwin_build
Open

Fix fzf with no cmdline arguments on Cygwin#4383
clarity20 wants to merge 1 commit into
junegunn:masterfrom
clarity20:cygwin_build

Conversation

@clarity20

Copy link
Copy Markdown

This addresses issue #4382.

In module src/winpty_windows.go, function runWinpty() is handing off the executable's full pathname to runProxy() as the first argument. The path was coming over Windows-style and causing Cygwin to choke. So I'm checking uname -s for Cygwin and calling cygpath to reformat if needed. Very basic test runs now looking good:

fzf test drive

@xrstf

xrstf commented May 18, 2025

Copy link
Copy Markdown

This PR makes fzf work in my Cygwin64 installation on Windows 10.

Comment thread src/winpty_windows.go
argStr := escapeSingleQuote(args[0])
_argStr := ""

osname, _ := exec.Command("uname", "-s").Output()

Copy link
Copy Markdown
Owner

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?

@clarity20 clarity20 May 29, 2025

Copy link
Copy Markdown
Author

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}?

Copy link
Copy Markdown
Owner

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*?

Copy link
Copy Markdown
Author

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.

Copy link
Copy Markdown

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.

Copy link
Copy Markdown
Owner

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

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@junegunn junegunn added windows Windows cygwin Cygwin labels May 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cygwin Cygwin windows Windows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants