x/tools/go/packages: document GOPACKAGESDRIVER
jmhodges opened this issue · 21 comments
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.
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.
Ping!
We didn't document GOPACKAGESDRIVER earlier because it wasn't as stable, but I think it's reasonable to add documentation now.
@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?
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.
@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)
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.
@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?
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.
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.
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.
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 😃
Even if I modify
toolwith 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.
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!
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?
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.
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.
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.
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.
@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.
Change https://go.dev/cl/547977 mentions this issue: go/packages: improve docs of 'go list' and GOPACKAGESDRIVER