Skip to content

fix the internal archive to include all of the original dependencies #4405

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jgautier-dd
Copy link
Contributor

@jgautier-dd jgautier-dd commented Jul 30, 2025

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 final internal_archive does not include the recompiled dependencies. Later when the packages driver aspect lists the dependencies for the go_test target it is missing the dependencies which were recompiled.

@fmeum fmeum requested a review from jayconrod July 31, 2025 05:44
Copy link
Contributor

@jayconrod jayconrod left a comment

Choose a reason for hiding this comment

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

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.

  1. 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 run gopackagesdriver within that workspace in a few different ways.

  2. For the actual code in test.bzl, I don't like that internal_archive is defined more than once, especially in a loop. internal_archive is 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.archive call on L672? I think attr["deps"] is what eventually ends up as internal_archive.direct. So maybe internal_deps should include the recompiled version if available or the original version of each dependency? Currently it only includes the original versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants