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.

Sign up for GitHub

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

Jump to bottom

Add the runfiles directory to the runtime library search paths #14600

Closed
wants to merge 1 commit into from
Closed

Add the runfiles directory to the runtime library search paths #14600

wants to merge 1 commit into from

Conversation

EdSchouten
Copy link
Contributor

@EdSchouten EdSchouten commented Jan 19, 2022 •
edited

When executing a C++ binary through REv2 directly (e.g., directly as a
genrule, not as a dependency of another script/executable), the input
root will only contain a _solib_arch directory inside the executable's
runfiles directory. This means that library lookups also need to
consider rpath $ORIGIN/$executable.runfiles/$workspace/_solib_$arch/.
Only looking at ../../../_solib_$arch/ is not sufficient.

This issue isn't noticeable when doing local execution, for the reason
that rtld/dyld will first do a realpath() on the executable, meaning the
lookup of shared objects takes place outside of the sandbox, inside of
bazel-out/ directly.

This change extends LibrariesToLinkCollector to duplicate all runtime
library search paths. One for bare execution, and one for execution
inside a runfiles directory.

@EdSchouten EdSchouten force-pushed the eschouten/20220119-runfiles-search-path branch 3 times, most recently from 4d9e0ca to 6583c1d Compare January 19, 2022 17:53
@aiuto aiuto added the team-Rules-CPP Issues for C++ rules label Jan 20, 2022
@EdSchouten EdSchouten marked this pull request as ready for review January 20, 2022 12:17
@ckolli5 ckolli5 added the awaiting-review PR is awaiting review from an assigned reviewer label Apr 26, 2022
@EdSchouten EdSchouten force-pushed the eschouten/20220119-runfiles-search-path branch from 6583c1d to 1e781d0 Compare July 15, 2022 11:49
@EdSchouten
Copy link
Contributor Author

@oquenchil PTAL!

oquenchil
oquenchil approved these changes Jul 25, 2022
View reviewed changes
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. Could you please verify there aren't merge conflicts? Then I will merge.

This was referenced Jul 30, 2022
Open
Closed
@EdSchouten
Add the runfiles directory to the runtime library search paths
83d2b03 
When executing a C++ binary through REv2 directly (e.g., directly as a
genrule, not as a dependency of another script/executable), the input
root will only contain a _solib_arch directory inside the executable's
runfiles directory. This means that library lookups also need to
consider rpath $ORIGIN/$executable.runfiles/$workspace/_solib_$arch/.
Only looking at ../../../_solib_$arch/ is not sufficient.

This issue isn't noticeable when doing local execution, for the reason
that rtld/dyld will first do a realpath() on the executable, meaning the
lookup of shared objects takes place outside of the sandbox, inside of
bazel-out/ directly.

This change extends LibrariesToLinkCollector to duplicate all runtime
library search paths. One for bare execution, and one for execution
inside a runfiles directory.

Change-Id: I4256cb46fee737139aba6f1cfb0531aea5deb315
@EdSchouten EdSchouten force-pushed the eschouten/20220119-runfiles-search-path branch from 1e781d0 to 83d2b03 Compare August 11, 2022 12:34
@EdSchouten
Copy link
Contributor Author

Thanks, @oquenchil! I've just addressed the merged conflicts. Please take another look.

oquenchil
oquenchil approved these changes Aug 17, 2022
View reviewed changes
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

@EdSchouten
Copy link
Contributor Author

Hey @oquenchil,
Just to double check: you're not waiting for me to merge this change, right? I don't have the permissions for that.

@Wyverald Wyverald added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Aug 23, 2022
@Wyverald
Copy link
Member

No action items for you, @EdSchouten. Our team will merge this. Thanks!

@fmeum fmeum mentioned this pull request Aug 23, 2022
Closed
@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Aug 24, 2022
@oquenchil
Copy link
Contributor

This causes targets to fail internally when there is a , in the target name.

ld: error: cannot open SOME_NAME.runfiles/workspace_name/_solib_k8/: No such file or directory
ld: error: cannot open  SOME_NAME.runfiles/workspace_name/some_name-compiler-k8-llvm/: No such file or directory
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Could you please take a look and make sure it takes that into account?

@Wyverald
Copy link
Member

Wyverald commented Sep 1, 2022

Note that this was rolled back in e898060 due to the breakage above.

@fmeum
Copy link
Collaborator

fmeum commented Sep 1, 2022 •
edited

@oquenchil Would you have compatibility concerns if we used -Xlinker instead of -Wl? That's the most direct fix I see. At least according to https://stackoverflow.com/questions/7221141/is-there-a-difference-between-the-wl-option-and-xlinker-option-syntax-for#comment8680282_7221187, it may even be more widely supported.

@oquenchil
Copy link
Contributor

oquenchil commented Sep 2, 2022 •
edited

On the crosstool? Can you think of any cases where that would break? I'm not familiar with the flag.

@fmeum
Copy link
Collaborator

fmeum commented Sep 2, 2022 •
edited

Yes, that would be set on the toolchain. GCC and clang support both flags. I know that there are compilers that support -Xlinker but not -Wl. I can't rule out the possibility that there is a compiler that only supports -Wl though.

@fmeum fmeum mentioned this pull request Sep 5, 2022
Closed
@Wyverald Wyverald mentioned this pull request Sep 5, 2022
Closed
@oquenchil
Copy link
Contributor

I'm not aware either, so quite possibly out of ignorance no compatibility concerns.

aiuto pushed a commit to aiuto/bazel that referenced this pull request Oct 12, 2022
@EdSchouten @aiuto
Add the runfiles directory to the runtime library search paths
3d123ed 
When executing a C++ binary through REv2 directly (e.g., directly as a
genrule, not as a dependency of another script/executable), the input
root will only contain a \_solib\_arch directory inside the executable's
runfiles directory. This means that library lookups also need to
consider rpath $ORIGIN/$executable.runfiles/$workspace/\_solib\_$arch/.
Only looking at ../../../\_solib\_$arch/ is not sufficient.

This issue isn't noticeable when doing local execution, for the reason
that rtld/dyld will first do a realpath() on the executable, meaning the
lookup of shared objects takes place outside of the sandbox, inside of
bazel-out/ directly.

This change extends LibrariesToLinkCollector to duplicate all runtime
library search paths. One for bare execution, and one for execution
inside a runfiles directory.

Closes bazelbuild#14600.

Change-Id: I4256cb46fee737139aba6f1cfb0531aea5deb315
PiperOrigin-RevId: 469733573
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oquenchil oquenchil oquenchil approved these changes

@lberki lberki Awaiting requested review from lberki lberki is a code owner

Assignees
No one assigned
Labels
team-Rules-CPP Issues for C++ rules
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@EdSchouten @Wyverald @oquenchil @fmeum @aiuto @sgowroji @ckolli5