From fbc1ec375d06541c5d3b010ea5979b9e39a9dbb0 Mon Sep 17 00:00:00 2001 From: Yuval K Date: Sun, 14 Nov 2021 19:56:16 +0200 Subject: [PATCH 1/2] Add the default solib dir to the rpath for cc_imports with transitions PR #14011 took care of cc_libraries, this fixes the same issue for cc_imports. Work towards #13819. --- .../rules/cpp/LibrariesToLinkCollector.java | 7 + .../cpp/CcImportBaseConfiguredTargetTest.java | 124 ++++++++++++++++++ 2 files changed, 131 insertions(+) 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 b5e4d5ac2c2e84..8fbdd1783ed367 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 @@ -332,6 +332,13 @@ private void addDynamicInputLinkOptions( rpathRootsForExplicitSoDeps.add( rpathRoot + dotdots + libDir.relativeTo(commonParent).getPathString()); + + // Unless running locally, libraries will be available under the root relative path, so we + // should add that to the rpath as well. + Preconditions.checkState(inputArtifact.getRootRelativePathString().startsWith("_solib_")); + rpathRootsForExplicitSoDeps.add( + rpathRoot + + inputArtifact.getRootRelativePath().subFragment(1).getParentDirectory().getPathString()); } librarySearchDirectories.add(inputArtifact.getExecPath().getParentDirectory().getPathString()); 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 1ac1fa220baf15..e9a569fabf7393 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 @@ -24,6 +24,7 @@ import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.packages.util.Crosstool.CcToolchainConfig; +import java.util.List; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -405,4 +406,127 @@ private void setupTestCcImportLoadedThroughMacro(boolean loadMacro) throws Excep getAnalysisMock().ccSupport().getMacroLoadStatement(loadMacro, "cc_import"), "cc_import(name='a', static_library='a.a')"); } + + @Test + public void testCcImportWithSharedLibraryAddsRpathEntry() throws Exception { + AnalysisMock.get() + .ccSupport() + .setupCcToolchainConfig( + mockToolsConfig, + CcToolchainConfig.builder().withFeatures(CppRuleClasses.SUPPORTS_DYNAMIC_LINKER)); + useConfiguration("--cpu=k8"); + ConfiguredTarget target = + scratchConfiguredTarget( + "a", + "foo", + starlarkImplementationLoadStatement, + "cc_import(name = 'foo', shared_library = 'libfoo.so')"); + scratch.file( + "bin/BUILD", + "cc_binary(name='bin', deps=['//a:foo'])"); + + Artifact dynamicLibrary = + target + .get(CcInfo.PROVIDER) + .getCcLinkingContext() + .getLibraries() + .getSingleton() + .getResolvedSymlinkDynamicLibrary(); + Iterable dynamicLibrariesForRuntime = + target + .get(CcInfo.PROVIDER) + .getCcLinkingContext() + .getDynamicLibrariesForRuntime(/* linkingStatically= */ false); + assertThat(artifactsToStrings(ImmutableList.of(dynamicLibrary))) + .containsExactly("src a/libfoo.so"); + assertThat(artifactsToStrings(dynamicLibrariesForRuntime)) + .containsExactly("bin _solib_k8/_U_S_Sa_Cfoo___Ua/libfoo.so"); + + ConfiguredTarget main = getConfiguredTarget("//bin:bin"); + 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"); + } + + @Test + public void testCcImportWithSharedLibraryWithTransitionAddsRpathEntry() throws Exception { + AnalysisMock.get() + .ccSupport() + .setupCcToolchainConfig( + mockToolsConfig, + CcToolchainConfig.builder().withFeatures(CppRuleClasses.SUPPORTS_DYNAMIC_LINKER)); + useConfiguration("--cpu=k8"); + ConfiguredTarget target = + scratchConfiguredTarget( + "a", + "foo", + starlarkImplementationLoadStatement, + "cc_import(name = 'foo', shared_library = 'libfoo.so')"); + + scratch.file( + "bin/custom_transition.bzl", + "def _custom_transition_impl(settings, attr):", + " _ignore = settings, attr", + "", + " return {'//command_line_option:copt': ['-DFLAG']}", + "", + "custom_transition = transition(", + " implementation = _custom_transition_impl,", + " inputs = [],", + " outputs = ['//command_line_option:copt'],", + ")", + "", + "def _apply_custom_transition_impl(ctx):", + " cc_infos = []", + " for dep in ctx.attr.deps:", + " cc_infos.append(dep[CcInfo])", + " merged_cc_info = cc_common.merge_cc_infos(cc_infos = cc_infos)", + " return merged_cc_info", + "", + "apply_custom_transition = rule(", + " implementation = _apply_custom_transition_impl,", + " attrs = {", + " '_whitelist_function_transition': attr.label(", + " default = '//tools/allowlists/function_transition_allowlist',", + " ),", + " 'deps': attr.label_list(cfg = custom_transition),", + " },", + ")"); + scratch.overwriteFile( + "tools/allowlists/function_transition_allowlist/BUILD", + "package_group(", + " name = 'function_transition_allowlist',", + " packages = ['//...'],", + ")"); + scratch.file( + "bin/BUILD", + "load(':custom_transition.bzl', 'apply_custom_transition')", + "cc_library(name='lib', deps=['//a:foo'])", + "apply_custom_transition(name='transitioned_lib', deps=[':lib'])", + "cc_binary(name='bin', deps=[':transitioned_lib'])"); + + Artifact dynamicLibrary = + target + .get(CcInfo.PROVIDER) + .getCcLinkingContext() + .getLibraries() + .getSingleton() + .getResolvedSymlinkDynamicLibrary(); + Iterable dynamicLibrariesForRuntime = + target + .get(CcInfo.PROVIDER) + .getCcLinkingContext() + .getDynamicLibrariesForRuntime(/* linkingStatically= */ false); + assertThat(artifactsToStrings(ImmutableList.of(dynamicLibrary))) + .containsExactly("src a/libfoo.so"); + assertThat(artifactsToStrings(dynamicLibrariesForRuntime)) + .containsExactly("bin _solib_k8/_U_S_Sa_Cfoo___Ua/libfoo.so"); + + ConfiguredTarget main = getConfiguredTarget("//bin:bin"); + 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"); + } } From b6707477d2b9377c5f4f5ad0da455534a1cd566e Mon Sep 17 00:00:00 2001 From: Yuval K Date: Sun, 14 Nov 2021 20:58:34 +0200 Subject: [PATCH 2/2] Windows paths seem different - be less strict --- .../build/lib/rules/cpp/LibrariesToLinkCollector.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 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 8fbdd1783ed367..c74c10a583309f 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 @@ -335,10 +335,11 @@ private void addDynamicInputLinkOptions( // Unless running locally, libraries will be available under the root relative path, so we // should add that to the rpath as well. - Preconditions.checkState(inputArtifact.getRootRelativePathString().startsWith("_solib_")); - rpathRootsForExplicitSoDeps.add( - rpathRoot + - inputArtifact.getRootRelativePath().subFragment(1).getParentDirectory().getPathString()); + if (inputArtifact.getRootRelativePathString().startsWith("_solib_")) { + PathFragment artifactPathUnderSolib = inputArtifact.getRootRelativePath().subFragment(1); + rpathRootsForExplicitSoDeps.add( + rpathRoot + artifactPathUnderSolib.getParentDirectory().getPathString()); + } } librarySearchDirectories.add(inputArtifact.getExecPath().getParentDirectory().getPathString());