rules_go icon indicating copy to clipboard operation
rules_go copied to clipboard

gopackagesdriver: skip root packages that can't be built

Open jayconrod opened this issue 1 year ago • 6 comments

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

Omits package IDs from gopackagesdriver's roots list if we can't build package metadata for them.

Which issues(s) does this PR fix?

Fixes #3805

Other notes for review

jayconrod avatar Dec 29 '23 20:12 jayconrod

Are there tests for gopackagesdriver? I didn't see a place to add a test case. If I missed it, please let me know, happy to add.

jayconrod avatar Dec 29 '23 20:12 jayconrod

@JamyDev added a basic test that you could extend (if possible): tests/integration/gopackagesdriver/gopackagesdriver_test.go

fmeum avatar Jan 02 '24 16:01 fmeum

Thanks! Not sure how I missed that. Added a test case there.

jayconrod avatar Jan 02 '24 19:01 jayconrod

@jayconrod There is still a failure on Bazel 5.4.0. I think it's fine if this fix doesn't support that version, so if you rather want to disable the test in the 5.4.0 CI run, feel free to do so.

fmeum avatar Feb 09 '24 16:02 fmeum

Sorry I haven't gotten back to this yet! I'll take some time this weekend and try to finish it.

I ran into some trouble with the testing framework and was planning to submit another PR ahead of this one that would fix it. Windows, as always, is being troublesome though.

jayconrod avatar Feb 09 '24 17:02 jayconrod

In #3856, I've moved gopackagesdriver_test into //go/tools/gopackagesdriver. Since it embeds the library instead of calling via bazel run, the test is considerably easier to write, debug, and faster to iterate on.

I've rebased this one, but GitHub won't let me set the base to a branch in my own fork. So... the diff will look ugly until #3856 is merged and this is rebased again. Sorry for the extra confusion.

jayconrod avatar Feb 11 '24 04:02 jayconrod