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

Use upstream definitions, fix gopackagesdriver #4185

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ellie-idb
Copy link

What type of PR is this?

Bug fix
Feature

What does this PR do? Why is it needed?

With some recent changes made, the gopackagesdriver seems to fail to resolve all standard library imports. This fixes that, as well as migrating the driver to use the upstream definitions as defined here: https://pkg.go.dev/golang.org/x/tools/go/packages#hdr-The_driver_protocol.

Which issues(s) does this PR fix?

Fixes #4184
Fixes #3962

Other notes for review

This is absolutely not fully ready to be merged in. I'm willing to take some time to clean this diff up, but would like some guidance on what specifically should be changed / what should be kept. Please let me know, and be patient with me here - it's my first time contributing code to this repo. Thanks <3!

@ellie-idb
Copy link
Author

It looks like this CI failure: https://buildkite.com/bazel/rules-go-golang/builds/7281#019373f5-722b-4160-8e7b-4cdc32943756 is unrelated to this PR. Going to ignore it for now.

@fmeum
Copy link
Member

fmeum commented Dec 2, 2024

Cc @JamyDev

go/runfiles/BUILD.bazel Outdated Show resolved Hide resolved
go/tools/gopackagesdriver/packageregistry.go Outdated Show resolved Hide resolved
@aran
Copy link

aran commented Dec 17, 2024

What else would be needed here to get this landed?

@ellie-idb
Copy link
Author

ellie-idb commented Dec 28, 2024

@aran

What else would be needed here to get this landed?

I'm not sure. At @devzero-inc, we use this patch-set daily, and I know that it definitively works for us, but we also have an entirely cgo-free code base. Earlier, at @fmeum's request, I tried to dig into /why/ specifically CompiledGoFiles isn't getting populated, and experimented with passing -compiled=true to go list... and, well, that compiles basically every single cgo file in the standard library from scratch, invoking the C compiler a bunch of times... and pinning CPU at 100% for 10+ minutes. Not ideal, especially for something that's invoked quite frequently from the IDE. I'm happy to make any more changes as needed, but I haven't necessarily received anything actionable.

@ellie-idb ellie-idb force-pushed the master branch 2 times, most recently from f8f6dba to 7782270 Compare December 28, 2024 20:30
@ellie-idb
Copy link
Author

@JamyDev addressed your comments

@ellie-idb ellie-idb force-pushed the master branch 4 times, most recently from 233b76b to e987ce0 Compare January 20, 2025 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants