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 9 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 and others added 3 commits November 28, 2024 13:05
Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.40.1 to 1.56.3.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.40.1...v1.56.3)

---
updated-dependencies:
- dependency-name: google.golang.org/grpc
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
@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 Show resolved Hide resolved
Imports: imports,
}

// TODO(ellie): wtf why does this fix things
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know why now?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess would be here:

I haven’t tested this yet, but it would line up here — we build everything with pure (i.e. no cgo)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checked - this was why this was actually broken.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm.. Okay, so. The problem with enabling -compiled is that it kicks off a really intensive build of every cgo file in the standard library - as seen by why the latest CI run is failing. I'm not convinced that this is actually the best fix either. (see: golang/go#29427)

ellie-idb and others added 5 commits December 2, 2024 10:49
…olang.org/grpc-1.56.3

Bump google.golang.org/grpc from 1.40.1 to 1.56.3
Bumps google.golang.org/protobuf from 1.31.0 to 1.33.0.

---
updated-dependencies:
- dependency-name: google.golang.org/protobuf
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
…olang.org/protobuf-1.33.0

Bump google.golang.org/protobuf from 1.31.0 to 1.33.0
@aran
Copy link

aran commented Dec 17, 2024

What else would be needed here to get this landed?

@@ -213,12 +211,12 @@ func (fp *FlatPackage) ResolveImports(resolve ResolvePkgFunc, overlays map[strin
if imp == "C" {
continue
}
if _, ok := fp.Imports[imp]; ok {
if pkg := pkg.Imports[imp]; pkg != nil {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the default nogo rules, this triggers an issue due to the shadowing:

nogo: errors found by nogo during build-time code analysis:
external/rules_go~/go/tools/gopackagesdriver/flatpackage.go:214:7: declaration of "pkg" shadows declaration at line 185 (shadow)

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