From 1e25152906b668bbe56aa4c1773186af85335315 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Mon, 12 Sep 2022 17:24:27 +0200 Subject: [PATCH] Fix local execution of external dynamically linked cc_* targets (#16253) The fix for remote execution of external dynamically linked cc_* targets in 95a5bfdc3ed regressed local execution of such targets. This is fixed by emitting two rpaths in the case of an external target. Fixes https://github.com/bazelbuild/bazel/pull/16008#issuecomment-1224267553 Closes #16214. PiperOrigin-RevId: 473706330 Change-Id: I500f8b1f79e4d994323651fbb5964a730f1136e3 Co-authored-by: kshyanashree <109167932+kshyanashree@users.noreply.github.com> --- .../rules/cpp/LibrariesToLinkCollector.java | 124 ++++++++++++++---- src/test/shell/bazel/cc_integration_test.sh | 88 +++++++++++++ .../bazel/remote/remote_execution_test.sh | 37 ++++-- 3 files changed, 213 insertions(+), 36 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java index 27be6f49d23b6f..fa1797ec4f8222 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java @@ -13,6 +13,8 @@ // limitations under the License. package com.google.devtools.build.lib.rules.cpp; +import static com.google.common.collect.ImmutableList.toImmutableList; + import com.google.common.base.Preconditions; import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; @@ -40,7 +42,6 @@ public class LibrariesToLinkCollector { private final PathFragment toolchainLibrariesSolibDir; private final CppConfiguration cppConfiguration; private final CcToolchainProvider ccToolchainProvider; - private final Artifact outputArtifact; private final boolean isLtoIndexing; private final PathFragment solibDir; private final Iterable linkerInputs; @@ -49,7 +50,8 @@ public class LibrariesToLinkCollector { private final Artifact thinltoParamFile; private final FeatureConfiguration featureConfiguration; private final boolean needWholeArchive; - private final String rpathRoot; + private final ImmutableList potentialExecRoots; + private final ImmutableList rpathRoots; private final boolean needToolchainLibrariesRpath; private final Map ltoMap; private final RuleErrorConsumer ruleErrorConsumer; @@ -76,7 +78,6 @@ public LibrariesToLinkCollector( this.cppConfiguration = cppConfiguration; this.ccToolchainProvider = toolchain; this.toolchainLibrariesSolibDir = toolchainLibrariesSolibDir; - this.outputArtifact = output; this.solibDir = solibDir; this.isLtoIndexing = isLtoIndexing; this.allLtoArtifacts = allLtoArtifacts; @@ -106,22 +107,83 @@ public LibrariesToLinkCollector( // and the second could use $ORIGIN/../_solib_[arch]. But since this is a shared // artifact, both are symlinks to the same place, so // there's no *one* RPATH setting that fits all targets involved in the sharing. - rpathRoot = ccToolchainProvider.getSolibDirectory() + "/"; + potentialExecRoots = ImmutableList.of(); + rpathRoots = ImmutableList.of(ccToolchainProvider.getSolibDirectory() + "/"); } else { - // When executed from within a runfiles directory, the binary lies under a path such as - // target.runfiles/some_repo/pkg/file, whereas the solib directory is located under - // target.runfiles/main_repo. - PathFragment runfilesPath = outputArtifact.getRunfilesPath(); - String runfilesExecRoot; - if (runfilesPath.startsWith(LabelConstants.EXTERNAL_RUNFILES_PATH_PREFIX)) { - // runfilesPath is of the form ../some_repo/pkg/file, walk back some_repo/pkg and then - // descend into the main workspace. - runfilesExecRoot = Strings.repeat("../", runfilesPath.segmentCount() - 2) + workspaceName + "/"; - } else { - // runfilesPath is of the form pkg/file, walk back pkg to reach the main workspace. - runfilesExecRoot = Strings.repeat("../", runfilesPath.segmentCount() - 1); + // The runtime location of the solib directory relative to the binary depends on four factors: + // + // * whether the binary is contained in the main repository or an external repository; + // * whether the binary is executed directly or from a runfiles tree; + // * whether the binary is staged as a symlink (sandboxed execution; local execution if the + // binary is in the runfiles of another target) or a regular file (remote execution) - the + // dynamic linker follows sandbox and runfiles symlinks into its location under the + // unsandboxed execroot, which thus becomes the effective $ORIGIN; + // * whether --experimental_sibling_repository_layout is enabled or not. + // + // The rpaths emitted into the binary thus have to cover the following cases (assuming that + // the binary target is located in the pkg `pkg` and has name `file`) for the directory used + // as $ORIGIN by the dynamic linker and the directory containing the solib directories: + // + // 1. main, direct, symlink: + // $ORIGIN: $EXECROOT/pkg + // solib root: $EXECROOT + // 2. main, direct, regular file: + // $ORIGIN: $EXECROOT/pkg + // solib root: $EXECROOT/pkg/file.runfiles/main_repo + // 3. main, runfiles, symlink: + // $ORIGIN: $EXECROOT/pkg + // solib root: $EXECROOT + // 4. main, runfiles, regular file: + // $ORIGIN: other_target.runfiles/main_repo/pkg + // solib root: other_target.runfiles/main_repo + // 5a. external, direct, symlink: + // $ORIGIN: $EXECROOT/external/other_repo/pkg + // solib root: $EXECROOT + // 5b. external, direct, symlink, with --experimental_sibling_repository_layout: + // $ORIGIN: $EXECROOT/../other_repo/pkg + // solib root: $EXECROOT/../other_repo + // 6a. external, direct, regular file: + // $ORIGIN: $EXECROOT/external/other_repo/pkg + // solib root: $EXECROOT/external/other_repo/pkg/file.runfiles/main_repo + // 6b. external, direct, regular file, with --experimental_sibling_repository_layout: + // $ORIGIN: $EXECROOT/../other_repo/pkg + // solib root: $EXECROOT/../other_repo/pkg/file.runfiles/other_repo + // 7a. external, runfiles, symlink: + // $ORIGIN: $EXECROOT/external/other_repo/pkg + // solib root: $EXECROOT + // 7b. external, runfiles, symlink, with --experimental_sibling_repository_layout: + // $ORIGIN: $EXECROOT/../other_repo/pkg + // solib root: $EXECROOT/../other_repo + // 8a. external, runfiles, regular file: + // $ORIGIN: other_target.runfiles/some_repo/pkg + // solib root: other_target.runfiles/main_repo + // 8b. external, runfiles, regular file, with --experimental_sibling_repository_layout: + // $ORIGIN: other_target.runfiles/some_repo/pkg + // solib root: other_target.runfiles/some_repo + // + // Cases 1, 3, 4, 5, 7, and 8b are covered by an rpath that walks up the root relative path. + // Case 8a is covered by walking up some_repo/pkg and then into main_repo. + // Cases 2 and 6 are currently not covered as they would require an rpath containing the + // binary filename, which may contain commas that would clash with the `-Wl` argument used to + // pass the rpath to the linker. + // TODO(#14600): Fix this by using `-Xlinker` instead of `-Wl`. + ImmutableList.Builder execRoots = ImmutableList.builder(); + // Handles cases 1, 3, 4, 5, and 7. + execRoots.add(Strings.repeat("../", output.getRootRelativePath().segmentCount() - 1)); + if (output.getRunfilesPath().startsWith(LabelConstants.EXTERNAL_RUNFILES_PATH_PREFIX) + && output.getRoot().isLegacy()) { + // Handles case 8a. The runfiles path is of the form ../some_repo/pkg/file and we need to + // walk up some_repo/pkg and then down into main_repo. + execRoots.add( + Strings.repeat("../", output.getRunfilesPath().segmentCount() - 2) + workspaceName + + "/"); } - rpathRoot = runfilesExecRoot + ccToolchainProvider.getSolibDirectory() + "/"; + + potentialExecRoots = execRoots.build(); + rpathRoots = + potentialExecRoots.stream() + .map((execRoot) -> execRoot + ccToolchainProvider.getSolibDirectory() + "/") + .collect(toImmutableList()); } ltoMap = generateLtoMap(); @@ -196,10 +258,10 @@ public CollectedLibrariesToLink collectLibrariesToLink() { // directory. In other words, given blaze-bin/my/package/binary, rpathRoot would be // "../../_solib_[arch]". if (needToolchainLibrariesRpath) { - runtimeLibrarySearchDirectories.add( - Strings.repeat("../", outputArtifact.getRootRelativePath().segmentCount() - 1) - + toolchainLibrariesSolibName - + "/"); + for (String potentialExecRoot : potentialExecRoots) { + runtimeLibrarySearchDirectories.add( + potentialExecRoot + toolchainLibrariesSolibName + "/"); + } } if (isNativeDeps) { // We also retain the $ORIGIN/ path to solibs that are in _solib_, as opposed to @@ -231,7 +293,9 @@ public CollectedLibrariesToLink collectLibrariesToLink() { NestedSetBuilder allRuntimeLibrarySearchDirectories = NestedSetBuilder.linkOrder(); // rpath ordering matters for performance; first add the one where most libraries are found. if (includeSolibDir) { - allRuntimeLibrarySearchDirectories.add(rpathRoot); + for (String rpathRoot : rpathRoots) { + allRuntimeLibrarySearchDirectories.add(rpathRoot); + } } allRuntimeLibrarySearchDirectories.addAll(rpathRootsForExplicitSoDeps.build()); if (includeToolchainLibrariesSolibDir) { @@ -346,17 +410,21 @@ private void addDynamicInputLinkOptions( // When all dynamic deps are built in transitioned configurations, the default solib dir is // not created. While resolving paths, the dynamic linker stops at the first directory that // does not exist, even when followed by "../". We thus have to normalize the relative path. - String relativePathToRoot = - rpathRoot + dotdots + libDir.relativeTo(commonParent).getPathString(); - String normalizedPathToRoot = PathFragment.create(relativePathToRoot).getPathString(); - rpathRootsForExplicitSoDeps.add(normalizedPathToRoot); + for (String rpathRoot : rpathRoots) { + String relativePathToRoot = + rpathRoot + dotdots + libDir.relativeTo(commonParent).getPathString(); + String normalizedPathToRoot = PathFragment.create(relativePathToRoot).getPathString(); + rpathRootsForExplicitSoDeps.add(normalizedPathToRoot); + } // Unless running locally, libraries will be available under the root relative path, so we // should add that to the rpath as well. if (inputArtifact.getRootRelativePathString().startsWith("_solib_")) { PathFragment artifactPathUnderSolib = inputArtifact.getRootRelativePath().subFragment(1); - rpathRootsForExplicitSoDeps.add( - rpathRoot + artifactPathUnderSolib.getParentDirectory().getPathString()); + for (String rpathRoot : rpathRoots) { + rpathRootsForExplicitSoDeps.add( + rpathRoot + artifactPathUnderSolib.getParentDirectory().getPathString()); + } } } diff --git a/src/test/shell/bazel/cc_integration_test.sh b/src/test/shell/bazel/cc_integration_test.sh index b86d97c8a0e06d..3528acc46152aa 100755 --- a/src/test/shell/bazel/cc_integration_test.sh +++ b/src/test/shell/bazel/cc_integration_test.sh @@ -1382,4 +1382,92 @@ EOF fail "bazel build should've succeeded with --features=compiler_param_file" } +function external_cc_test_setup() { + cat >> WORKSPACE <<'EOF' +local_repository( + name = "other_repo", + path = "other_repo", +) +EOF + + mkdir -p other_repo + touch other_repo/WORKSPACE + + mkdir -p other_repo/lib + cat > other_repo/lib/BUILD <<'EOF' +cc_library( + name = "lib", + srcs = ["lib.cpp"], + hdrs = ["lib.h"], + visibility = ["//visibility:public"], +) +EOF + cat > other_repo/lib/lib.h <<'EOF' +void print_greeting(); +EOF + cat > other_repo/lib/lib.cpp <<'EOF' +#include +void print_greeting() { + printf("Hello, world!\n"); +} +EOF + + mkdir -p other_repo/test + cat > other_repo/test/BUILD <<'EOF' +cc_test( + name = "test", + srcs = ["test.cpp"], + deps = ["//lib"], +) +EOF + cat > other_repo/test/test.cpp <<'EOF' +#include "lib/lib.h" +int main() { + print_greeting(); +} +EOF +} + +function test_external_cc_test_sandboxed() { + [ "$PLATFORM" != "windows" ] || return 0 + + external_cc_test_setup + + bazel test \ + --test_output=errors \ + --strategy=sandboxed \ + @other_repo//test >& $TEST_log || fail "Test should pass" +} + +function test_external_cc_test_sandboxed_sibling_repository_layout() { + [ "$PLATFORM" != "windows" ] || return 0 + + external_cc_test_setup + + bazel test \ + --test_output=errors \ + --strategy=sandboxed \ + --experimental_sibling_repository_layout \ + @other_repo//test >& $TEST_log || fail "Test should pass" +} + +function test_external_cc_test_local() { + external_cc_test_setup + + bazel test \ + --test_output=errors \ + --strategy=local \ + @other_repo//test >& $TEST_log || fail "Test should pass" +} + +function test_external_cc_test_local_sibling_repository_layout() { + external_cc_test_setup + + bazel test \ + --test_output=errors \ + --strategy=local \ + --experimental_sibling_repository_layout \ + @other_repo//test >& $TEST_log || fail "Test should pass" +} + run_suite "cc_integration_test" diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 318fdccd25cd54..6b3cab2e81658a 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -3811,14 +3811,7 @@ EOF expect_log "Executing genrule .* failed: (Exit 1):" } -function test_external_cc_test() { - if [[ "$PLATFORM" == "darwin" ]]; then - # TODO(b/37355380): This test is disabled due to RemoteWorker not supporting - # setting SDKROOT and DEVELOPER_DIR appropriately, as is required of - # action executors in order to select the appropriate Xcode toolchain. - return 0 - fi - +function setup_external_cc_test() { cat >> WORKSPACE <<'EOF' local_repository( name = "other_repo", @@ -3862,10 +3855,38 @@ int main() { print_greeting(); } EOF +} + +function test_external_cc_test() { + if [[ "$PLATFORM" == "darwin" ]]; then + # TODO(b/37355380): This test is disabled due to RemoteWorker not supporting + # setting SDKROOT and DEVELOPER_DIR appropriately, as is required of + # action executors in order to select the appropriate Xcode toolchain. + return 0 + fi + + setup_external_cc_test + + bazel test \ + --test_output=errors \ + --remote_executor=grpc://localhost:${worker_port} \ + @other_repo//test >& $TEST_log || fail "Test should pass" +} + +function test_external_cc_test_sibling_repository_layout() { + if [[ "$PLATFORM" == "darwin" ]]; then + # TODO(b/37355380): This test is disabled due to RemoteWorker not supporting + # setting SDKROOT and DEVELOPER_DIR appropriately, as is required of + # action executors in order to select the appropriate Xcode toolchain. + return 0 + fi + + setup_external_cc_test bazel test \ --test_output=errors \ --remote_executor=grpc://localhost:${worker_port} \ + --experimental_sibling_repository_layout \ @other_repo//test >& $TEST_log || fail "Test should pass" }