Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

quval
Copy link
Contributor

@quval quval commented Nov 14, 2021

PR #14011 took care of cc_libraries. This fixes the same issue for cc_imports.

Work towards #13819.

PR bazelbuild#14011 took care of cc_libraries, this fixes the same issue for cc_imports.

Work towards bazelbuild#13819.
@lberki lberki removed their request for review November 15, 2021 08:13
@gregestren gregestren added the team-Rules-CPP Issues for C++ rules label Nov 23, 2021
@comius comius assigned oquenchil and unassigned comius Nov 24, 2021
@quval
Copy link
Contributor Author

quval commented Dec 1, 2021

@oquenchil: friendly ping :)

@quval
Copy link
Contributor Author

quval commented Dec 14, 2021

@oquenchil: another friendly ping :)

@Wyverald
Copy link
Member

@oquenchil is on vacation until January. @comius do we have anyone to pick this up in the meantime?

@quval
Copy link
Contributor Author

quval commented Dec 14, 2021

Oh, I see. This can probably wait then.

@quval
Copy link
Contributor Author

quval commented Jan 20, 2022

@oquenchil / @Wyverald - is it time for another friendly ping?

@oquenchil
Copy link
Contributor

Sorry I'm swamped right now and last time there was a contribution for this part of the codebase it became tricky to review.

@quval
Copy link
Contributor Author

quval commented Jan 28, 2022

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.

@brentleyjones
Copy link
Contributor

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(
Copy link
Contributor

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!

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@oquenchil oquenchil left a comment

Choose a reason for hiding this comment

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

LGTM

@bazel-io bazel-io closed this in 8734ccf Feb 8, 2022
@brentleyjones
Copy link
Contributor

@bazel-io fork 5.1

@brentleyjones
Copy link
Contributor

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Feb 8, 2022
@brentleyjones
Copy link
Contributor

@bazel-io fork 5.1

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Feb 8, 2022
brentleyjones pushed a commit to brentleyjones/bazel that referenced this pull request Feb 8, 2022
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)
Wyverald pushed a commit that referenced this pull request Feb 9, 2022
#14757)

PR #14011 took care of cc_libraries. This fixes the same issue for cc_imports.

Work towards #13819.

Closes #14272.

PiperOrigin-RevId: 427137675
(cherry picked from commit 8734ccf)

Co-authored-by: Yuval K <yuvalk@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants