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

EdSchouten · 2022-01-19T17:22:39Z

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 · 2022-07-15T11:51:00Z

@oquenchil PTAL!

oquenchil

LGTM. Could you please verify there aren't merge conflicts? Then I will merge.

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 · 2022-08-11T12:38:37Z

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

oquenchil

LGTM

EdSchouten · 2022-08-23T12:32:47Z

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 · 2022-08-23T12:38:33Z

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

oquenchil · 2022-08-29T13:41:00Z

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 · 2022-09-01T13:12:49Z

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

fmeum · 2022-09-01T13:30:24Z

@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 · 2022-09-02T14:26:37Z

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

fmeum · 2022-09-02T15:46:13Z

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.

oquenchil · 2022-09-09T09:53:03Z

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

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

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 added the team-Rules-CPP Issues for C++ rules label Jan 20, 2022

EdSchouten marked this pull request as ready for review January 20, 2022 12:17

EdSchouten requested review from lberki and oquenchil as code owners January 20, 2022 12:17

ckolli5 added the awaiting-review PR is awaiting review from an assigned reviewer label Apr 26, 2022

EdSchouten force-pushed the eschouten/20220119-runfiles-search-path branch from 6583c1d to 1e781d0 Compare July 15, 2022 11:49

oquenchil approved these changes Jul 25, 2022

View reviewed changes

This was referenced Jul 30, 2022

Bazel 5.3 fails to run external tests #16003

Open

Fix rpath for binaries in external repositories #16008

Closed

EdSchouten force-pushed the eschouten/20220119-runfiles-search-path branch from 1e781d0 to 83d2b03 Compare August 11, 2022 12:34

oquenchil approved these changes Aug 17, 2022

View reviewed changes

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

fmeum mentioned this pull request Aug 23, 2022

[5.3.1] Bazel 5.3 fails to run external tests #16153

Closed

copybara-service bot closed this in 14a0d83 Aug 24, 2022

sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Aug 24, 2022

fmeum mentioned this pull request Sep 5, 2022

Fix dynamic library lookup with remotely executed tools #16215

Closed

Wyverald mentioned this pull request Sep 5, 2022

Fix local execution of external dynamically linked cc_* targets #16214

Closed

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

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

Uh oh!

Conversation

EdSchouten commented Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EdSchouten commented Jul 15, 2022

Uh oh!

oquenchil left a comment

Choose a reason for hiding this comment

Uh oh!

EdSchouten commented Aug 11, 2022

Uh oh!

oquenchil left a comment

Choose a reason for hiding this comment

Uh oh!

EdSchouten commented Aug 23, 2022

Uh oh!

Wyverald commented Aug 23, 2022

Uh oh!

oquenchil commented Aug 29, 2022

Uh oh!

Wyverald commented Sep 1, 2022

Uh oh!

fmeum commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oquenchil commented Sep 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmeum commented Sep 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oquenchil commented Sep 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

EdSchouten commented Jan 19, 2022 •

edited

Loading

fmeum commented Sep 1, 2022 •

edited

Loading

oquenchil commented Sep 2, 2022 •

edited

Loading

fmeum commented Sep 2, 2022 •

edited

Loading