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

Fix rpath for binaries in external repositories #16008

Closed
wants to merge 2 commits into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Jul 30, 2022

The solib directory is located within the subdirectory of the runfiles
directory corresponding to the workspace. Thus, if a binary is contained
in an external repository, its $ORIGIN relative rpath has to first
ascend to the runfiles directory and then descend into the workspace
directory.

Fixes #16003

@fmeum
Copy link
Collaborator Author

fmeum commented Jul 30, 2022

@oquenchil This PR is similar to #14600, but resolves a different issue. I can rebase this PR onto #14600 if you should prefer to merge that one first.

@fmeum fmeum marked this pull request as ready for review July 30, 2022 21:48
@fmeum fmeum requested review from a team and lberki as code owners July 30, 2022 21:48
The solib directory is located within the subdirectory of the runfiles
directory corresponding to the workspace. Thus, if a binary is contained
in an external repository, its $ORIGIN relative rpath has to first
ascend to the runfiles directory and then descend into the workspace
directory.
@fmeum
Copy link
Collaborator Author

fmeum commented Jul 31, 2022

@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 Jul 31, 2022
@sgowroji sgowroji added team-Remote-Exec Issues and PRs for the Execution (Remote) team awaiting-review PR is awaiting review from an assigned reviewer labels Aug 1, 2022
@AustinSchuh
Copy link
Contributor

Success, this fixes test in external repositories! Thanks @fmeum !

@ShreeM01
Copy link
Contributor

ShreeM01 commented Aug 1, 2022

@bazel-io fork 5.3.0

1 similar comment
@ShreeM01
Copy link
Contributor

ShreeM01 commented Aug 1, 2022

@bazel-io fork 5.3.0

@fmeum
Copy link
Collaborator Author

fmeum commented Aug 1, 2022

@kshyanashree The command lacks the rc1.

@ShreeM01
Copy link
Contributor

ShreeM01 commented Aug 1, 2022

@bazel-io fork 5.3.0rc1

@fmeum
Copy link
Collaborator Author

fmeum commented Aug 1, 2022

@kshyanashree I don't know why, but the last command failed as well.

@coeuvre coeuvre added team-Rules-CPP Issues for C++ rules and removed team-Remote-Exec Issues and PRs for the Execution (Remote) team labels Aug 2, 2022
@coeuvre coeuvre requested review from oquenchil and removed request for a team and lberki August 2, 2022 13:44
@coeuvre
Copy link
Member

coeuvre commented Aug 2, 2022

Assigning to @oquenchil since this is about cc rules.

@sgowroji
Copy link
Member

sgowroji commented Aug 2, 2022

@bazel-io fork 5.3.0

@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 Aug 2, 2022
@ShreeM01
Copy link
Contributor

ShreeM01 commented Aug 3, 2022

Hi @fmeum, Is it possible for you to get the PR review done? Since we will be merging it under the release 5.3.0.

@fmeum
Copy link
Collaborator Author

fmeum commented Aug 3, 2022

@kshyanashree I don't know when @oquenchil will be able to review the change. It does fix a known Bazel 5 regression - is that sufficient to hold the release for it/up the priority?

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

@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 9, 2022
@fmeum fmeum deleted the 16003-external-cc-test branch August 10, 2022 10:55
fmeum added a commit to fmeum/bazel that referenced this pull request Aug 10, 2022
The solib directory is located within the subdirectory of the runfiles
directory corresponding to the workspace. Thus, if a binary is contained
in an external repository, its $ORIGIN relative rpath has to first
ascend to the runfiles directory and then descend into the workspace
directory.

Fixes bazelbuild#16003

Closes bazelbuild#16008.

PiperOrigin-RevId: 466634083
Change-Id: I4ada28b459f23f68a2091dbaad9147cfec2fbe43
ShreeM01 pushed a commit that referenced this pull request Aug 10, 2022
The solib directory is located within the subdirectory of the runfiles
directory corresponding to the workspace. Thus, if a binary is contained
in an external repository, its $ORIGIN relative rpath has to first
ascend to the runfiles directory and then descend into the workspace
directory.

Fixes #16003

Closes #16008.

PiperOrigin-RevId: 466634083
Change-Id: I4ada28b459f23f68a2091dbaad9147cfec2fbe43
@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Aug 11, 2022
@pcjanzen
Copy link
Contributor

Although this patch fixes the remote execution case, it breaks the local execution case.

In local execution, the runfiles tree contains symlinks to regular files in the output root; the $ORIGIN that the dynamic loader uses is the location of that regular file, not the symlink:

bazel-bin/external/other_repo/test/test.runfiles/other_repo/test/test -> bazel-bin/external/other_repo/test/test

and the solib directory is directly underneath bazel-bin, there is no subdirectory for the main workspace.

This causes the test to fail:

INFO: From Testing @other_repo//test:test:
==================== Test output for @other_repo//test:test:
/home/pauljanzen/.cache/bazel/_bazel_pauljanzen/ce3ed1e55900fb9d8cbdc1181abe7157/sandbox/processwrapper-sandbox/4/execroot/__main__/bazel-out/k8-fastbuild/bin/external/other_repo/test/test.runfiles/__main__/../other_repo/test/test: error while loading shared libraries: libexternal_Sother_Urepo_Slib_Sliblib.so: cannot open shared object file: No such file or directory
================================================================================

@fmeum
Copy link
Collaborator Author

fmeum commented Aug 23, 2022

CC @kshyanashree as a potential 5.3.0 regression.

@pcjanzen Would adding both rpaths fix this problem?

@fmeum fmeum mentioned this pull request Aug 23, 2022
6 tasks
@pcjanzen
Copy link
Contributor

Would adding both rpaths fix this problem?

I think so, but between this problem and #14600 there are starting to be kind of a lot of rpath entries and I can't honestly say I've thought through all the combinations.

copybara-service bot pushed a commit that referenced this pull request Sep 12, 2022
The fix for remote execution of external dynamically linked cc_* targets
in 95a5bfd regressed local execution of such targets. This is fixed
by emitting two rpaths in the case of an external target.

Fixes #16008 (comment)

Closes #16214.

PiperOrigin-RevId: 473706330
Change-Id: I500f8b1f79e4d994323651fbb5964a730f1136e3
fmeum added a commit to fmeum/bazel that referenced this pull request Sep 12, 2022
The fix for remote execution of external dynamically linked cc_* targets
in 95a5bfd regressed local execution of such targets. This is fixed
by emitting two rpaths in the case of an external target.

Fixes bazelbuild#16008 (comment)

Closes bazelbuild#16214.

PiperOrigin-RevId: 473706330
Change-Id: I500f8b1f79e4d994323651fbb5964a730f1136e3
fmeum added a commit to fmeum/bazel that referenced this pull request Sep 12, 2022
The fix for remote execution of external dynamically linked cc_* targets
in 95a5bfd regressed local execution of such targets. This is fixed
by emitting two rpaths in the case of an external target.

Fixes bazelbuild#16008 (comment)

Closes bazelbuild#16214.

PiperOrigin-RevId: 473706330
Change-Id: I500f8b1f79e4d994323651fbb5964a730f1136e3
fmeum added a commit to fmeum/bazel that referenced this pull request Sep 12, 2022
The fix for remote execution of external dynamically linked cc_* targets
in 95a5bfd regressed local execution of such targets. This is fixed
by emitting two rpaths in the case of an external target.

Fixes bazelbuild#16008 (comment)

Closes bazelbuild#16214.

PiperOrigin-RevId: 473706330
Change-Id: I500f8b1f79e4d994323651fbb5964a730f1136e3
fmeum added a commit to fmeum/bazel that referenced this pull request Sep 12, 2022
The fix for remote execution of external dynamically linked cc_* targets
in 95a5bfd regressed local execution of such targets. This is fixed
by emitting two rpaths in the case of an external target.

Fixes bazelbuild#16008 (comment)

Closes bazelbuild#16214.

PiperOrigin-RevId: 473706330
Change-Id: I500f8b1f79e4d994323651fbb5964a730f1136e3
ShreeM01 added a commit that referenced this pull request Sep 12, 2022
The fix for remote execution of external dynamically linked cc_* targets
in 95a5bfd regressed local execution of such targets. This is fixed
by emitting two rpaths in the case of an external target.

Fixes #16008 (comment)

Closes #16214.

PiperOrigin-RevId: 473706330
Change-Id: I500f8b1f79e4d994323651fbb5964a730f1136e3

Co-authored-by: kshyanashree <109167932+kshyanashree@users.noreply.github.com>
aiuto pushed a commit to aiuto/bazel that referenced this pull request Oct 12, 2022
The fix for remote execution of external dynamically linked cc_* targets
in 95a5bfd regressed local execution of such targets. This is fixed
by emitting two rpaths in the case of an external target.

Fixes bazelbuild#16008 (comment)

Closes bazelbuild#16214.

PiperOrigin-RevId: 473706330
Change-Id: I500f8b1f79e4d994323651fbb5964a730f1136e3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bazel 5.3 fails to run external tests
9 participants