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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Sep 3, 2022 •
edited
Loading

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)

@fmeum fmeum force-pushed the 16153-add-rpath-entries branch from ebe1081 to 952c901 Compare September 3, 2022 13:13
@fmeum fmeum marked this pull request as ready for review September 3, 2022 13:56
@fmeum fmeum requested a review from lberki as a code owner September 3, 2022 13:56
@fmeum
Copy link
Collaborator Author

fmeum commented Sep 3, 2022

@oquenchil @Wyverald

@fmeum fmeum mentioned this pull request Sep 4, 2022
Closed
@sgowroji sgowroji added team-Rules-CPP Issues for C++ rules awaiting-review PR is awaiting review from an assigned reviewer labels Sep 5, 2022
@Wyverald
Copy link
Member

Wyverald commented Sep 5, 2022

Just checking, so this doesn't need #14600 ?

lberki
lberki reviewed Sep 5, 2022
View reviewed changes
Copy link
Contributor

@lberki lberki left a comment

Choose a reason for hiding this comment

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

In principle, I like this change, even though it results in somewhat unwieldy rpaths. Let's make sure, though, that it works with --experimental_sibling_repository_layout. It looks like it does because that flag does not change the layout of the runfiles tree, but better be safe than sorry.

I'm also not very convinced that we need to worry about quoting commas in -Wl.

@oquenchil , WDYT?

src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java Outdated Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java Outdated Show resolved Hide resolved
@fmeum fmeum force-pushed the 16153-add-rpath-entries branch from 952c901 to 55b1f58 Compare September 5, 2022 17:31
@fmeum fmeum requested a review from a team as a code owner September 5, 2022 17:31
@fmeum
Copy link
Collaborator Author

fmeum commented Sep 5, 2022 •
edited
Loading

Just checking, so this doesn't need #14600 ?

That's right, mostly because it steals large parts from it :-)

@fmeum
Copy link
Collaborator Author

fmeum commented Sep 5, 2022 •
edited
Loading

@oquenchil The cc_builtin_test is failing with RBE only, but the logs don't contain much information about the failure: https://storage.googleapis.com/bazel-untrusted-buildkite-artifacts/01830eb5-da8b-48ab-8206-9cbff9d36d41/src/main/starlark/tests/builtins_bzl/cc_builtin_tests/test.log
Do you have an idea what could cause this failure if all other tests pass or how I could debug this problem?

Turned out to be a flake.

@fmeum fmeum force-pushed the 16153-add-rpath-entries branch 6 times, most recently from 6cf13fa to 5b792fe Compare September 8, 2022 09:33
@fmeum
Fix local execution of external dynamically linked cc_* targets
7a750cc 
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.
@fmeum fmeum force-pushed the 16153-add-rpath-entries branch from 5b792fe to 7a750cc Compare September 8, 2022 10:24
oquenchil
oquenchil approved these changes Sep 9, 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

@oquenchil oquenchil 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 Sep 9, 2022
@fmeum
Copy link
Collaborator Author

fmeum commented Sep 9, 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 Sep 9, 2022
@fmeum fmeum mentioned this pull request Sep 9, 2022
Closed
@Wyverald Wyverald removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Sep 9, 2022
@Wyverald
Copy link
Member

Wyverald commented Sep 9, 2022

The issue fixed by this PR is tracked for 5.3.1 at #16153

@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Sep 12, 2022
@fmeum fmeum deleted the 16153-add-rpath-entries branch September 12, 2022 11:48
fmeum added a commit to fmeum/bazel that referenced this pull request Sep 12, 2022
@fmeum
Fix local execution of external dynamically linked cc_* targets
3977705 
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 fmeum mentioned this pull request Sep 12, 2022
Merged
fmeum added a commit to fmeum/bazel that referenced this pull request Sep 12, 2022
@fmeum
Fix local execution of external dynamically linked cc_* targets
4ab4f64 
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
@fmeum
Fix local execution of external dynamically linked cc_* targets
a5f91bd 
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
@fmeum
Fix local execution of external dynamically linked cc_* targets
25af0f9 
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
@fmeum @ShreeM01
Fix local execution of external dynamically linked cc_* targets (#16253)
1e25152 
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 <[email protected]>
copybara-service bot pushed a commit that referenced this pull request Sep 16, 2022
@oquenchil @copybara-github
Fix dynamic library lookup with remotely executed tools
c7aa392 
When a cc_binary tool is executed directly (e.g. in a genrule) with
remote execution, its dynamic dependencies are only available from the
solib directory in the runfiles directory. This commit adds an
additional rpath entry for this directory.

Since target names may contain commas and the newly added rpaths contain
target names, the `-Xlinker` compiler is now used everywhere instead of
the `-Wl` flag, which doesn't support commas in linker arguments.

Stacked on #16214

Closes #16215.

PiperOrigin-RevId: 474792702
Change-Id: I2bfa1fd77be83d7bfe54ba591af5cb0ad0e630fc
aiuto pushed a commit to aiuto/bazel that referenced this pull request Oct 12, 2022
@fmeum @aiuto
Fix local execution of external dynamically linked cc_* targets
ee1d221 
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
aiuto pushed a commit to aiuto/bazel that referenced this pull request Oct 12, 2022
@oquenchil @aiuto
Fix dynamic library lookup with remotely executed tools
f254f8d 
When a cc_binary tool is executed directly (e.g. in a genrule) with
remote execution, its dynamic dependencies are only available from the
solib directory in the runfiles directory. This commit adds an
additional rpath entry for this directory.

Since target names may contain commas and the newly added rpaths contain
target names, the `-Xlinker` compiler is now used everywhere instead of
the `-Wl` flag, which doesn't support commas in linker arguments.

Stacked on bazelbuild#16214

Closes bazelbuild#16215.

PiperOrigin-RevId: 474792702
Change-Id: I2bfa1fd77be83d7bfe54ba591af5cb0ad0e630fc
copybara-service bot pushed a commit that referenced this pull request Oct 17, 2022
@fmeum @copybara-github
Roll forward of c7aa392:
e3dcfa5 
Fix dynamic library lookup with remotely executed tools

When a cc_binary tool is executed directly (e.g. in a genrule) with
remote execution, its dynamic dependencies are only available from the
solib directory in the runfiles directory. This commit adds an
additional rpath entry for this directory.

Since target names may contain commas and the newly added rpaths contain
target names, the `-Xlinker` compiler is now used everywhere instead of
the `-Wl` flag, which doesn't support commas in linker arguments.

Stacked on #16214

Closes #16215.

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

@lberki lberki lberki left review comments

@oquenchil oquenchil oquenchil approved these changes

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.
6 participants
@fmeum @Wyverald @lberki @oquenchil @bazel-io @sgowroji