Skip to content
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

Path issues on :GoDef in Windows #1082

Closed
pbsf opened this issue Oct 13, 2016 · 13 comments
Closed

Path issues on :GoDef in Windows #1082

pbsf opened this issue Oct 13, 2016 · 13 comments

Comments

@pbsf
Copy link

pbsf commented Oct 13, 2016

I have been using vim-go on Windows for a while without any major issues. Today I decided to update my vim plugins (dumb me), and I started to receive the following when when calling :GoDef:

vim-go: /usr/bin/bash: C:toolsgobinguru.EXE: command not found

The file is in /c/tools/go/bin/guru.exe, which means the slashes are not being used correctly on the Windows environment. Any tips on which file I should look into to fix this? I can provide a PR in case it works.

VIM Version: 7.4
vim-go Version: 1.9 - d9fa1bc

Edit: On version 1.6 of vim-go I don't have this issue. From 1.7-1.9, I do.

@jsfaint
Copy link
Contributor

jsfaint commented Oct 31, 2016

Maybe you want to see #1013

@fatih
Copy link
Owner

fatih commented Nov 21, 2016

Hi @pbsf

Can you try this with the latest master again? I don't use windows, so not sure how to fix it. Probably we need to escape something in def.vim. However this is used by many people using Windows. I wonder why it's not reported by them as well.

@fatih fatih added the windows label Nov 21, 2016
@jsfaint
Copy link
Contributor

jsfaint commented Nov 21, 2016

@fatih This issue is same with #1013
If they don't use vimproc, this issue is not exist.
With your latest commit dc80733, this issue is gone.
Thanks

@fatih
Copy link
Owner

fatih commented Nov 22, 2016

Got it. Thanks @jsfaint 👍

@pbsf are you using vimproc? If yes can you please update to latest master and test again. Waiting for an answer from you.

@pbsf
Copy link
Author

pbsf commented Nov 23, 2016

Hi @fatih,

I'm not using vimproc. The Vim error message is:

vim-go: /usr/bin/bash: C:Gobinguru.EXE: command not found

Therefore it's not similar to #1013.

I updated to master(d3c4fb3), and the error still happens. I'm using VIM 7.4 with LUA.

Vim-go works just fine when I checkout the git hash 6e1072e. I read about the new features I'm eager to use it. Just let me know if you want me to run more tests. Thanks.

@fatih
Copy link
Owner

fatih commented Nov 23, 2016

I've doubled check the code of def.vim and it's very similiar to other we use in other commands. I wonder why especially this fails. I can't do much for you unfortunately as I don't use windows. Someone who uses windows (maybe @mattn?) can solve this.

@mattn
Copy link
Contributor

mattn commented Nov 23, 2016

Anyone, please try this?

diff --git a/autoload/go/def.vim b/autoload/go/def.vim
index 41464fe..725ffcc 100644
--- a/autoload/go/def.vim
+++ b/autoload/go/def.vim
@@ -25,7 +25,7 @@ function! go#def#Jump(mode)
       let $GOPATH = old_gopath
       return
     endif
-    let command = printf("%s -f=%s -o=%s -t", bin_path, fname, go#util#OffsetCursor())
+    let command = printf("%s -f=%s -o=%s -t", shellescape(bin_path), shellescape(fname), go#util#OffsetCursor())
     let out = go#util#System(command)
 
     " append the type information to the same line so our
@@ -59,7 +59,7 @@ function! go#def#Jump(mode)
     endif
 
     let fname = shellescape(fname.':#'.go#util#OffsetCursor())
-    let command = printf("%s %s definition %s", bin_path, flags, fname)
+    let command = printf("%s %s definition %s", shellescape(bin_path), flags, shellescape(fname))
 
     if &modified
       let out = go#util#System(command, in)

@fatih
Copy link
Owner

fatih commented Nov 23, 2016

Oh I see, I think the guru command also needs to be escaped if it's not executed inside the job:

diff --git a/autoload/go/def.vim b/autoload/go/def.vim
index bc226af..b72e8cc 100644
--- a/autoload/go/def.vim
+++ b/autoload/go/def.vim
@@ -71,6 +71,7 @@ function! go#def#Jump(mode) abort
       return
     endif

+    let cmd[0] = shellescape(cmd[0])
     let command = join(cmd, " ")
     if &modified
       let out = go#util#System(command, stdin_content

@tdilo
Copy link
Contributor

tdilo commented Nov 23, 2016

@pbsf, which environment are you running Vim in? It doesn't look like a native Windows build as it's calling /usr/bin/bash.

I am currently working on getting vim-go to run in the shells provided by Git for Windows and MSYS2. The issue you are experiencing, in which the external tools cannot be called due to missing escaping, has not occurred in either of the two on my machine and the native Windows build worked well so far.

In my case (in MSYS2 or Git Bash from Git for Windows), the command line tools are launched properly but receive command line arguments in Unix path notation, which does not work with the native windows binaries installed by go get. This happens because Vim in both these environments is pretty certain that it is running on a Unix system. Therefore it also expects the path names in the tools' output to be in Unix notation, whereas it currently receives Windows-style path names and breaks horribly.

My current work in progress branch is https://github.com/tdilo/vim-go/tree/fix-msys2 - I did some spot checks and most tools already work with the changes I made. I am trying to make sure that my changes work in

  • native Windows builds (Vim 8.0)
  • MSYS2 (with Vim 8.0)
  • Git for Windows (which is based on MSYS2 but still ships Vim 7.4), hence using different code paths

I still need to verify that I did not break Cygwin (does it actually work?) or anything else before opening a pull request in the next few days. In any case it's a huge combinatorial mess with all the slightly differing options to run Vim on Windows.

EDIT: Git for Windows does ship Vim 8 by now.

@fatih
Copy link
Owner

fatih commented Nov 24, 2016

@tdilo this seems like to be great improvement overall for many users. We also have a PR open that improves the situation in cygwin #1092

@fatih
Copy link
Owner

fatih commented Nov 25, 2016

@pbsf can you please report back with the fixes from @mattn and the one I've added?

@pbsf
Copy link
Author

pbsf commented Nov 25, 2016

@fatih, adding both fixes solved my issue. Thanks a lot! Feel free to close the issue.

@fatih fatih added the bug label Nov 27, 2016
@fatih fatih closed this as completed Apr 2, 2017
@fatih
Copy link
Owner

fatih commented Apr 2, 2017

Closed due @pbsf's comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants