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

:GoInstall error navigation seems broken #404

Closed
daaku opened this issue Apr 29, 2015 · 16 comments · Fixed by #410
Closed

:GoInstall error navigation seems broken #404

daaku opened this issue Apr 29, 2015 · 16 comments · Fixed by #410

Comments

@daaku
Copy link

daaku commented Apr 29, 2015

Haven't figure out what exactly is happening, but this was working fine until I upgraded yesterday. Steps to reproduce:

  1. Start vim in a say $HOME
  2. Open in go file say $HOME/go/src/foo/foo.go
  3. :GoInstall foo/..
  4. If there is a error from the GoInstall in $HOME/go/src/bar/bar.go you'll see it listed but won't be opened as it used to be.
@fatih
Copy link
Owner

fatih commented Apr 29, 2015

Hi @daaku. Weird thing indeed. The thing is there is no code that changed :GoInstall since the previous release. If there is any error, :GoInstall calls cwindow which opens the quick fix list and shows all errors. It doesn't jump to it though, you need to call :cc. In your case, you want to jump to it, just like :GoBuild right ?

@daaku
Copy link
Author

daaku commented Apr 29, 2015

It's getting the absolute paths wrong, :cc doesn't work either. :GoBuild does in fact get them right.

@fatih
Copy link
Owner

fatih commented Apr 29, 2015

It's so hard to test this without a proper test files. I've tried in my current GOPATH with several other packages, and in all of them it was working. Is there a way you could paste me a self contained go code so I could directly reproduce it ?

@fatih
Copy link
Owner

fatih commented Apr 29, 2015

I also wonder if this is due: 0d1e8c9 But again there is no way I can test it. Pinging @mattn may be he has some idea. I couldn't reproduce it (waiting for your example files)

@mattn
Copy link
Contributor

mattn commented Apr 30, 2015

I'll look into it later

@fatih
Copy link
Owner

fatih commented Apr 30, 2015

@daaku can you test with #405 please ?

@daaku
Copy link
Author

daaku commented Apr 30, 2015

I pulled but it still fails for me :/ Here's a repro case:

# setup a test case
export GOPATH=$PWD/tmp
mkdir -p $GOPATH/src/foo
mkdir -p $GOPATH/src/bar
cat << 'EOF' > $GOPATH/src/foo/foo.go
package foo

import "fmt"

func Do() {
  fmt.Println("hello from foo")
}
EOF
cat << 'EOF' > $GOPATH/src/bar/bar.go
package main

import (
  "fmt"
  "foo"
)

func main() {
  foo.Do()
  fmt.Println("hello from bar")
}
EOF
go install -v ...
$GOPATH/bin/bar

# now break the code so go install fails
cat << 'EOF' > $GOPATH/src/foo/foo.go
package foo

func Do() {
  fmt.Println("hello from foo")
}
EOF
go install -v ...

# open this and run :GoInstall
vim $GOPATH/src/bar/bar.go

@fatih
Copy link
Owner

fatih commented Apr 30, 2015

Thanks @daaku. Will look at it in the morning.

fatih added a commit that referenced this issue May 3, 2015
We need to cd into the current files directory. This is important so
namemodify does create a full path for outputs when the token is only a
ingle file name (such as for a go test output, i.e.: 'demo_test.go').
For ther outputs, such as 'go install' we already get an absolute path
i.e.: '../foo/foo.go') and fnamemodify successfuly creates the full
ath.)

Fixes #404,#407
@fatih
Copy link
Owner

fatih commented May 3, 2015

Thanks @daaku I can now reproduce it. I've fixed it with #410. Can you please try it? Now it successfully parses the error location and put's into the quickfix list. Note that :GoInstall doesn't have an autojump to error like the others (just noticed it). So you need to call manually :cc. That is also fixed with the PR: #411. Please let me know how it works for you, I've really tried to cover all cases this time.

@daaku
Copy link
Author

daaku commented May 3, 2015

Really appreciate the effort @fatih! The path logic still isn't working -- it's getting the correct filename, but not the directory.

@fatih
Copy link
Owner

fatih commented May 3, 2015

It will show the file name but if you call :cc you should go the correct definition. Do you mean it should show the full path rather the file name? Or do I miss something?

@fatih
Copy link
Owner

fatih commented May 3, 2015

Here is a screenshot of the fixed version of your code:

image

@daaku
Copy link
Author

daaku commented May 3, 2015

I'm not getting the same behavior. I setup a stripped vimrc to make sure there isn't anything interfering which looks like this:

set nocompatible
call plug#begin('~/.vim/plugged')
Plug 'fatih/vim-go'
call plug#end()

(I'm using https://github.com/junegunn/vim-plug but I don't think that should matter.)

The vim-go I'm using is at rev 22544ec.

When I run :GoInstall my quickfix shows:

/home/me/src/go/src/bar/foo.go|4| undefined: fmt

So it's getting the foo.go and the line/error correct, but the directory is bar instead of foo presumably because I ran :GoInstall while editing a file in the bar directory. The rest of the instructions for the repro are the same as before.

@fatih
Copy link
Owner

fatih commented May 8, 2015

This time I really couldn't reproduce it :/ With the fix at #410 it works perfect. I've also tried with the your simple vimrc. Here is a screencast showing it: http://quick.as/4ovsbqqm

I'm merging #410. Please pull master again. I think it's because you didn't try with #410 may be?

@fatih fatih closed this as completed in #410 May 8, 2015
@fatih fatih reopened this May 8, 2015
@daaku
Copy link
Author

daaku commented May 12, 2015

It's fixed! Thank you very much @fatih!

@daaku daaku closed this as completed May 12, 2015
@fatih
Copy link
Owner

fatih commented May 12, 2015

Finally 👍 :)

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

Successfully merging a pull request may close this issue.

3 participants