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

Merged
merged 1 commit into from
Feb 18, 2025

Conversation

ellie-idb
Copy link
Contributor

@ellie-idb ellie-idb commented Nov 28, 2024

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

@ellie-idb
Copy link
Contributor 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
Contributor 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
Contributor Author

@JamyDev addressed your comments

@JamyDev
Copy link
Contributor

JamyDev commented Feb 6, 2025

@ellie-idb can you update the description to indicate that this is ready for merging in yet, and see if you can fix the build failures?

@ellie-idb
Copy link
Contributor Author

sure, sorry for the delay

@ellie-idb
Copy link
Contributor Author

@JamyDev build failures are fixed

@ellie-idb ellie-idb requested a review from fmeum February 18, 2025 19:13
@fmeum
Copy link
Member

fmeum commented Feb 18, 2025

Could you rebase onto master? GitHub doesn't let me merge this as is.

@ellie-idb
Copy link
Contributor Author

rebased

@fmeum fmeum enabled auto-merge (squash) February 18, 2025 19:40
@fmeum fmeum merged commit cf3c3af into bazel-contrib:master Feb 18, 2025
1 check passed
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