-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Add the default solib dir to the rpath for cc_imports with transitions #14272
Conversation
PR bazelbuild#14011 took care of cc_libraries, this fixes the same issue for cc_imports. Work towards bazelbuild#13819.
@oquenchil: friendly ping :) |
@oquenchil: another friendly ping :) |
@oquenchil is on vacation until January. @comius do we have anyone to pick this up in the meantime? |
Oh, I see. This can probably wait then. |
@oquenchil / @Wyverald - is it time for another friendly ping? |
Sorry I'm swamped right now and last time there was a contribution for this part of the codebase it became tricky to review. |
I feel like this PR is much simpler than the previous one (basically 5 lines of code + a test) and fixes a real bug, so I'd appreciate it if you could review it when you do get the chance. Thanks. |
And if not, @comius can you help find someone to review this? |
// should add that to the rpath as well. | ||
if (inputArtifact.getRootRelativePathString().startsWith("_solib_")) { | ||
PathFragment artifactPathUnderSolib = inputArtifact.getRootRelativePath().subFragment(1); | ||
rpathRootsForExplicitSoDeps.add( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your patience.
I have a few questions. Running the test locally I get this:
Without your change:
Test 1:
-Wl,-rpath,$ORIGIN/../_solib_k8/_U_S_Sa_Cfoo___Ua
Test 2:
-Wl,-rpath,$ORIGIN/../_solib_k8/../../../k8-fastbuild-ST-1556fd9fdf05/bin/_solib_k8/_U_S_Sa_Cfoo___Ua
With your change:
Test 1:
-Wl,-rpath,$ORIGIN/../_solib_k8/_U_S_Sa_Cfoo___Ua
Test 2:
A) -Wl,-rpath,$ORIGIN/../_solib_k8/../../../k8-fastbuild-ST-1556fd9fdf05/bin/_solib_k8/_U_S_Sa_Cfoo___Ua
B) -Wl,-rpath,$ORIGIN/../_solib_k8/_U_S_Sa_Cfoo___Ua
IIUC if we are running locally we put the symlink under A) when there is a transition. Do we also put the symlink in B) every time, where in our codebase?
When we are not running locally A) won' t exist but B) will exist, where in the codebase do we put the symlink in B)?
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for getting to this!
Your understanding is the same as mine: A is where the symlink exists locally with a transition, and B is where it exists otherwise (no transition or not running locally). As far as I remember, B doesn't exist when running locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why A) is not created with a transition running remotely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
k8-fastbuild...
(both with and without transitions) aren't created for remote execution or under the runfiles directory, and the correct versions of shared libraries are found under the solib directory instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@bazel-io fork 5.1 |
@bazel-io flag |
@bazel-io fork 5.1 |
PR bazelbuild#14011 took care of cc_libraries. This fixes the same issue for cc_imports. Work towards bazelbuild#13819. Closes bazelbuild#14272. PiperOrigin-RevId: 427137675 (cherry picked from commit 8734ccf)
PR #14011 took care of cc_libraries. This fixes the same issue for cc_imports.
Work towards #13819.