diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java index aa8658f1f1c31f..1ade3e2dfc0103 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java @@ -549,18 +549,25 @@ public static ImmutableList getLegacyFeatures( " flag_group {", " expand_if_true: 'is_cc_test'", // TODO(b/27153401): This should probably be @loader_path on osx. - " flag: ", - " '-Wl,-rpath,$EXEC_ORIGIN/%{runtime_library_search_directories}'", + " flag: '-Xlinker'", + " flag: '-rpath'", + " flag: '-Xlinker'", + " flag: '$EXEC_ORIGIN/%{runtime_library_search_directories}'", " }", " flag_group {", " expand_if_false: 'is_cc_test'", ifLinux( platform, - " flag: '-Wl,-rpath,$ORIGIN/" - + "%{runtime_library_search_directories}'"), + " flag: '-Xlinker'", + " flag: '-rpath'", + " flag: '-Xlinker'", + " flag: '$ORIGIN/" + "%{runtime_library_search_directories}'"), ifMac( platform, - " flag: '-Wl,-rpath,@loader_path/" + " flag: '-Xlinker'", + " flag: '-rpath'", + " flag: '-Xlinker'", + " flag: '@loader_path/" + "%{runtime_library_search_directories}'"), " }", " }", @@ -579,11 +586,16 @@ public static ImmutableList getLegacyFeatures( " flag_group {", ifLinux( platform, - " flag: '-Wl,-rpath,$ORIGIN/" - + "%{runtime_library_search_directories}'"), + " flag: '-Xlinker'", + " flag: '-rpath'", + " flag: '-Xlinker'", + " flag: '$ORIGIN/" + "%{runtime_library_search_directories}'"), ifMac( platform, - " flag: '-Wl,-rpath,@loader_path/" + " flag: '-Xlinker'", + " flag: '-rpath'", + " flag: '-Xlinker'", + " flag: '@loader_path/" + "%{runtime_library_search_directories}'"), " }", " }", 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 5e38bc46e4375c..a0052121121e84 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 @@ -162,16 +162,25 @@ public LibrariesToLinkCollector( // 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. + // Cases 2 and 6 covered by walking into file.runfiles/main_repo. // 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`. + boolean isExternal = + output.getRunfilesPath().startsWith(LabelConstants.EXTERNAL_RUNFILES_PATH_PREFIX); + boolean usesLegacyRepositoryLayout = output.getRoot().isLegacy(); ImmutableList.Builder execRoots = ImmutableList.builder(); // Handles cases 1, 3, 4, 5, and 7. execRoots.add("../".repeat(output.getRootRelativePath().segmentCount() - 1)); - if (output.getRunfilesPath().startsWith(LabelConstants.EXTERNAL_RUNFILES_PATH_PREFIX) - && output.getRoot().isLegacy()) { + // Handle cases 2 and 6. + String solibRepositoryName; + if (isExternal && !usesLegacyRepositoryLayout) { + // Case 6b + solibRepositoryName = output.getRunfilesPath().getSegment(1); + } else { + // Cases 2 and 6a + solibRepositoryName = workspaceName; + } + execRoots.add(output.getFilename() + ".runfiles/" + solibRepositoryName + "/"); + if (isExternal && usesLegacyRepositoryLayout) { // 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( diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/mock/osx_cc_toolchain_config.bzl b/src/test/java/com/google/devtools/build/lib/packages/util/mock/osx_cc_toolchain_config.bzl index 339c0b8e9eb38c..b3015cb2740214 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/util/mock/osx_cc_toolchain_config.bzl +++ b/src/test/java/com/google/devtools/build/lib/packages/util/mock/osx_cc_toolchain_config.bzl @@ -7922,7 +7922,10 @@ def _impl(ctx): flag_groups = [ flag_group( flags = [ - "-Wl,-rpath,$ORIGIN/%{runtime_library_search_directories}", + "-Xlinker", + "-rpath", + "-Xlinker", + "@loader_path/%{runtime_library_search_directories}", ], iterate_over = "runtime_library_search_directories", expand_if_available = "runtime_library_search_directories", diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcImportBaseConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcImportBaseConfiguredTargetTest.java index d06b9a80fdddeb..1293593be733f3 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcImportBaseConfiguredTargetTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcImportBaseConfiguredTargetTest.java @@ -444,7 +444,9 @@ public void testCcImportWithSharedLibraryAddsRpathEntry() throws Exception { Artifact mainBin = getBinArtifact("bin", main); CppLinkAction action = (CppLinkAction) getGeneratingAction(mainBin); List linkArgv = action.getLinkCommandLine().arguments(); - assertThat(linkArgv).contains("-Wl,-rpath,$ORIGIN/../_solib_k8/_U_S_Sa_Cfoo___Ua"); + assertThat(linkArgv) + .containsAtLeast("-Xlinker", "-rpath", "-Xlinker", "$ORIGIN/../_solib_k8/_U_S_Sa_Cfoo___Ua") + .inOrder(); } @Test @@ -525,6 +527,8 @@ public void testCcImportWithSharedLibraryWithTransitionAddsRpathEntry() throws E Artifact mainBin = getBinArtifact("bin", main); CppLinkAction action = (CppLinkAction) getGeneratingAction(mainBin); List linkArgv = action.getLinkCommandLine().arguments(); - assertThat(linkArgv).contains("-Wl,-rpath,$ORIGIN/../_solib_k8/_U_S_Sa_Cfoo___Ua"); + assertThat(linkArgv) + .containsAtLeast("-Xlinker", "-rpath", "-Xlinker", "$ORIGIN/../_solib_k8/_U_S_Sa_Cfoo___Ua") + .inOrder(); } } diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java index e70c09c4356795..93b4ce7f95b963 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java @@ -2122,7 +2122,7 @@ public void testRpathIsNotAddedWhenThereAreNoSoDeps() throws Exception { Artifact mainBin = getBinArtifact("main", main); CppLinkAction action = (CppLinkAction) getGeneratingAction(mainBin); assertThat(Joiner.on(" ").join(action.getLinkCommandLine().arguments())) - .doesNotContain("-Wl,-rpath"); + .doesNotContain("-Xlinker -rpath"); } @Test @@ -2155,12 +2155,21 @@ public void testRpathAndLinkPathsWithoutTransitions() throws Exception { Artifact mainBin = getBinArtifact("main", main); CppLinkAction action = (CppLinkAction) getGeneratingAction(mainBin); List linkArgv = action.getLinkCommandLine().arguments(); - assertThat(linkArgv).contains("-Wl,-rpath,$ORIGIN/../_solib_k8/"); + assertThat(linkArgv) + .containsAtLeast("-Xlinker", "-rpath", "-Xlinker", "$ORIGIN/../_solib_k8/") + .inOrder(); + assertThat(linkArgv) + .containsAtLeast( + "-Xlinker", + "-rpath", + "-Xlinker", + "$ORIGIN/main.runfiles/" + TestConstants.WORKSPACE_NAME + "/_solib_k8/") + .inOrder(); assertThat(linkArgv) .contains("-L" + TestConstants.PRODUCT_NAME + "-out/k8-fastbuild/bin/_solib_k8"); assertThat(linkArgv).contains("-lno-transition_Slibdep1"); assertThat(Joiner.on(" ").join(linkArgv)) - .doesNotContain("-Wl,-rpath,$ORIGIN/../_solib_k8/../../../k8-fastbuild-ST-"); + .doesNotContain("-Xlinker -rpath -Xlinker $ORIGIN/../_solib_k8/../../../k8-fastbuild-ST-"); assertThat(Joiner.on(" ").join(linkArgv)) .doesNotContain("-L" + TestConstants.PRODUCT_NAME + "-out/k8-fastbuild-ST-"); assertThat(Joiner.on(" ").join(linkArgv)).doesNotContain("-lST-"); @@ -2215,9 +2224,18 @@ public void testRpathRootIsAddedEvenWithTransitionedDepsOnly() throws Exception Artifact mainBin = getBinArtifact("main", main); CppLinkAction action = (CppLinkAction) getGeneratingAction(mainBin); List linkArgv = action.getLinkCommandLine().arguments(); - assertThat(linkArgv).contains("-Wl,-rpath,$ORIGIN/../_solib_k8/"); + assertThat(linkArgv) + .containsAtLeast("-Xlinker", "-rpath", "-Xlinker", "$ORIGIN/../_solib_k8/") + .inOrder(); + assertThat(linkArgv) + .containsAtLeast( + "-Xlinker", + "-rpath", + "-Xlinker", + "$ORIGIN/main.runfiles/" + TestConstants.WORKSPACE_NAME + "/_solib_k8/") + .inOrder(); assertThat(Joiner.on(" ").join(linkArgv)) - .contains("-Wl,-rpath,$ORIGIN/../../../k8-fastbuild-ST-"); + .contains("-Xlinker -rpath -Xlinker $ORIGIN/../../../k8-fastbuild-ST-"); assertThat(Joiner.on(" ").join(linkArgv)) .contains("-L" + TestConstants.PRODUCT_NAME + "-out/k8-fastbuild-ST-"); assertThat(Joiner.on(" ").join(linkArgv)).containsMatch("-lST-[0-9a-f]+_transition_Slibdep2"); diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java index d8c8423f75d682..08ead2a154cca8 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java @@ -260,7 +260,9 @@ public void testLibOptsAndLibSrcsAreInCorrectOrder() throws Exception { ".* -L[^ ]*some-dir(?= ).* -L[^ ]*some-other-dir(?= ).* " + "-lbar -l:qux.so(?= ).* -ldl -lutil .*"); assertThat(Joiner.on(" ").join(arguments)) - .matches(".* -Wl,-rpath[^ ]*some-dir(?= ).* -Wl,-rpath[^ ]*some-other-dir .*"); + .matches( + ".* -Xlinker -rpath -Xlinker [^ ]*some-dir(?= ).* -Xlinker -rpath -Xlinker [^" + + " ]*some-other-dir .*"); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java index 05d9091f09142f..0fcd6bead3acb2 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java @@ -521,7 +521,8 @@ protected static void addAppleBinaryStarlarkRule(Scratch scratch) throws Excepti " if binary_type == 'dylib':", " linkopts.append('-dynamiclib')", " elif binary_type == 'loadable_bundle':", - " linkopts.extend(['-bundle', '-Wl,-rpath,@loader_path/Frameworks'])", + " linkopts.extend(['-bundle', '-Xlinker', '-rpath', '-Xlinker'," + + " '@loader_path/Frameworks'])", " if ctx.attr.bundle_loader:", " bundle_loader = ctx.attr.bundle_loader", " bundle_loader_file = bundle_loader[apple_common.AppleExecutableBinary].binary", @@ -782,7 +783,7 @@ protected void checkBundleLoaderIsCorrectlyPassedToTheLinker(RuleType ruleType) assertThat(Joiner.on(" ").join(linkAction.getArguments())) .contains("-bundle_loader " + getBinArtifact("bin_lipobin", binTarget).getExecPath()); assertThat(Joiner.on(" ").join(linkAction.getArguments())) - .contains("-Wl,-rpath,@loader_path/Frameworks"); + .contains("-Xlinker -rpath -Xlinker @loader_path/Frameworks"); } protected Action lipoLibAction(String libLabel) throws Exception { diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 599c3e1542ef6f..46a5c5ac94fc1d 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -3037,4 +3037,130 @@ EOF [[ $(cat bazel-bin/pkg/b.txt) == non/existent ]] || fail "expected symlink target to be non/existent" } +function setup_cc_binary_tool_with_dynamic_deps() { + local repo=$1 + + cat >> WORKSPACE <<'EOF' +local_repository( + name = "other_repo", + path = "other_repo", +) +EOF + + mkdir -p $repo + touch $repo/WORKSPACE + + mkdir -p $repo/lib + # Use a comma in the target name as that is known to be problematic whith -Wl, + # which is commonly used to pass rpaths to the linker. + cat > $repo/lib/BUILD <<'EOF' +cc_binary( + name = "l,ib", + srcs = ["lib.cpp"], + linkshared = True, + linkstatic = True, +) + +cc_import( + name = "dynamic_l,ib", + shared_library = ":l,ib", + hdrs = ["lib.h"], + visibility = ["//visibility:public"], +) +EOF + cat > $repo/lib/lib.h <<'EOF' +void print_greeting(); +EOF + cat > $repo/lib/lib.cpp <<'EOF' +#include +void print_greeting() { + printf("Hello, world!\n"); +} +EOF + + mkdir -p $repo/pkg + cat > $repo/pkg/BUILD <<'EOF' +cc_binary( + name = "tool", + srcs = ["tool.cpp"], + deps = ["//lib:dynamic_l,ib"], +) + +genrule( + name = "rule", + outs = ["out"], + cmd = "$(location :tool) > $@", + tools = [":tool"], +) +EOF + cat > $repo/pkg/tool.cpp <<'EOF' +#include "lib/lib.h" +int main() { + print_greeting(); +} +EOF +} + +function test_cc_binary_tool_with_dynamic_deps() { + 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_cc_binary_tool_with_dynamic_deps . + + bazel build \ + --remote_executor=grpc://localhost:${worker_port} \ + //pkg:rule >& $TEST_log || fail "Build should succeed" +} + +function test_cc_binary_tool_with_dynamic_deps_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_cc_binary_tool_with_dynamic_deps . + + bazel build \ + --experimental_sibling_repository_layout \ + --remote_executor=grpc://localhost:${worker_port} \ + //pkg:rule >& $TEST_log || fail "Build should succeed" +} + +function test_external_cc_binary_tool_with_dynamic_deps() { + 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_cc_binary_tool_with_dynamic_deps other_repo + + bazel build \ + --remote_executor=grpc://localhost:${worker_port} \ + @other_repo//pkg:rule >& $TEST_log || fail "Build should succeed" +} + +function test_external_cc_binary_tool_with_dynamic_deps_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_cc_binary_tool_with_dynamic_deps other_repo + + bazel build \ + --experimental_sibling_repository_layout \ + --remote_executor=grpc://localhost:${worker_port} \ + @other_repo//pkg:rule >& $TEST_log || fail "Build should succeed" +} + run_suite "Remote execution and remote cache tests" diff --git a/tools/cpp/osx_cc_wrapper.sh.tpl b/tools/cpp/osx_cc_wrapper.sh.tpl index 25ed2ece8e8b4e..ab58d1f1df4fd5 100644 --- a/tools/cpp/osx_cc_wrapper.sh.tpl +++ b/tools/cpp/osx_cc_wrapper.sh.tpl @@ -42,7 +42,7 @@ function parse_option() { LIBS="${BASH_REMATCH[1]} $LIBS" elif [[ "$opt" =~ ^-L(.*)$ ]]; then LIB_DIRS="${BASH_REMATCH[1]} $LIB_DIRS" - elif [[ "$opt" =~ ^-Wl,-rpath,\@loader_path/(.*)$ ]]; then + elif [[ "$opt" =~ ^\@loader_path/(.*)$ ]]; then RPATHS="${BASH_REMATCH[1]} ${RPATHS}" elif [[ "$opt" = "-o" ]]; then # output is coming diff --git a/tools/cpp/unix_cc_toolchain_config.bzl b/tools/cpp/unix_cc_toolchain_config.bzl index 5768a746578804..b951b09c24eed0 100644 --- a/tools/cpp/unix_cc_toolchain_config.bzl +++ b/tools/cpp/unix_cc_toolchain_config.bzl @@ -486,13 +486,19 @@ def _impl(ctx): flag_groups = [ flag_group( flags = [ - "-Wl,-rpath,$EXEC_ORIGIN/%{runtime_library_search_directories}", + "-Xlinker", + "-rpath", + "-Xlinker", + "$EXEC_ORIGIN/%{runtime_library_search_directories}", ], expand_if_true = "is_cc_test", ), flag_group( flags = [ - "-Wl,-rpath,$ORIGIN/%{runtime_library_search_directories}", + "-Xlinker", + "-rpath", + "-Xlinker", + "$ORIGIN/%{runtime_library_search_directories}", ], expand_if_false = "is_cc_test", ), @@ -513,7 +519,10 @@ def _impl(ctx): flag_groups = [ flag_group( flags = [ - "-Wl,-rpath,$ORIGIN/%{runtime_library_search_directories}", + "-Xlinker", + "-rpath", + "-Xlinker", + "$ORIGIN/%{runtime_library_search_directories}", ], ), ], diff --git a/tools/osx/crosstool/cc_toolchain_config.bzl b/tools/osx/crosstool/cc_toolchain_config.bzl index 2574ba86ec5c27..455379604aeef3 100644 --- a/tools/osx/crosstool/cc_toolchain_config.bzl +++ b/tools/osx/crosstool/cc_toolchain_config.bzl @@ -793,7 +793,10 @@ def _impl(ctx): flag_groups = [ flag_group( flags = [ - "-Wl,-rpath,@loader_path/%{runtime_library_search_directories}", + "-Xlinker", + "-rpath", + "-Xlinker", + "@loader_path/%{runtime_library_search_directories}", ], iterate_over = "runtime_library_search_directories", expand_if_available = "runtime_library_search_directories",