Update gocode #305

anjmao · 2018-08-11T19:51:43Z

This PR removes old gocode from nsf and adds gocode from mdempsky. Any cache is not added yet, there are failing unit tests which I'm working on.

keegancsmith · 2018-08-13T19:52:14Z

thanks! Just ping when CI is happy and you want a review.

anjmao · 2018-09-02T13:53:06Z

@keegancsmith

I fixed tests and added some todos for feature work since gocode doesn't support some of them like imports autocomplete, builtin global functions autocomplete...
Some autocomplete tests are failing for go 1.8 since gocode dropped support for go 1.8 mdempsky/gocode@4389b7c Should we consider dropping go 1.8 support in langserver (make sense to me)?

keegancsmith · 2018-09-02T17:00:41Z

Yup dropping support of go1.8 sounds good to me. If I am not mistaken most tooling aims to support the last 2 minor releases. Do you mind changing the travis config to be "1.10.x" and "1.11.x" in this PR?

anjmao · 2018-09-03T06:18:34Z

@keegancsmith I updated travis to drop 1.8, now tests are passing. There is CI problem with AppVeyor, I'm getting error:

exec: "gcc": executable file not found in %PATH%

I never worked with AppVeyor, but I see that it uses global go version. I checked previous successful PR and it was using go version go1.9.2 windows/amd64, now it uses newer go go version go1.11 windows/amd64

I checked more go projects which uses appveyor, they are also failing (https://ci.appveyor.com/project/mholt/caddy)

keegancsmith · 2018-09-04T02:47:03Z

Finally took a look at the PR. You are removing functionality that was added in that is relied on, like SetBuildContext and InitDaemon. You should just be able to keep export.go, I believe that was added to export the functionality we hook into.

I also see there are lots of tests you have now commented out. Anyidea why they are breaking? Could it be due to the change in SetBuildContext, or is it just due to how much this fork of gocode has diverged from upstream?

anjmao · 2018-09-05T13:44:25Z

langserver/completion.go

+		Data:     contents,
+		Cursor:   offset,
+		Source:   !h.config.UseBinaryPkgCache,
+		Context:  gbimporter.PackContext(h.BuildContext(ctx)),


@keegancsmith Thanks for review.

As I'm not using gocode daemon (not starting any net listener) I'm setting build context here. In current master https://github.com/sourcegraph/go-langserver/blob/master/langserver/internal/gocode/export.go#L17 is used only in unit tests.
There are failing integration tests since this gocode fork returns different suggestions for some cases.

Is it possible to update those tests instead of just commenting them out?

Ok great about setting the buildcontext where you set it, that actually makes a lot of sense.

keegancsmith · 2018-09-06T22:28:33Z

If you could update the commented out tests that would be great, then I'll merge in.

anjmao · 2018-09-07T11:35:21Z

Sure I will look at these failing tests, but some tests are failing since gocode doesn't support imports autocomplete, I thought it is better if I add this feature on separate PR or do you want me to add it in this PR?

keegancsmith · 2018-09-07T16:56:57Z

Another PR works for me.

anjmao · 2018-09-10T07:02:46Z

langserver/completion.go

+		Filename: filename,
+		Data:     contents,
+		Cursor:   offset,
+		Builtin:  true,


@keegancsmith I was missing this flag, after adding Builtin: true I was able to fix failing tests except whos with package autocomplete. There is one bug in gocode , if I place dot inside package path I get all builtins. In real world this is not a big deal and this can also be fixed with package autocomplete.

anjmao · 2018-09-12T05:52:41Z

@keegancsmith Can you have a look now, I added Builtin: true flag and updated some tests which was not working before.

lloiser · 2018-09-12T06:25:22Z

langserver/langserver_test.go

+
+		h.config.UseBinaryPkgCache = false
+
+		// TODO(anjmao): autocomplete tests fails if binary cache is used


This settings is by default activated and therefore used by editors.
Do you know why this is happening? Is it because gocode has some problems or is it a problem in the go-langserver?

As gocode is using binary cache for completion if I set this to UseBinaryPkgCache = true a lot of completion tests are failing since autocomplete returns empty result because test case binary is not built yet at that moment. Is there any way I can pre build each test case code during mountFS?

The tests already install the packages:

go-langserver/langserver/langserver_test.go

Lines 1401 to 1409 in 68907e0

// Install all Go packages in the $GOPATH.

oldGOPATH := os.Getenv("GOPATH")

os.Setenv("GOPATH", tmpDir)

out, err := exec.Command("go", "install", "-v", "all").CombinedOutput()

os.Setenv("GOPATH", oldGOPATH)

t.Logf("$ GOPATH='%s' go install -v all\n%s", tmpDir, out)

if err != nil {

t.Fatal(err)

}

Maybe there is another problem that I cannot see at the moment...

If I understand correctly, this is only failing because $GOPATH/pkg would be used when UseBinaryPkgCache = true, and in the case of the tests here we are not actually building the code with go install because the code is not on disk. i.e. this wouldn't actually fail in practice / in real use when UseBinaryPkgCache = true, yeah?

If so, I think it is fine to not address this for now.

Yeah, I am wrong here (I submitted my reply before I saw @lloiser's reply). We are copying the test sources into a temporary GOPATH and go install'ing them, so it should be working

@lloiser Thanks, yes it's actually building code and pkg binary cache files are created during tests, but for some reasons imported packages scope is empty.

In gocode package imports prints empty scope if binary cache is used, but it works perfectly fine in vscode when using binary cache.

pkg.Imports()[0].Scope()

I'm currently investigating go/importer used in gocode.

.travis.yml

anjmao · 2018-09-16T16:26:47Z

@lloiser @keegancsmith I finally found why completion tests was not working with BinaryCache.

Since I'm on macOS my GOOS is darwin, tests was using hardcoded linux. I changed tests to use runtime.GOOS instead.
Gocode has support for gb pased projects and there was a bug with importPaths, which I fixed in separate commit.

I never used https://getgb.io/, but I'm not sure if we should support that. I would rather drop gb support since custom GOPATH is a easy solution to someone who want to have project based workflow.

keegancsmith · 2018-09-17T10:23:42Z

drop gb support

👍

I see only appveyor is failing, but its failing due to unrelated issues.

update gocode

cad92cb

anjmao mentioned this pull request Aug 11, 2018

completion doesn't work with go master #268

Open

Andzej Maciusovic added 3 commits September 2, 2018 16:38

use lang server binary cache config

bbd3c49

add recover for gocode panic

8305a24

update completion tests and add todos

57c86bd

anjmao changed the title ~~wip: update gocode~~ Update gocode Sep 2, 2018

drop go 1.8 support

e54ccd0

lloiser mentioned this pull request Sep 3, 2018

Added simple caching and set the default to use source importer mdempsky/gocode#51

Closed

anjmao commented Sep 5, 2018

View reviewed changes

keegancsmith approved these changes Sep 6, 2018

View reviewed changes

include gocode builtin autocompletion

9c95515

anjmao commented Sep 10, 2018

View reviewed changes

merge with upstream

68907e0

This was referenced Sep 11, 2018

Support for go 1.11 / go modules #316

Open

Getting weird "PANIC" in my go-langserver #317

Closed

lloiser reviewed Sep 12, 2018

View reviewed changes

.travis.yml Show resolved Hide resolved

anjmao and others added 4 commits September 12, 2018 13:08

add go 1.11.x travis build

3d6ca62

remove @ from import path to work on go 1.11

622d511

fix: change incorrect gb relative path formatting

e8fb6d4

fix: do not hardcode GOOS and GOARCH in langserver_test.go

8104bff

use binary cache for completion tests

7b8431e

keegancsmith merged commit 28e52ba into sourcegraph:master Sep 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update gocode #305

Update gocode #305

anjmao commented Aug 11, 2018 •

edited

Loading

keegancsmith commented Aug 13, 2018

anjmao commented Sep 2, 2018

keegancsmith commented Sep 2, 2018

anjmao commented Sep 3, 2018

keegancsmith commented Sep 4, 2018

anjmao Sep 5, 2018

keegancsmith Sep 6, 2018

keegancsmith commented Sep 6, 2018 •

edited

Loading

anjmao commented Sep 7, 2018 •

edited

Loading

keegancsmith commented Sep 7, 2018

anjmao Sep 10, 2018

anjmao commented Sep 12, 2018

lloiser Sep 12, 2018

anjmao Sep 12, 2018

lloiser Sep 12, 2018

slimsag Sep 12, 2018

slimsag Sep 12, 2018

anjmao Sep 13, 2018

anjmao commented Sep 16, 2018

keegancsmith commented Sep 17, 2018


		h.config.UseBinaryPkgCache = false

		// TODO(anjmao): autocomplete tests fails if binary cache is used

	// Install all Go packages in the $GOPATH.
	oldGOPATH := os.Getenv("GOPATH")
	os.Setenv("GOPATH", tmpDir)
	out, err := exec.Command("go", "install", "-v", "all").CombinedOutput()
	os.Setenv("GOPATH", oldGOPATH)
	t.Logf("$ GOPATH='%s' go install -v all\n%s", tmpDir, out)
	if err != nil {
	t.Fatal(err)
	}

Update gocode #305

Update gocode #305

Conversation

anjmao commented Aug 11, 2018 • edited Loading

keegancsmith commented Aug 13, 2018

anjmao commented Sep 2, 2018

keegancsmith commented Sep 2, 2018

anjmao commented Sep 3, 2018

keegancsmith commented Sep 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keegancsmith commented Sep 6, 2018 • edited Loading

anjmao commented Sep 7, 2018 • edited Loading

keegancsmith commented Sep 7, 2018

Choose a reason for hiding this comment

anjmao commented Sep 12, 2018

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

Choose a reason for hiding this comment

anjmao commented Sep 16, 2018

keegancsmith commented Sep 17, 2018

anjmao commented Aug 11, 2018 •

edited

Loading

keegancsmith commented Sep 6, 2018 •

edited

Loading

anjmao commented Sep 7, 2018 •

edited

Loading