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

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

Merged
merged 2 commits into from
Sep 12, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
25af0f9
Fix local execution of external dynamically linked cc_* targets
fmeum Sep 12, 2022
2bb36a2
Merge branch 'release-5.3.1' into release-5.3.1
ShreeM01 Sep 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 96 additions & 28 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<? extends LinkerInput> linkerInputs;
Expand All @@ -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<String> potentialExecRoots;
private final ImmutableList<String> rpathRoots;
private final boolean needToolchainLibrariesRpath;
private final Map<Artifact, Artifact> ltoMap;
private final RuleErrorConsumer ruleErrorConsumer;
Expand All @@ -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;
Expand Down Expand Up @@ -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<String> 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();
Expand Down Expand Up @@ -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_<arch>, as opposed to
Expand Down Expand Up @@ -231,7 +293,9 @@ public CollectedLibrariesToLink collectLibrariesToLink() {
NestedSetBuilder<String> 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) {
Expand Down Expand Up @@ -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());
}
}
}

Expand Down
88 changes: 88 additions & 0 deletions src/test/shell/bazel/cc_integration_test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 <cstdio>
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"
37 changes: 29 additions & 8 deletions src/test/shell/bazel/remote/remote_execution_test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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"
}

Expand Down