Compute rpath correctly with nested bazel modules #4390
Merged
+66
−4
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dgoel
force-pushed
the
rpath_nested_bzlmods
branch
from
0957235 to
a1d452b
Compare
fmeum
reviewed
Jul 2, 2025
Member
fmeum
left a comment
fmeum
left a comment
There was a problem hiding this comment.
Functionally looks good to me, just a few stylistic comments.
fmeum
reviewed
Jul 2, 2025
fmeum
reviewed
Jul 2, 2025
fmeum
reviewed
Jul 2, 2025
fmeum
approved these changes
Jul 2, 2025
Member
fmeum
left a comment
fmeum
left a comment
There was a problem hiding this comment.
I applied minor edits that inline the new variables to keep the diff small. I hope you don't mind :-)
Previous code was assuming that both the binary and the shared library are present under the working directory. This assumption is violated for a repo with nested modules. The fix is to compute rpath relative to the runfiles directory.
fmeum
force-pushed
the
rpath_nested_bzlmods
branch
from
ab37811 to
7cfc71c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Bug fix
What does this PR do? Why is it needed?
My company has a complicated, uncommon bazel set up with nested bazel modules. We also use cgo extensively and have many go tests and binaries that end up linking to shared C++ libraries generated via other bazel modules. When such go tests are run with rbe, they fail due to failure to locate the shared libraries within the runfiles directory.
On deeper investigation, I found that the existing
rpathcomputation assumes that both the executable and the library are present within the working directory. In reality, the executable is present outside the working directory but within the runfiles directory.Normalizing the paths with respect to the runfiles directory fixes the issue.
Relevant links:
Other notes for review
I added a test to verify that rpath computation works with nested modules. In fairness, this mainly guarantees that there's no regression in existing functionality as the test passes even without my fix. So far, I've not been able to produce a minimum example that showcases the problem.
Existing tests in tests/core/cgo also exercise rpath computation.