go/tools/gopackagesdriver: fix package walk panic if bzlmod enabled

[pziggo]

❗ Update: This PR has been abandoned as agreed here #3700 (comment)

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

With bzlmod enabled, gopackagesdriver panics with:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x1145c5e]

With bzlmod enabled, also canonical repository names are enabled. Thus
labels from the local repository have to be prefixed with two '@' (in
order to refer to the canonical repository name) to match the IDs in
the package registry.
Also, the result of a bazel query does not return the canonical name of
the rules_go repository, but the apparent repository name. Thus, the
query for go targets it will not match the stdlib label as constructed
with the native.repository_name() rule as injected via x_defs.

Since the roots do not match the package IDs, the lookup return nil which
leads to a segmentation violation while trying to access attributes from the
structure.

Which issues(s) does this PR fix?

Fixes #3604 #3659

Other notes for review

Tests: TODO

This fix must be considered as workaround as it leads to a mix of
canonical and apparant repository names in the package lists. Once
bazel query also supports returning the canonincal repository names, a
more correct fix can be applied.

[fmeum]

Thanks for sending a workaround.

I am working on a full fix based on Bazel 6.4.0 over at #3701. Since 6.4.0rc1 should come out either today or tomorrow, I would like to wait and see whether this fully addresses the issue. If it does, I would slightly prefer to have everyone move over to 6.4.0 instead of adding workarounds - the logic around labels in gopackagesdriver is already pretty complex and doesn't have sufficient test coverage.

[pziggo]

I am definitely in favour of a real and clean fix and agree that yet another badly tested workaround might make it worse. But from my experience, not everyone can immediately jump onto the next Bazel version. That's why I offered this workaround based on v0.41.0 still until your fix is ready to be shipped with the compatibility break.
But no worries, debugging this issue gave me already a pretty good impression of the complexity and why another workaround is definitely not a nice thing. Feel free to close the PR again.

[fmeum]

I do see value in making this work in the short term. How about the following?

1. Once 6.4.0rc1 is out, we test the full fix and ensure that it works reliably.
2. We document that it requires 6.4.0 and if someone wants to use gopackagesdriver with an earlier version of Bazel, they can use your PR as a patch.

In this way the workaround doesn't become a maintenance problem, but we still have something better to offer users on Bazel < 6.4.0 than "doesn't work, update".

[pziggo]

Sounds good to me 👍

[seanmorton-afs]

Hi @pziggo and @fmeum thanks for working on this issue. I'm on a team currently on 5.4.0 and we won't upgrade soon so this patch will help me.

Now that 6.4.0 is out, was the intention to merge this for < 6.4.0 users? Or do you mean we should fork and patch? It would be nice to not have to maintain a fork if possible. Nevermind, I just found out about bazel patching.

We document that it requires 6.4.0 and if someone wants to use gopackagesdriver with an earlier version of Bazel, they can use your PR as a patch.

If you point me to where this should be documented I can submit a PR for that and this could be closed out.

[ian-h-chamberlain]

Btw, I just tried applying this patch in my org's repo and it doesn't seem to totally work around the issue on Bazel 5.4.0 (patched onto v0.42.0):

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x112ae6d]

goroutine 1 [running]:
main.(*PackageRegistry).walk(0xc000144310, 0xc0000c3aa0?, {0xc0001fec80?, 0x19?})
        external/io_bazel_rules_go/go/tools/gopackagesdriver/packageregistry.go:79 +0x4d
main.(*PackageRegistry).Match(0xc000144310, {0xc000038290, 0x1, 0xc0003390c0?})
        external/io_bazel_rules_go/go/tools/gopackagesdriver/packageregistry.go:116 +0x406
main.(*JSONPackagesDriver).GetResponse(0xc000338000?, {0xc000038290?, 0xc00008a140?, 0xc000038290?})
        external/io_bazel_rules_go/go/tools/gopackagesdriver/json_packages_driver.go:51 +0x1f
main.run()
        external/io_bazel_rules_go/go/tools/gopackagesdriver/main.go:115 +0x558
main.main()
        external/io_bazel_rules_go/go/tools/gopackagesdriver/main.go:119 +0x1f

For now we are just using a revert as I mentioned in #3604 (comment) but just wanted to raise awareness that this may not be a perfect workaround.

Happy to try and help debug if there is more info I can provide

[seanmorton-afs]

Thanks Ian, I also hit the same problem with this patch -- I think it only applies to when bzlmod is enabled which is not enabled in my project.

I also tried your sugggestion from #3604 to patch the two reverts, and things are working again for me with 5.4.0 as well. Thank you!

When I get some extra time I'll try to add this to documentation in https://github.com/bazelbuild/rules_go/tree/master/docs/go/editors.

[pziggo]

Hi @seanmorton-afs, @ian-h-chamberlain, in fact this patch was created based on my debugging efforts with bzlmod enabled. I didn't even realize back then that there is an issue without bzlmod as well.
So if the workaround by applying the revert patches works for both configurations, this patch should maybe be ignored to not confuse people even more ?
@fmeum what do you think ?

[fmeum]

@pziggo If I understand the situation correctly, then:

• my PR fixes the issue with Bazel 6.4.0
• reverting the two PRs mentioned above fixes the issue without Bzlmod as well as before Bazel 6.4.0

If that's the case then I think this patch isn't needed anymore. Thanks for working on it though!

[seanmorton-afs]

Yes @fmeum that looks correct to me.

[pziggo]

Abandoned as agreed here #3700 (comment)