Skip to content

Commit 1e25152

Browse files
fmeumShreeM01
fmeum
and
ShreeM01
authored
Fix local execution of external dynamically linked cc_* targets (bazelbuild#16253)
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 Co-authored-by: kshyanashree <[email protected]>
1 parent 2ca1ab2 commit 1e25152

File tree

3 files changed

+213
-36
lines changed

3 files changed

+213
-36
lines changed

‎src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java‎

Lines changed: 96 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@
1313
// limitations under the License.
1414
package com.google.devtools.build.lib.rules.cpp;
1515

16+
import static com.google.common.collect.ImmutableList.toImmutableList;
17+
1618
import com.google.common.base.Preconditions;
1719
import com.google.common.base.Strings;
1820
import com.google.common.collect.ImmutableList;
@@ -40,7 +42,6 @@ public class LibrariesToLinkCollector {
4042
private final PathFragment toolchainLibrariesSolibDir;
4143
private final CppConfiguration cppConfiguration;
4244
private final CcToolchainProvider ccToolchainProvider;
43-
private final Artifact outputArtifact;
4445
private final boolean isLtoIndexing;
4546
private final PathFragment solibDir;
4647
private final Iterable<? extends LinkerInput> linkerInputs;
@@ -49,7 +50,8 @@ public class LibrariesToLinkCollector {
4950
private final Artifact thinltoParamFile;
5051
private final FeatureConfiguration featureConfiguration;
5152
private final boolean needWholeArchive;
52-
private final String rpathRoot;
53+
private final ImmutableList<String> potentialExecRoots;
54+
private final ImmutableList<String> rpathRoots;
5355
private final boolean needToolchainLibrariesRpath;
5456
private final Map<Artifact, Artifact> ltoMap;
5557
private final RuleErrorConsumer ruleErrorConsumer;
@@ -76,7 +78,6 @@ public LibrariesToLinkCollector(
7678
this.cppConfiguration = cppConfiguration;
7779
this.ccToolchainProvider = toolchain;
7880
this.toolchainLibrariesSolibDir = toolchainLibrariesSolibDir;
79-
this.outputArtifact = output;
8081
this.solibDir = solibDir;
8182
this.isLtoIndexing = isLtoIndexing;
8283
this.allLtoArtifacts = allLtoArtifacts;
@@ -106,22 +107,83 @@ public LibrariesToLinkCollector(
106107
// and the second could use $ORIGIN/../_solib_[arch]. But since this is a shared
107108
// artifact, both are symlinks to the same place, so
108109
// there's no *one* RPATH setting that fits all targets involved in the sharing.
109-
rpathRoot = ccToolchainProvider.getSolibDirectory() + "/";
110+
potentialExecRoots = ImmutableList.of();
111+
rpathRoots = ImmutableList.of(ccToolchainProvider.getSolibDirectory() + "/");
110112
} else {
111-
// When executed from within a runfiles directory, the binary lies under a path such as
112-
// target.runfiles/some_repo/pkg/file, whereas the solib directory is located under
113-
// target.runfiles/main_repo.
114-
PathFragment runfilesPath = outputArtifact.getRunfilesPath();
115-
String runfilesExecRoot;
116-
if (runfilesPath.startsWith(LabelConstants.EXTERNAL_RUNFILES_PATH_PREFIX)) {
117-
// runfilesPath is of the form ../some_repo/pkg/file, walk back some_repo/pkg and then
118-
// descend into the main workspace.
119-
runfilesExecRoot = Strings.repeat("../", runfilesPath.segmentCount() - 2) + workspaceName + "/";
120-
} else {
121-
// runfilesPath is of the form pkg/file, walk back pkg to reach the main workspace.
122-
runfilesExecRoot = Strings.repeat("../", runfilesPath.segmentCount() - 1);
113+
// The runtime location of the solib directory relative to the binary depends on four factors:
114+
//
115+
// * whether the binary is contained in the main repository or an external repository;
116+
// * whether the binary is executed directly or from a runfiles tree;
117+
// * whether the binary is staged as a symlink (sandboxed execution; local execution if the
118+
// binary is in the runfiles of another target) or a regular file (remote execution) - the
119+
// dynamic linker follows sandbox and runfiles symlinks into its location under the
120+
// unsandboxed execroot, which thus becomes the effective $ORIGIN;
121+
// * whether --experimental_sibling_repository_layout is enabled or not.
122+
//
123+
// The rpaths emitted into the binary thus have to cover the following cases (assuming that
124+
// the binary target is located in the pkg `pkg` and has name `file`) for the directory used
125+
// as $ORIGIN by the dynamic linker and the directory containing the solib directories:
126+
//
127+
// 1. main, direct, symlink:
128+
// $ORIGIN: $EXECROOT/pkg
129+
// solib root: $EXECROOT
130+
// 2. main, direct, regular file:
131+
// $ORIGIN: $EXECROOT/pkg
132+
// solib root: $EXECROOT/pkg/file.runfiles/main_repo
133+
// 3. main, runfiles, symlink:
134+
// $ORIGIN: $EXECROOT/pkg
135+
// solib root: $EXECROOT
136+
// 4. main, runfiles, regular file:
137+
// $ORIGIN: other_target.runfiles/main_repo/pkg
138+
// solib root: other_target.runfiles/main_repo
139+
// 5a. external, direct, symlink:
140+
// $ORIGIN: $EXECROOT/external/other_repo/pkg
141+
// solib root: $EXECROOT
142+
// 5b. external, direct, symlink, with --experimental_sibling_repository_layout:
143+
// $ORIGIN: $EXECROOT/../other_repo/pkg
144+
// solib root: $EXECROOT/../other_repo
145+
// 6a. external, direct, regular file:
146+
// $ORIGIN: $EXECROOT/external/other_repo/pkg
147+
// solib root: $EXECROOT/external/other_repo/pkg/file.runfiles/main_repo
148+
// 6b. external, direct, regular file, with --experimental_sibling_repository_layout:
149+
// $ORIGIN: $EXECROOT/../other_repo/pkg
150+
// solib root: $EXECROOT/../other_repo/pkg/file.runfiles/other_repo
151+
// 7a. external, runfiles, symlink:
152+
// $ORIGIN: $EXECROOT/external/other_repo/pkg
153+
// solib root: $EXECROOT
154+
// 7b. external, runfiles, symlink, with --experimental_sibling_repository_layout:
155+
// $ORIGIN: $EXECROOT/../other_repo/pkg
156+
// solib root: $EXECROOT/../other_repo
157+
// 8a. external, runfiles, regular file:
158+
// $ORIGIN: other_target.runfiles/some_repo/pkg
159+
// solib root: other_target.runfiles/main_repo
160+
// 8b. external, runfiles, regular file, with --experimental_sibling_repository_layout:
161+
// $ORIGIN: other_target.runfiles/some_repo/pkg
162+
// solib root: other_target.runfiles/some_repo
163+
//
164+
// Cases 1, 3, 4, 5, 7, and 8b are covered by an rpath that walks up the root relative path.
165+
// Case 8a is covered by walking up some_repo/pkg and then into main_repo.
166+
// Cases 2 and 6 are currently not covered as they would require an rpath containing the
167+
// binary filename, which may contain commas that would clash with the `-Wl` argument used to
168+
// pass the rpath to the linker.
169+
// TODO(#14600): Fix this by using `-Xlinker` instead of `-Wl`.
170+
ImmutableList.Builder<String> execRoots = ImmutableList.builder();
171+
// Handles cases 1, 3, 4, 5, and 7.
172+
execRoots.add(Strings.repeat("../", output.getRootRelativePath().segmentCount() - 1));
173+
if (output.getRunfilesPath().startsWith(LabelConstants.EXTERNAL_RUNFILES_PATH_PREFIX)
174+
&& output.getRoot().isLegacy()) {
175+
// Handles case 8a. The runfiles path is of the form ../some_repo/pkg/file and we need to
176+
// walk up some_repo/pkg and then down into main_repo.
177+
execRoots.add(
178+
Strings.repeat("../", output.getRunfilesPath().segmentCount() - 2) + workspaceName
179+
+ "/");
123180
}
124-
rpathRoot = runfilesExecRoot + ccToolchainProvider.getSolibDirectory() + "/";
181+
182+
potentialExecRoots = execRoots.build();
183+
rpathRoots =
184+
potentialExecRoots.stream()
185+
.map((execRoot) -> execRoot + ccToolchainProvider.getSolibDirectory() + "/")
186+
.collect(toImmutableList());
125187
}
126188

127189
ltoMap = generateLtoMap();
@@ -196,10 +258,10 @@ public CollectedLibrariesToLink collectLibrariesToLink() {
196258
// directory. In other words, given blaze-bin/my/package/binary, rpathRoot would be
197259
// "../../_solib_[arch]".
198260
if (needToolchainLibrariesRpath) {
199-
runtimeLibrarySearchDirectories.add(
200-
Strings.repeat("../", outputArtifact.getRootRelativePath().segmentCount() - 1)
201-
+ toolchainLibrariesSolibName
202-
+ "/");
261+
for (String potentialExecRoot : potentialExecRoots) {
262+
runtimeLibrarySearchDirectories.add(
263+
potentialExecRoot + toolchainLibrariesSolibName + "/");
264+
}
203265
}
204266
if (isNativeDeps) {
205267
// We also retain the $ORIGIN/ path to solibs that are in _solib_<arch>, as opposed to
@@ -231,7 +293,9 @@ public CollectedLibrariesToLink collectLibrariesToLink() {
231293
NestedSetBuilder<String> allRuntimeLibrarySearchDirectories = NestedSetBuilder.linkOrder();
232294
// rpath ordering matters for performance; first add the one where most libraries are found.
233295
if (includeSolibDir) {
234-
allRuntimeLibrarySearchDirectories.add(rpathRoot);
296+
for (String rpathRoot : rpathRoots) {
297+
allRuntimeLibrarySearchDirectories.add(rpathRoot);
298+
}
235299
}
236300
allRuntimeLibrarySearchDirectories.addAll(rpathRootsForExplicitSoDeps.build());
237301
if (includeToolchainLibrariesSolibDir) {
@@ -346,17 +410,21 @@ private void addDynamicInputLinkOptions(
346410
// When all dynamic deps are built in transitioned configurations, the default solib dir is
347411
// not created. While resolving paths, the dynamic linker stops at the first directory that
348412
// does not exist, even when followed by "../". We thus have to normalize the relative path.
349-
String relativePathToRoot =
350-
rpathRoot + dotdots + libDir.relativeTo(commonParent).getPathString();
351-
String normalizedPathToRoot = PathFragment.create(relativePathToRoot).getPathString();
352-
rpathRootsForExplicitSoDeps.add(normalizedPathToRoot);
413+
for (String rpathRoot : rpathRoots) {
414+
String relativePathToRoot =
415+
rpathRoot + dotdots + libDir.relativeTo(commonParent).getPathString();
416+
String normalizedPathToRoot = PathFragment.create(relativePathToRoot).getPathString();
417+
rpathRootsForExplicitSoDeps.add(normalizedPathToRoot);
418+
}
353419

354420
// Unless running locally, libraries will be available under the root relative path, so we
355421
// should add that to the rpath as well.
356422
if (inputArtifact.getRootRelativePathString().startsWith("_solib_")) {
357423
PathFragment artifactPathUnderSolib = inputArtifact.getRootRelativePath().subFragment(1);
358-
rpathRootsForExplicitSoDeps.add(
359-
rpathRoot + artifactPathUnderSolib.getParentDirectory().getPathString());
424+
for (String rpathRoot : rpathRoots) {
425+
rpathRootsForExplicitSoDeps.add(
426+
rpathRoot + artifactPathUnderSolib.getParentDirectory().getPathString());
427+
}
360428
}
361429
}
362430

‎src/test/shell/bazel/cc_integration_test.sh‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1382,4 +1382,92 @@ EOF
13821382
fail "bazel build should've succeeded with --features=compiler_param_file"
13831383
}
13841384

1385+
function external_cc_test_setup() {
1386+
cat >> WORKSPACE <<'EOF'
1387+
local_repository(
1388+
name = "other_repo",
1389+
path = "other_repo",
1390+
)
1391+
EOF
1392+
1393+
mkdir -p other_repo
1394+
touch other_repo/WORKSPACE
1395+
1396+
mkdir -p other_repo/lib
1397+
cat > other_repo/lib/BUILD <<'EOF'
1398+
cc_library(
1399+
name = "lib",
1400+
srcs = ["lib.cpp"],
1401+
hdrs = ["lib.h"],
1402+
visibility = ["//visibility:public"],
1403+
)
1404+
EOF
1405+
cat > other_repo/lib/lib.h <<'EOF'
1406+
void print_greeting();
1407+
EOF
1408+
cat > other_repo/lib/lib.cpp <<'EOF'
1409+
#include <cstdio>
1410+
void print_greeting() {
1411+
printf("Hello, world!\n");
1412+
}
1413+
EOF
1414+
1415+
mkdir -p other_repo/test
1416+
cat > other_repo/test/BUILD <<'EOF'
1417+
cc_test(
1418+
name = "test",
1419+
srcs = ["test.cpp"],
1420+
deps = ["//lib"],
1421+
)
1422+
EOF
1423+
cat > other_repo/test/test.cpp <<'EOF'
1424+
#include "lib/lib.h"
1425+
int main() {
1426+
print_greeting();
1427+
}
1428+
EOF
1429+
}
1430+
1431+
function test_external_cc_test_sandboxed() {
1432+
[ "$PLATFORM" != "windows" ] || return 0
1433+
1434+
external_cc_test_setup
1435+
1436+
bazel test \
1437+
--test_output=errors \
1438+
--strategy=sandboxed \
1439+
@other_repo//test >& $TEST_log || fail "Test should pass"
1440+
}
1441+
1442+
function test_external_cc_test_sandboxed_sibling_repository_layout() {
1443+
[ "$PLATFORM" != "windows" ] || return 0
1444+
1445+
external_cc_test_setup
1446+
1447+
bazel test \
1448+
--test_output=errors \
1449+
--strategy=sandboxed \
1450+
--experimental_sibling_repository_layout \
1451+
@other_repo//test >& $TEST_log || fail "Test should pass"
1452+
}
1453+
1454+
function test_external_cc_test_local() {
1455+
external_cc_test_setup
1456+
1457+
bazel test \
1458+
--test_output=errors \
1459+
--strategy=local \
1460+
@other_repo//test >& $TEST_log || fail "Test should pass"
1461+
}
1462+
1463+
function test_external_cc_test_local_sibling_repository_layout() {
1464+
external_cc_test_setup
1465+
1466+
bazel test \
1467+
--test_output=errors \
1468+
--strategy=local \
1469+
--experimental_sibling_repository_layout \
1470+
@other_repo//test >& $TEST_log || fail "Test should pass"
1471+
}
1472+
13851473
run_suite "cc_integration_test"

‎src/test/shell/bazel/remote/remote_execution_test.sh‎

Lines changed: 29 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3811,14 +3811,7 @@ EOF
38113811
expect_log "Executing genrule .* failed: (Exit 1):"
38123812
}
38133813

3814-
function test_external_cc_test() {
3815-
if [[ "$PLATFORM" == "darwin" ]]; then
3816-
# TODO(b/37355380): This test is disabled due to RemoteWorker not supporting
3817-
# setting SDKROOT and DEVELOPER_DIR appropriately, as is required of
3818-
# action executors in order to select the appropriate Xcode toolchain.
3819-
return 0
3820-
fi
3821-
3814+
function setup_external_cc_test() {
38223815
cat >> WORKSPACE <<'EOF'
38233816
local_repository(
38243817
name = "other_repo",
@@ -3862,10 +3855,38 @@ int main() {
38623855
print_greeting();
38633856
}
38643857
EOF
3858+
}
3859+
3860+
function test_external_cc_test() {
3861+
if [[ "$PLATFORM" == "darwin" ]]; then
3862+
# TODO(b/37355380): This test is disabled due to RemoteWorker not supporting
3863+
# setting SDKROOT and DEVELOPER_DIR appropriately, as is required of
3864+
# action executors in order to select the appropriate Xcode toolchain.
3865+
return 0
3866+
fi
3867+
3868+
setup_external_cc_test
3869+
3870+
bazel test \
3871+
--test_output=errors \
3872+
--remote_executor=grpc://localhost:${worker_port} \
3873+
@other_repo//test >& $TEST_log || fail "Test should pass"
3874+
}
3875+
3876+
function test_external_cc_test_sibling_repository_layout() {
3877+
if [[ "$PLATFORM" == "darwin" ]]; then
3878+
# TODO(b/37355380): This test is disabled due to RemoteWorker not supporting
3879+
# setting SDKROOT and DEVELOPER_DIR appropriately, as is required of
3880+
# action executors in order to select the appropriate Xcode toolchain.
3881+
return 0
3882+
fi
3883+
3884+
setup_external_cc_test
38653885

38663886
bazel test \
38673887
--test_output=errors \
38683888
--remote_executor=grpc://localhost:${worker_port} \
3889+
--experimental_sibling_repository_layout \
38693890
@other_repo//test >& $TEST_log || fail "Test should pass"
38703891
}
38713892

0 commit comments

Comments
 (0)