Remove g:go_autodetect_gopath #1525

arp242 · 2017-10-19T00:01:43Z

Follow-up to #1461.

Needs a bit more testing.

arp242 · 2017-10-19T00:02:20Z

autoload/go/complete.vim

-  let old_gopath = $GOPATH
-  let old_goroot = $GOROOT
-  let $GOPATH = go#path#Detect()
-  let $GOROOT = go#util#env("goroot")


Not 100% sure about removing this particular case though. Need to carefully read that issue.

this is needed for Go upgrades. Unfortunately gocode embeds the goroot into the binary instead of looking from env, after a Go upgrade gocode can be broken if the user has a version based path. i.e: /usr/lib/go/1.9.1/src.

We should keep the goroot behavior but remove the gopath change.

bhcleek · 2017-10-19T00:32:19Z

I like getting rid of all the code that this eliminates, and I agree that setting $GOPATH using events is better. But I'm not sure we should break people like this. Why not go ahead and add support for the events?

We'd need to handle a number of events:

BufNewFile
BufReadPre
BufFilePre
BufEnter

arp242 · 2017-10-19T00:47:34Z

Wouldn't just the FileType autocommand be enough?

bhcleek · 2017-10-19T01:54:15Z

No, because environment variable are global within a Vim session. If two files with different detected gopaths were open, the last one to be opened would set the gopath that would take effect when running commands in the first unless $GOPATH were set when changing windows and buffers.

arp242 · 2017-10-19T01:54:56Z

Oh, right. That makes sense 👍

arp242 · 2017-11-04T09:54:32Z

Okay, I added it back as autocommands.

I think just the BufEnter and BufLeave should be enough? That seems to work well in my tests anyway.

bhcleek

I've started looking at this and testing it. I want to spend a bit more time with it, especially making sure that the autocmd events are complete.

bhcleek · 2017-11-06T01:14:04Z

autoload/go/complete.vim

+          \ join(a:args)
+          \ )
+
+    let result = go#util#System(cmd)


This seems like a good time to convert this to go#util#Exec, but I won't object if you don't want to do it now.

I'd rather not, since much of this isn't tested and can introduce regressions. Unless we're very careful we may have issues with double shell quoting for example.

I want to add a test for every tool which tests stuff like filenames with spaces, directories that don't exist, etc. and also fix whatever is needed (add build tags, use #Exec(), and whatever else might be needed). This is why I made the changes to the testing script and removed all the GOPATH detection.

arp242 · 2017-11-17T00:08:02Z

I've started looking at this and testing it. I want to spend a bit more time with it, especially making sure that the autocmd events are complete.

Don't want to push you, but did you have a chance to look at this?

I tested with a whole bunch of different scenarios, and it seemed to work well in all of them.

bhcleek · 2017-11-17T00:17:29Z

Not yet. I should be able to get to it by next week (though probably this weekend).

bhcleek

This mostly looks great. Thanks @Carpetsmoker.

I tested a couple of different ways.
Besides testing this PR as is, I also tested the firing of events: I added

autocmd BufEnter * echom "bufenter"
autocmd BufLeave * echom "bufleave"

to my vimrc and then I opened three windows. Two of the windows had the same buffer loaded, and the third window had a second buffer loaded. I then opened a new tab with a third buffer loaded. Finally, I moved between the windows and did a variety of moving between the windows, tabs, loading other buffers (for both new and existing files), splitting, and running :bufdo. After each test I checked :messages to make sure it contained what I expected. Before each test I ran :messages clear to make sure the messages after the next test would contain only the messages caused by entering and leaving the buffers for that test.

Among other things that I documented inline,
I also noticed that if $HOME/src exists, and the user is someplace in$HOME/go/src/ with g:go_autodetect_gopath on and GOPATH unset , $HOME and $HOME/src get added to $GOPATH it's not clear to me if that's an existing issue or not; I'll leave it to you to figure out if it's an existing issue and whether or not to fix it in this PR if it is. If it is introduced by this PR, then I'd ask that it be fixed before merging this PR.

bhcleek · 2017-11-19T01:12:04Z

plugin/go.vim

+        \  if get(g:, 'go_autodetect_gopath', 0)
+        \|   let $GOPATH = b:old_gopath
+        \| endif
+augroup end


In the rare case when a user changes g:go_autotdetect_gopath, BufLeave will not behave as expected. Instead of if get(g: 'go_autodetect_gopath', 0), if exists(b:old_gopath) will guard restoring GOPATH correctly. We should also unset b:old_gopath...

Good point; I've updated the PR with this issue and the other small issues you found.

bhcleek · 2017-11-19T01:23:02Z

plugin/go.vim

+        \| endif
+  autocmd BufLeave *.go
+        \  if get(g:, 'go_autodetect_gopath', 0)
+        \|   let $GOPATH = b:old_gopath


(nit) When the old GOPATH was not set, restoring GOPATH to an empty string does not faithfully restore the state to what it was. As a pragmatic matter, all the go tooling will handle this case correctly, but users may have other scripts, etc. that do not. As I see it, vim-go should restore GOPATH to its state prior to entering the buffer.

bhcleek · 2017-11-19T01:34:23Z

plugin/go.vim

@@ -260,6 +260,16 @@ augroup vim-go
  " in the same window doesn't highlight the most recently matched
  " identifier's positions.
  autocmd BufWinEnter *.go call go#guru#ClearSameIds()
-augroup END
+
+  autocmd BufEnter *.go


I saw some cases where BufEnter gets fired twice, but I was unable to duplicate it when I tried to.

In any event, it caused me to realize that if vim fires BufEnter twice, the second event will cause b:old_gopath to be overwritten with the current $GOPATH, and then when the buffer is left, BufLeave cannot assign the original value of GOPATH. Can we add protection for this case by only checking g:go_autodetect_gopath when if !exists('b:old_gopath')

bhcleek · 2017-11-24T16:42:49Z

Did you mean to duplicate the changelog message in BACKWARDS INCOMPATIBILITIES and IMPROVEMENTS?

arp242 · 2017-11-24T17:25:16Z

Oops, no; that was just a merge error.

bhcleek · 2017-11-25T05:53:48Z

Don't forget to unlet b:old_gopath in the BufLeave after restoring `$GOPATH, too.

And this still didn't handle the situation I mentioned in #1525 (comment)

arp242 · 2017-11-25T06:02:16Z

Don't forget to unlet b:old_gopath in the BufLeave after restoring `$GOPATH, too.

Ah, right. Fixed.

And this still didn't handle the situation I mentioned in #1525 (comment)

I believe it should, as it checks for if get(b:, 'old_gopath', '') isnot ''.

bhcleek · 2017-11-25T15:56:51Z

I believe it should, as it checks for if get(b:, 'old_gopath', '') isnot ''.

On my system, GOPATH is not set, so in BufEnter, let b:old_gopath = $GOPATH will set b:old_gopath to the empty string., then in BufLeave, instead of unlet $GOPATH, GOPATH is set to the empty string using let $GOPATH = b:old_gopath. Unfortunately, Vim doesn't seem to have a way to unset an environment variable: vim/vim#1116.

Can you add a comment in the BufLeave, though, documenting this edge case?

Also, now that unlet b:old_gopath is used in BufLeave, its guard should use if exists(b:old_gopath), otherwise for people whose GOPATH is the empty string (which will be everyone after the BufLeave executes after BufEnter where GOPATH was not set!), GOPATH will never be set to the empty string.

Deals better with some errors, add some commandline flags, and improve output. It will now also run tests from all directories, instead of the autoload directory. I wanted to add a test for #1525, and putting it in `autoload/go/` seemed wrong to me. Would also be nice to be able to run just one test with e.g. `-r tags_test.vim:Test_Foo`. I'll add that when I need it I guess. Regular: [~/.vim/pack/plugins/start/vim-go](master)% ./scripts/test vim-8.0 ok tags_test.vim 0.146627s / 2 tests ok fillstruct_test.vim 0.685770s / 1 tests ok tool_test.vim 0.083538s / 2 tests ok def_test.vim 0.018329s / 2 tests ok fmt_test.vim 0.023622s / 3 tests All tests PASSED Test failure: [~/.vim/pack/plugins/start/vim-go](master)% ./scripts/test vim-8.0 ok tags_test.vim 0.117528s / 2 tests ok fillstruct_test.vim 0.772903s / 1 tests ok tool_test.vim 0.043084s / 2 tests --- FAIL Test_jump_to_declaration_guru (0.000698s) function Test_jump_to_declaration_guru line 13: Expected 'oh' but got 'noes' FAIL def_test.vim 0.007922s / 2 tests ok fmt_test.vim 0.021428s / 3 tests Some tests FAILED Syntax error (would prevously keep Vim open!): [~/.vim/pack/plugins/start/vim-go](test)% ./scripts/test vim-8.0 ok tags_test.vim 0.169554s / 2 tests --- FAIL Test_fillstruct (0.845644s) Vim:E492: Not an editor command: asd FAIL fillstruct_test.vim 0.846325s / 1 tests ok tool_test.vim 0.060288s / 2 tests ok def_test.vim 0.023219s / 2 tests ok fmt_test.vim 0.026293s / 3 tests Some tests FAILED Verbose: [~/.vim/pack/plugins/start/vim-go](master)% ./scripts/test -v vim-8.0 === RUN Test_add_tags --- PASS Test_add_tags (0.084296s) === RUN Test_remove_tags --- PASS Test_remove_tags (0.086975s) ok tags_test.vim 0.172008s / 2 tests === RUN Test_fillstruct --- PASS Test_fillstruct (0.792921s) ok fillstruct_test.vim 0.793609s / 1 tests === RUN Test_ExecuteInDir --- PASS Test_ExecuteInDir (0.044198s) === RUN Test_ExecuteInDir_nodir --- PASS Test_ExecuteInDir_nodir (0.003989s) ok tool_test.vim 0.048599s / 2 tests === RUN Test_jump_to_declaration_godef --- PASS Test_jump_to_declaration_godef (0.006618s) === RUN Test_jump_to_declaration_guru --- FAIL Test_jump_to_declaration_guru (0.000832s) function Test_jump_to_declaration_guru line 13: Expected 'oh' but got 'noes' FAIL def_test.vim 0.007953s / 2 tests === RUN Test_goimports --- PASS Test_goimports (0.013912s) === RUN Test_run_fmt --- PASS Test_run_fmt (0.006824s) === RUN Test_update_file --- PASS Test_update_file (0.000501s) ok fmt_test.vim 0.021532s / 3 tests Some tests FAILED Running just one test file: [~/.vim/pack/plugins/start/vim-go](master)% ./scripts/test -r tags_test.vim vim-8.0 ok tags_test.vim 0.093540s / 2 tests All tests PASSED

Follow-up to #1461. Needs a bit more testing.

arp242 · 2017-11-25T21:21:20Z

On my system, GOPATH is not set, so in BufEnter, let b:old_gopath = $GOPATH will set b:old_gopath to the empty string
GOPATH is set to the empty string using let $GOPATH = b:old_gopath

How would this happen? Right now the code is:

  autocmd BufLeave *.go
        \  if get(b:, 'old_gopath', '') isnot ''
        \|   let $GOPATH = b:old_gopath
        \| endif

So I don't see how it will ever set $GOPATH to an empty string?

bhcleek · 2017-11-25T21:50:42Z

Load vim with GOPATH unset or null and enable GOPATH autodetection:

$> GOPATH= vim
:let g:go_autodetect_gopath = 1

Then edit two files:

:e  $HOME/src/gb-project/bar.go
:e $HOME/go/src/quux/foo.go

When bar.go was entered, its b:old_gopath was set to the empty string and GOPATH was set to the detected value. When bar.go was left, it should have restored GOPATH to the empty string, but did not, because b:old_gopath is empty. The mere existence of b:old_gopath is what should trigger the restoration of GOPATH, not the value of b:old_gopath.

arp242 · 2017-11-25T22:12:05Z

Okay; I pushed an update which should handle all cases I think. We can't unlet $GOPATH but exists('$GOPATH') does work correct.

bhcleek · 2017-11-25T22:18:30Z

LGTM, but setting b:old_gopath to -1 and checking for that value is extraneous.

arp242 · 2017-11-25T22:20:48Z

LGTM, but setting b:old_gopath to -1 and checking for that value is extraneous.

Without that it will run let $GOPATH = go#path#Detect() on a second BufEnter event as exists('b:old_gopath') is false. Not a huge deal, but better to not do it, I think.

arp242 commented Oct 19, 2017

View reviewed changes

arp242 added wip and removed wip labels Oct 30, 2017

bhcleek reviewed Nov 6, 2017

View reviewed changes

bhcleek self-requested a review November 6, 2017 01:20

bhcleek mentioned this pull request Nov 13, 2017

Godeps/gb aware tools #425

Closed

bhcleek requested changes Nov 19, 2017

View reviewed changes

arp242 mentioned this pull request Nov 25, 2017

Some small test running improvements #1584

Merged

arp242 added 6 commits November 25, 2017 20:37

Remove g:go_autodetect_gopath

0e02dac

Follow-up to #1461. Needs a bit more testing.

Put it back for gocode

1427ce1

Move GOPATH detection to autocmd

ac14337

Fix PR feedback from Billy

b769947

unlet b:old_gopath

66a2d80

Add basic test

bb2c53d

arp242 added 2 commits November 25, 2017 21:24

Update changelog

b34e38a

Hmm, forgot I already added a test for this.

b4a8b05

Distinguish between GOPATH= and GOPATH never being set

ab4d0c4

shorter

a7b23c6

arp242 merged commit 974b289 into fatih:master Nov 25, 2017

arp242 deleted the gopath-detect branch November 25, 2017 23:41

arp242 mentioned this pull request Apr 7, 2018

don't set GOROOT #1763

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove g:go_autodetect_gopath #1525

Remove g:go_autodetect_gopath #1525

arp242 commented Oct 19, 2017

arp242 Oct 19, 2017

fatih Oct 20, 2017

fatih Oct 20, 2017

bhcleek commented Oct 19, 2017

arp242 commented Oct 19, 2017

bhcleek commented Oct 19, 2017

arp242 commented Oct 19, 2017

arp242 commented Nov 4, 2017

bhcleek left a comment

bhcleek Nov 6, 2017

arp242 Nov 6, 2017

arp242 commented Nov 17, 2017

bhcleek commented Nov 17, 2017

bhcleek left a comment

bhcleek Nov 19, 2017

arp242 Nov 23, 2017

bhcleek Nov 19, 2017

bhcleek Nov 19, 2017

bhcleek commented Nov 24, 2017

arp242 commented Nov 24, 2017

bhcleek commented Nov 25, 2017

arp242 commented Nov 25, 2017

bhcleek commented Nov 25, 2017

arp242 commented Nov 25, 2017

bhcleek commented Nov 25, 2017

arp242 commented Nov 25, 2017

bhcleek commented Nov 25, 2017

arp242 commented Nov 25, 2017

Remove g:go_autodetect_gopath #1525

Remove g:go_autodetect_gopath #1525

Conversation

arp242 commented Oct 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhcleek commented Oct 19, 2017

arp242 commented Oct 19, 2017

bhcleek commented Oct 19, 2017

arp242 commented Oct 19, 2017

arp242 commented Nov 4, 2017

bhcleek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arp242 commented Nov 17, 2017

bhcleek commented Nov 17, 2017

bhcleek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhcleek commented Nov 24, 2017

arp242 commented Nov 24, 2017

bhcleek commented Nov 25, 2017

arp242 commented Nov 25, 2017

bhcleek commented Nov 25, 2017

arp242 commented Nov 25, 2017

bhcleek commented Nov 25, 2017

arp242 commented Nov 25, 2017

bhcleek commented Nov 25, 2017

arp242 commented Nov 25, 2017