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

x/tools/go/packages: document GOPACKAGESDRIVER #34341

Open
jmhodges opened this issue Sep 17, 2019 · 20 comments
Open

x/tools/go/packages: document GOPACKAGESDRIVER #34341

jmhodges opened this issue Sep 17, 2019 · 20 comments

Comments

@jmhodges
Copy link
Contributor

@jmhodges jmhodges commented Sep 17, 2019

There is currently no documentation that shows up on godoc.org for the GOPACKAGESDRIVER environment variable. However, it's very useful for alternative build systems to provide lookups for gopls and other tools. For instance, it's mentioned in the bazel rules_go design doc for editor support.

It'd be nice for GOPACKAGESDRIVER to have publicly available documents.

@toothrot
Copy link
Contributor

@toothrot toothrot commented Sep 17, 2019

This sounds related to https://golang.org/cl/184943, which documents this environment variable in the code: https://github.com/golang/tools/blob/3b4f30a44f3bd52b676ee9e88db10d11971b7363/go/packages/external.go#L19-L28

/cc @matloob who may be able to explain why this is not exposed in the public documentation.

@jmhodges
Copy link
Contributor Author

@jmhodges jmhodges commented Oct 3, 2019

Ping!

@matloob
Copy link
Contributor

@matloob matloob commented Oct 28, 2019

We didn't document GOPACKAGESDRIVER earlier because it wasn't as stable, but I think it's reasonable to add documentation now.

@thepacketgeek
Copy link

@thepacketgeek thepacketgeek commented Dec 17, 2020

@matloob, @jmhodges, @toothrot: I noticed gopackagesdriver is now deprecated (as of gopls 0.5.3)

Do any of you know if there is an alternate path for resolving packages (E.g. Buck) to provide gopls?

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Dec 17, 2020

gopackagesdriver is still supported in go/packages--the reason it was disabled in gopls was because we weren't aware of any gopackagesdrivers that were actually available for use with gopls. If another gopackagesdriver becomes available, we can add back support for it in gopls, though that may be a large undertaking because we have started making assumptions in the codebase.

@thepacketgeek
Copy link

@thepacketgeek thepacketgeek commented Dec 18, 2020

@stamblerre thank you for the clarification! We have a gopackagedriver for gopls so I'm trying to figure out a path forward. The commit above mentions gopls only supporting the go command, does go list offer something similar to gopackagedriver that gopls could use?

(as I ask this I realize that's what this very PR may be about, sorry for my confusion)

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Dec 18, 2020

So the default gopackagesdriver is a go list driver, which is how gopls works by default. If you already have a gopackagesdriver then we'd be happy to work with you on making sure it works with gopls, because there is some logic in gopls that assumes the go list driver.

@thepacketgeek
Copy link

@thepacketgeek thepacketgeek commented Dec 18, 2020

@stamblerre That would be incredible! What can I do to help? If I modify our gopackages driver to behave more similarly to go list, will that make integration easier?

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Dec 21, 2020

That shouldn't be necessary--as long as it produces the correct output it should work. I would guess that we would need to revert https://golang.org/cl/268977 and then confirm whether or not gopls works. I think there will be a number of bugs and edge cases that might cause trouble, so it may take a while to fully diagnose all of the issues.

One thing that will be particularly tricky is our package metadata invalidation logic--for example, Bazel relies on BUILD files, but gopls isn't aware of those files, and so it won't invalidate package metadata when they change.

@thepacketgeek
Copy link

@thepacketgeek thepacketgeek commented Dec 22, 2020

I've reverted that change locally and I'm still not seeing gopls using the external driver. I believe we're currently on 0.5.0.

That sounds similar to Buck and TARGETS files, our gopackagesdriver daemon watches for file changes to update its cache. It's working currently as the driver can return the updated packages with each gopls request.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Dec 22, 2020

I've reverted that change locally and I'm still not seeing gopls using the external driver. I believe we're currently on 0.5.0.

It may be tricky to work with older versions of gopls. Is it possible to use the latest version? Figuring out why it's not working may require diving into the guts of gopls and go/packages a bit. If you can rebuild gopls with some logging, you can check that the correct driver is chosen here: https://github.com/golang/tools/blob/f2e330f49058f692e942445a77c6ee5ab1ca879b/go/packages/packages.go#L253.

It's working currently as the driver can return the updated packages with each gopls request.

The issue is that gopls will not re-request package metadata until something changes in the Go file (currently, a package name or the list of imports). So, if the TARGETS file changes, and that changes the package metadata, gopls won't try to reload it until something changes in the Go file. We have special handling for go.mod files since they're built into go list, but we don't for other gopackagesdrivers.

@thepacketgeek
Copy link

@thepacketgeek thepacketgeek commented Dec 22, 2020

Oh, sorry, I phrased that weirdly.

I believe we're currently on 0.5.0.

This is the version of gopls that we are running currently and it works wonderfully with gopackages driver.

I've reverted that change locally and I'm still not seeing gopls using the external driver.

This testing (reverting the force of GOPACKAGESDRIVER=off) is being done with 0.6.0. I'm noticing that GOPACKAGESDRIVER is still not present in cfg.Enveven with the change reverted.

Even if I modify tool with the bin name of our gopackagesdriver in external.go, the packages driver is not getting called.


gopls won't try to reload it until something changes in the Go file

Got it, is this related to changes since 0.5.0? What I'm seeing in 0.5.0 is that every event for gopls (like hovering, requesting auto-complete, etc) will send a file= query to our gopackagesdriver. Things work out really well this way because our daemon is ready to go with updated package info.


P.S. I really appreciate your time and help with this! gopls has been working really well for us and I'm excited at the thought of preserving the external gopackagesdriver support so we can stay up with the latest improvements 😃

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Dec 23, 2020

Even if I modify tool with the bin name of our gopackagesdriver in external.go, the packages driver is not getting called.

Hm, this may take a bit more debugging then. I will try to look into it when I get a chance, but that may not happen until after February, as we are currently focusing on stability-related fixes for golang/vscode-go#1037. To debug this, I would except you will want to add some logging throughout the logic that populates the environment and confirm that gopls finds the right GOPACKAGESDRIVER. By the way, are you using a binary named gopackagesdriver or are you setting the environment variable?

What I'm seeing in 0.5.0 is that every event for gopls (like hovering, requesting auto-complete, etc) will send a file= query to our gopackagesdriver. Things work out really well this way because our daemon is ready to go with updated package info.

This may actually be the result of a bug -- I'd be curious to see a gopls log if you're able to share one. My guess is that something is preventing gopls from caching the data, probably because it doesn't do the right thing when there is a GOPACKAGESDRIVER present.

@thepacketgeek
Copy link

@thepacketgeek thepacketgeek commented Dec 23, 2020

are you using a binary named gopackagesdriver or are you setting the environment variable?

GOPACKAGESDRIVER environment variable (although I'll test out with a named binary also, that could simplify things)

My guess is that something is preventing gopls from caching the data

Ah, very fortuitous then in this case! This bug(?) is what prompted us to build a daemon mode for our gopackagesdriver, and seems like the only reason it works at all :)


I'll keep digging in and see what I can do to get 0.6.0 working with our go packagesdriver and hopefully share some of that here. Thanks again for the pointers!

@ribrdb
Copy link
Contributor

@ribrdb ribrdb commented Dec 31, 2020

I'm planning to work on a gopackagesdriver implementation for bazel soon, and would also like to make sure it works with gopls.

For the metadata invalidation, would it help if we included the BUILD or TARGETS files in Package.OtherFiles so you know what to watch?

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Dec 31, 2020

I'm planning to work on a gopackagesdriver implementation for bazel soon, and would also like to make sure it works with gopls.

That's awesome! I'm sure you've already seen bazelbuild/rules_go#512, but I believe some work has been done on this in the past.

For the metadata invalidation, would it help if we included the BUILD or TARGETS files in Package.OtherFiles so you know what to watch?

That might be a start, but the issue is a little bit deeper than that. Right now, we avoid excessive calls to the gopackagesdriver by assuming that go/packages results won't change unless a package name or import list changes, but that's not true in the case of Bazel or other build systems. This may require a more significant modification to the go/packages API to indicate "file changes that cause invalidation"--go.mod files actually fall into this same category, but we handle them explicitly in gopls.

@ribrdb
Copy link
Contributor

@ribrdb ribrdb commented Jan 14, 2021

What's the best place to ask questions about implementing the gopackagesdriver? E.g:

  • Does gopls expect package IDs to be in a particular format? I'd like to use bazel target names since theoretically there could be multiple packages with the same import path.
  • Something (not sure if its gopls and/or the vscode plugin) is finding extraneous files in the bazel-* symlinks. How can I stop that? For example, sometimes gopls sends me a query for each of the 6000+ files under bazel-{workspace}/external/go_sdk.
    Also, vscode shows problems for every go file under bazel-bin. However in my packages I listed those files with the bazel-bin symlink de-referenced.
@ribrdb
Copy link
Contributor

@ribrdb ribrdb commented Jan 15, 2021

Ok, fixing other problems resolved my second issue, and gopls seems to be working with bazel names for package ids.
One issue I've found is in isTestMain: the bazel generated tests are not in to go build cache.

@matttproud
Copy link
Contributor

@matttproud matttproud commented Feb 22, 2021

If another gopackagesdriver becomes available, we can add back support for it in gopls, though that may be a large undertaking because we have started making assumptions in the codebase

@stamblerre, @matloob, et al., would you be able to enumerate these assumptions concretely?

Motivation: We are interested in using a custom GOPACKAGESDRIVER for Blaze integration, which I expect would dovetail (to a limited extent) with Bazel. I just tested out overriding GOPACKAGESDRIVER=/bin/false, and it appears that recent gopls releases simply stub out the setting.

@ribrdb
Copy link
Contributor

@ribrdb ribrdb commented Mar 1, 2021

@matttproud Here's what I've found with my bazel driver:

  • Need to revert https://golang.org/cl/268977
  • Need to update isTestMain in load.go to recognize bazel generated test mains
  • gopls doesn't really understand dependencies on non-go files, so it doesn't update when you edit a BUILD or .proto file for example

It also seems you have to configure the editor to ignore things like bazel-out and bazel-bin/**/.runfiles.
Otherwise gopls can go cpu crazy scanning files under the bazel-
symlinks.

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

Successfully merging a pull request may close this issue.

None yet
8 participants