Skip to content

Commit

Permalink
Fix rpath for binaries in external repositories (#16079)
Browse files Browse the repository at this point in the history
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.

Fixes #16003

Closes #16008.

PiperOrigin-RevId: 466634083
Change-Id: I4ada28b459f23f68a2091dbaad9147cfec2fbe43
  • Loading branch information
fmeum authored Aug 10, 2022
1 parent 82452c7 commit e745468
Show file tree
Hide file tree
Showing 3 changed files with 78 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -798,7 +799,8 @@ public CppLinkAction build() throws InterruptedException, RuleErrorException {
allowLtoIndexing,
nonExpandedLinkerInputs,
needWholeArchive,
ruleErrorConsumer);
ruleErrorConsumer,
((RuleContext) actionConstructionContext).getWorkspaceName());
CollectedLibrariesToLink collectedLibrariesToLink =
librariesToLinkCollector.collectLibrariesToLink();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,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;
Expand Down Expand Up @@ -69,7 +70,8 @@ public LibrariesToLinkCollector(
boolean allowLtoIndexing,
Iterable<LinkerInput> linkerInputs,
boolean needWholeArchive,
RuleErrorConsumer ruleErrorConsumer) {
RuleErrorConsumer ruleErrorConsumer,
String workspaceName) {
this.isNativeDeps = isNativeDeps;
this.cppConfiguration = cppConfiguration;
this.ccToolchainProvider = toolchain;
Expand Down Expand Up @@ -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 =
Strings.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 = 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);
}
rpathRoot = runfilesExecRoot + ccToolchainProvider.getSolibDirectory() + "/";
}

ltoMap = generateLtoMap();
Expand Down
58 changes: 58 additions & 0 deletions src/test/shell/bazel/remote/remote_execution_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -3811,4 +3811,62 @@ 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

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

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"

0 comments on commit e745468

Please sign in to comment.