fix the internal archive to include all of the original dependencies#4405
fix the internal archive to include all of the original dependencies#4405jgautier-dd wants to merge 2 commits intobazel-contrib:masterfrom
Conversation
jayconrod
left a comment
There was a problem hiding this comment.
Sorry to take a while to review. It's been a chaotic week. This code is also probably the most complicated logic in rules_go, so it takes a while to read back and understand.
-
Could you please ensure the tests cover this and reproduce the problem? Maybe add something to
gopackagesdriver_test.go. That file creates a test workspace, then has test cases that rungopackagesdriverwithin that workspace in a few different ways. -
For the actual code in
test.bzl, I don't like thatinternal_archiveis defined more than once, especially in a loop.internal_archiveis also used a few lines below, and it's hard to reason about whether that happens before or after.Can this be fixed by changing some input to the
go.archivecall on L672? I thinkattr["deps"]is what eventually ends up asinternal_archive.direct. So maybeinternal_depsshould include the recompiled version if available or the original version of each dependency? Currently it only includes the original versions.
What type of PR is this?
Bug fix
What does this PR do? Why is it needed?
I had an issue where the go packages driver was not loading all of the dependencies in a test resulting in unresolved imports. I tracked it down to the process of recompiling for internal/external in
go_test. For the internal archive it is filtering out the dependencies that need to be recompiled, and recompiling them, but the finalinternal_archivedoes not include the recompiled dependencies. Later when the packages driver aspect lists the dependencies for thego_testtarget it is missing the dependencies which were recompiled.