don't default to binaries in the PATH #823
Conversation
| if executable(binpath) | ||
| return binpath | ||
| endif | ||
|
|
There was a problem hiding this comment.
Any reason you removed this? This shouldn't be removed.
There was a problem hiding this comment.
Yeah, maybe this isn't the best way to do this, but, without that it always uses what's present in $PATH, regardless of the ordering changes. Maybe we should move the let $PATH line up above this block?
There was a problem hiding this comment.
I think it should use it from PATH. Can you please change it back? PATH is universal and it's ok to read it from there. It also makes the logic clear.
There was a problem hiding this comment.
Did you see my updated changes?
|
Hi @luan The only reason is that PATH always comes first. Other than this there is no other significant reason. You have a valid concenr and I'm ok changing the order of the binary paths. |
|
@fatih take a look at the updated changes, I added the temporary |
| " append our GOBIN and GOPATH paths and be sure they can be found there... | ||
| " let us search in our GOBIN and GOPATH paths | ||
| let old_path = $PATH | ||
| let $PATH = go_bin_path . go#util#PathListSep() . $PATH |
There was a problem hiding this comment.
This is wrong imho. This variable needs to be fetched first below. I think you need to revisit all the logic here?
There was a problem hiding this comment.
Yes, it is, sorry. I just realized I had my rtp set to something else when I was doing this. pushing a fish now.
|
@luan Can you please check the logic. The variable |
|
@fatih yeah sorry. I moved those lines up. What do you mean I need to change it though? |
| @@ -139,26 +139,29 @@ function! go#path#CheckBinPath(binpath) | |||
| " remove whitespaces if user applied something like 'goimports ' | |||
| let binpath = substitute(a:binpath, '^\s*\(.\{-}\)\s*$', '\1', '') | |||
|
|
|||
| " check if we have an appropriate bin_path | |||
| let go_bin_path = go#path#BinPath() | |||
There was a problem hiding this comment.
Can you please handle the case when this is empty (it can be). You should handle it here, not below.
|
@fatih handled that case by not changing the |
| " append our GOBIN and GOPATH paths and be sure they can be found there... | ||
| " let us search in our GOBIN and GOPATH paths | ||
| let $PATH = go_bin_path . go#util#PathListSep() . $PATH | ||
| endif |
There was a problem hiding this comment.
Ok I didn't quite get this. If go_bin_path is empty, this means we're going to prepend an empty string to PATH. Why do we prepend an empty string ? We shouldn't do that :)
|
@fatih yes... Fixed that. |
|
Thanks @luan 👍 |
I asked this on #188 but since that's a closed PR I feel like it's less likely to receive attention so I'm creating a new issue instead.
I have to start this with the question: is there any particular reason for append instead of prepend? This is causing issues if people have some other binary in their path with a colliding name, because appending makes our gobin the last lookup path rather than the first.
I'm getting several reports of people hitting this issue because they had tools compiled on 1.6 and were hanging out in their
$GOPATH/binand now that we're usingg:go_bin_paththat is not getting updated, short term fix for us is just removing those stale binaries but seems like a prepend to the$PATHwould be appropriate.I made this as a PR because this may be the right solution. I didn't think through all the edge cases here because I wanted to just get opinions on this first. It seems like surprising behavior to me that when I configure
g:go_bin_paththe$PATHstill overrides it.Thoughts?