From 8dd6376917f349550686e756fa99523e0f643e25 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sat, 30 Jul 2022 14:19:45 +0200 Subject: [PATCH 1/2] Fix rpath for binaries in external repositories 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. --- .../lib/rules/cpp/CppLinkActionBuilder.java | 4 +- .../rules/cpp/LibrariesToLinkCollector.java | 22 ++++++-- .../bazel/remote/remote_execution_test.sh | 51 +++++++++++++++++++ 3 files changed, 71 insertions(+), 6 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java index 802e3ee474e971..33307e3611b1fc 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java @@ -26,6 +26,7 @@ import com.google.devtools.build.lib.actions.ActionOwner; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ParameterFile; +import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.analysis.RuleErrorConsumer; import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext; import com.google.devtools.build.lib.analysis.actions.ParameterFileWriteAction; @@ -801,7 +802,8 @@ public CppLinkAction build() throws InterruptedException, RuleErrorException { allowLtoIndexing, nonExpandedLinkerInputs, needWholeArchive, - ruleErrorConsumer); + ruleErrorConsumer, + ((RuleContext) actionConstructionContext).getWorkspaceName()); CollectedLibrariesToLink collectedLibrariesToLink = librariesToLinkCollector.collectLibrariesToLink(); 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 761f2aa220dccd..ece64ecd510bca 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 @@ -19,6 +19,7 @@ import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.analysis.RuleErrorConsumer; +import com.google.devtools.build.lib.cmdline.LabelConstants; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration; @@ -69,7 +70,8 @@ public LibrariesToLinkCollector( boolean allowLtoIndexing, Iterable linkerInputs, boolean needWholeArchive, - RuleErrorConsumer ruleErrorConsumer) { + RuleErrorConsumer ruleErrorConsumer, + String workspaceName) { this.isNativeDeps = isNativeDeps; this.cppConfiguration = cppConfiguration; this.ccToolchainProvider = toolchain; @@ -106,10 +108,20 @@ public LibrariesToLinkCollector( // there's no *one* RPATH setting that fits all targets involved in the sharing. rpathRoot = ccToolchainProvider.getSolibDirectory() + "/"; } else { - rpathRoot = - "../".repeat(outputArtifact.getRootRelativePath().segmentCount() - 1) - + ccToolchainProvider.getSolibDirectory() - + "/"; + // 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 = "../".repeat(runfilesPath.segmentCount() - 2) + workspaceName + "/"; + } else { + // runfilesPath is of the form pkg/file, walk back pkg to reach the main workspace. + runfilesExecRoot = "../".repeat(runfilesPath.segmentCount() - 1); + } + rpathRoot = runfilesExecRoot + ccToolchainProvider.getSolibDirectory() + "/"; } ltoMap = generateLtoMap(); diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index a23aeca555df86..052f5a51de4f7a 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -2883,4 +2883,55 @@ EOF || fail "Remote execution generated different result" } +function test_external_cc_test() { + 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 + + bazel test \ + --test_output=errors \ + --remote_executor=grpc://localhost:${worker_port} \ + @other_repo//test >& $TEST_log || fail "Test should pass" +} + run_suite "Remote execution and remote cache tests" From 797a562721c3502a85daffdd0c53e7b45861c36e Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 31 Jul 2022 08:42:05 +0200 Subject: [PATCH 2/2] Skip test on macOS --- src/test/shell/bazel/remote/remote_execution_test.sh | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 052f5a51de4f7a..55655b1429899b 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -2884,6 +2884,13 @@ 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 + cat >> WORKSPACE <<'EOF' local_repository( name = "other_repo",