From 2cfda71699e9e8d2d17ebd71e348bec45bfee8b4 Mon Sep 17 00:00:00 2001 From: Zach Yu Date: Thu, 15 Dec 2022 14:43:22 -0800 Subject: [PATCH 1/2] Refactor logics to find runtime library search directories * Extracts logic to find execroots as a separate method and moves it into the `collectLibrariesToLink()` method from the constructor. * Extracts logic to create rpaths for toolchain libraries as a separate method. * Reorganizes the logic for better readability. This debloats the constructor and makes further changes easier. --- .../rules/cpp/LibrariesToLinkCollector.java | 278 +++++++++--------- 1 file changed, 140 insertions(+), 138 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 a0052121121e84..3e919ad22cb29f 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 @@ -50,11 +50,10 @@ public class LibrariesToLinkCollector { private final Artifact thinltoParamFile; private final FeatureConfiguration featureConfiguration; private final boolean needWholeArchive; - private final ImmutableList potentialExecRoots; - private final ImmutableList rpathRoots; private final boolean needToolchainLibrariesRpath; - private final Map ltoMap; private final RuleErrorConsumer ruleErrorConsumer; + private final Artifact output; + private final String workspaceName; public LibrariesToLinkCollector( boolean isNativeDeps, @@ -87,114 +86,13 @@ public LibrariesToLinkCollector( this.linkerInputs = linkerInputs; this.needWholeArchive = needWholeArchive; this.ruleErrorConsumer = ruleErrorConsumer; + this.output = output; + this.workspaceName = workspaceName; needToolchainLibrariesRpath = toolchainLibrariesSolibDir != null && (linkType.isDynamicLibrary() || (linkType == LinkTargetType.EXECUTABLE && linkingMode == LinkingMode.DYNAMIC)); - - // Calculate the correct relative value for the "-rpath" link option (which sets - // the search path for finding shared libraries). - if (isNativeDeps && cppConfiguration.shareNativeDeps()) { - // For shared native libraries, special symlinking is applied to ensure C++ - // toolchain libraries are available under $ORIGIN/_solib_[arch]. So we set the RPATH to find - // them. - // - // Note that we have to do this because $ORIGIN points to different paths for - // different targets. In other words, blaze-bin/d1/d2/d3/a_shareddeps.so and - // blaze-bin/d4/b_shareddeps.so have different path depths. The first could - // reference a standard blaze-bin/_solib_[arch] via $ORIGIN/../../../_solib[arch], - // 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. - potentialExecRoots = ImmutableList.of(); - rpathRoots = ImmutableList.of(ccToolchainProvider.getSolibDirectory() + "/"); - } else { - // 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. - // 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. - 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)); - // 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( - "../".repeat(output.getRunfilesPath().segmentCount() - 2) + workspaceName + "/"); - } - - potentialExecRoots = execRoots.build(); - rpathRoots = - potentialExecRoots.stream() - .map((execRoot) -> execRoot + ccToolchainProvider.getSolibDirectory() + "/") - .collect(toImmutableList()); - } - - ltoMap = generateLtoMap(); } /** @@ -236,6 +134,109 @@ public NestedSet getRuntimeLibrarySearchDirectories() { } } + private NestedSet collectToolchainRuntimeLibrarySearchDirectories( + ImmutableList potentialSolibParents) { + NestedSetBuilder runtimeLibrarySearchDirectories = NestedSetBuilder.linkOrder(); + if (!needToolchainLibrariesRpath) { + return runtimeLibrarySearchDirectories.build(); + } + + String toolchainLibrariesSolibName = toolchainLibrariesSolibDir.getBaseName(); + if (!(isNativeDeps && cppConfiguration.shareNativeDeps())) { + for (String potentialExecRoot : potentialSolibParents) { + runtimeLibrarySearchDirectories.add(potentialExecRoot + toolchainLibrariesSolibName + "/"); + } + } + if (isNativeDeps) { + runtimeLibrarySearchDirectories.add("../" + toolchainLibrariesSolibName + "/"); + runtimeLibrarySearchDirectories.add("."); + } + runtimeLibrarySearchDirectories.add(toolchainLibrariesSolibName + "/"); + + return runtimeLibrarySearchDirectories.build(); + } + + private ImmutableList findPotentialSolibParents() { + // 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. + // 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. + boolean isExternal = + output.getRunfilesPath().startsWith(LabelConstants.EXTERNAL_RUNFILES_PATH_PREFIX); + boolean usesLegacyRepositoryLayout = output.getRoot().isLegacy(); + ImmutableList.Builder solibParents = ImmutableList.builder(); + // Handles cases 1, 3, 4, 5, and 7. + solibParents.add("../".repeat(output.getRootRelativePath().segmentCount() - 1)); + // Handle cases 2 and 6. + String solibRepositoryName; + if (isExternal && !usesLegacyRepositoryLayout) { + // Case 6b + solibRepositoryName = output.getRunfilesPath().getSegment(1); + } else { + // Cases 2 and 6a + solibRepositoryName = workspaceName; + } + solibParents.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. + solibParents.add( + "../".repeat(output.getRunfilesPath().segmentCount() - 2) + workspaceName + "/"); + } + + return solibParents.build(); + } + /** * When linking a shared library fully or mostly static then we need to link in *all* dependent * files, not just what the shared library needs for its own code. This is done by wrapping all @@ -247,48 +248,43 @@ public NestedSet getRuntimeLibrarySearchDirectories() { */ public CollectedLibrariesToLink collectLibrariesToLink() { NestedSetBuilder librarySearchDirectories = NestedSetBuilder.linkOrder(); - NestedSetBuilder runtimeLibrarySearchDirectories = NestedSetBuilder.linkOrder(); ImmutableSet.Builder rpathRootsForExplicitSoDeps = ImmutableSet.builder(); NestedSetBuilder expandedLinkerInputsBuilder = NestedSetBuilder.linkOrder(); // List of command line parameters that need to be placed *outside* of // --whole-archive ... --no-whole-archive. SequenceBuilder librariesToLink = new SequenceBuilder(); - String toolchainLibrariesSolibName = - toolchainLibrariesSolibDir != null ? toolchainLibrariesSolibDir.getBaseName() : null; + ImmutableList potentialSolibParents; + ImmutableList rpathRoots; + // Calculate the correct relative value for the "-rpath" link option (which sets + // the search path for finding shared libraries). if (isNativeDeps && cppConfiguration.shareNativeDeps()) { - if (needToolchainLibrariesRpath) { - runtimeLibrarySearchDirectories.add("../" + toolchainLibrariesSolibName + "/"); - } + // For shared native libraries, special symlinking is applied to ensure C++ + // toolchain libraries are available under $ORIGIN/_solib_[arch]. So we set the RPATH to find + // them. + // + // Note that we have to do this because $ORIGIN points to different paths for + // different targets. In other words, blaze-bin/d1/d2/d3/a_shareddeps.so and + // blaze-bin/d4/b_shareddeps.so have different path depths. The first could + // reference a standard blaze-bin/_solib_[arch] via $ORIGIN/../../../_solib[arch], + // 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. + potentialSolibParents = ImmutableList.of(); + rpathRoots = ImmutableList.of(ccToolchainProvider.getSolibDirectory() + "/"); } else { - // For all other links, calculate the relative path from the output file to _solib_[arch] - // (the directory where all shared libraries are stored, which resides under the blaze-bin - // directory. In other words, given blaze-bin/my/package/binary, rpathRoot would be - // "../../_solib_[arch]". - if (needToolchainLibrariesRpath) { - for (String potentialExecRoot : potentialExecRoots) { - runtimeLibrarySearchDirectories.add( - potentialExecRoot + toolchainLibrariesSolibName + "/"); - } - } - if (isNativeDeps) { - // We also retain the $ORIGIN/ path to solibs that are in _solib_, as opposed to - // the package directory) - if (needToolchainLibrariesRpath) { - runtimeLibrarySearchDirectories.add("../" + toolchainLibrariesSolibName + "/"); - } - } - } - - if (needToolchainLibrariesRpath) { - if (isNativeDeps) { - runtimeLibrarySearchDirectories.add("."); - } - runtimeLibrarySearchDirectories.add(toolchainLibrariesSolibName + "/"); + potentialSolibParents = findPotentialSolibParents(); + rpathRoots = + potentialSolibParents.stream() + .map((execRoot) -> execRoot + ccToolchainProvider.getSolibDirectory() + "/") + .collect(toImmutableList()); } + Map ltoMap = generateLtoMap(); Pair includeSolibsPair = addLinkerInputs( + rpathRoots, + ltoMap, librarySearchDirectories, rpathRootsForExplicitSoDeps, librariesToLink, @@ -307,7 +303,8 @@ public CollectedLibrariesToLink collectLibrariesToLink() { } allRuntimeLibrarySearchDirectories.addAll(rpathRootsForExplicitSoDeps.build()); if (includeToolchainLibrariesSolibDir) { - allRuntimeLibrarySearchDirectories.addTransitive(runtimeLibrarySearchDirectories.build()); + allRuntimeLibrarySearchDirectories.addTransitive( + collectToolchainRuntimeLibrarySearchDirectories(potentialSolibParents)); } return new CollectedLibrariesToLink( @@ -318,6 +315,8 @@ public CollectedLibrariesToLink collectLibrariesToLink() { } private Pair addLinkerInputs( + ImmutableList rpathRoots, + Map ltoMap, NestedSetBuilder librarySearchDirectories, ImmutableSet.Builder rpathEntries, SequenceBuilder librariesToLink, @@ -366,9 +365,10 @@ private Pair addLinkerInputs( librariesToLink, expandedLinkerInputsBuilder, librarySearchDirectories, + rpathRoots, rpathEntries); } else { - addStaticInputLinkOptions(input, librariesToLink, expandedLinkerInputsBuilder); + addStaticInputLinkOptions(input, ltoMap, librariesToLink, expandedLinkerInputsBuilder); } } return Pair.of(includeSolibDir, includeToolchainLibrariesSolibDir); @@ -384,6 +384,7 @@ private void addDynamicInputLinkOptions( SequenceBuilder librariesToLink, NestedSetBuilder expandedLinkerInputsBuilder, NestedSetBuilder librarySearchDirectories, + ImmutableList rpathRoots, ImmutableSet.Builder rpathRootsForExplicitSoDeps) { Preconditions.checkState( input.getArtifactCategory() == ArtifactCategory.DYNAMIC_LIBRARY @@ -470,6 +471,7 @@ private void addDynamicInputLinkOptions( */ private void addStaticInputLinkOptions( LinkerInput input, + Map ltoMap, SequenceBuilder librariesToLink, NestedSetBuilder expandedLinkerInputsBuilder) { ArtifactCategory artifactCategory = input.getArtifactCategory(); From da9ef5bea3572741701d0c711fd6139699bde89a Mon Sep 17 00:00:00 2001 From: Zach Yu Date: Mon, 9 Jan 2023 18:05:05 -0800 Subject: [PATCH 2/2] Correctly compute RPATHs for toolchain solib. When sibling repository layout is used, the toolchain solib directory is not co-located with the main solib (solib_). Therefore, the RPATHs for toolchain solib need to be computed separately, in a similar manner as how they are computed for the main solib. This change uses a private implementation of `getRelative()` function, rather than the `PathFragment.relativeTo()` method. The reason is that `PathFragment.relativeTo()` requires the base path to be a prefix which is not true in the cases here. Fixes #16956 --- .../rules/cpp/LibrariesToLinkCollector.java | 154 ++++++++++- .../google/devtools/build/lib/rules/cpp/BUILD | 17 ++ .../cpp/LibrariesToLinkCollectorTest.java | 241 ++++++++++++++++++ 3 files changed, 410 insertions(+), 2 deletions(-) create mode 100644 src/test/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollectorTest.java 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 3e919ad22cb29f..0a0f720021d7db 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 @@ -15,6 +15,7 @@ import static com.google.common.collect.ImmutableList.toImmutableList; +import com.google.common.base.Joiner; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; @@ -22,6 +23,7 @@ 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.cmdline.RepositoryName; 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; @@ -30,7 +32,9 @@ import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType; import com.google.devtools.build.lib.rules.cpp.Link.LinkingMode; import com.google.devtools.build.lib.util.Pair; +import com.google.devtools.build.lib.vfs.OsPathPolicy; import com.google.devtools.build.lib.vfs.PathFragment; +import java.util.Collections; import java.util.HashMap; import java.util.Map; import javax.annotation.Nullable; @@ -38,6 +42,9 @@ /** Class that goes over linker inputs and produces {@link LibraryToLinkValue}s */ public class LibrariesToLinkCollector { + private static final OsPathPolicy OS = OsPathPolicy.getFilePathOs(); + private static final Joiner PATH_JOINER = Joiner.on(PathFragment.SEPARATOR_CHAR); + private final boolean isNativeDeps; private final PathFragment toolchainLibrariesSolibDir; private final CppConfiguration cppConfiguration; @@ -143,7 +150,7 @@ private NestedSet collectToolchainRuntimeLibrarySearchDirectories( String toolchainLibrariesSolibName = toolchainLibrariesSolibDir.getBaseName(); if (!(isNativeDeps && cppConfiguration.shareNativeDeps())) { - for (String potentialExecRoot : potentialSolibParents) { + for (String potentialExecRoot : findToolchainSolibParents(potentialSolibParents)) { runtimeLibrarySearchDirectories.add(potentialExecRoot + toolchainLibrariesSolibName + "/"); } } @@ -237,6 +244,149 @@ private ImmutableList findPotentialSolibParents() { return solibParents.build(); } + private ImmutableList findToolchainSolibParents( + ImmutableList potentialSolibParents) { + boolean usesLegacyRepositoryLayout = output.getRoot().isLegacy(); + // When -experimental_sibling_repository_layout is not enabled, the toolchain solib sits next to + // the solib_ directory - so that it shares the same parents. + if (usesLegacyRepositoryLayout) { + return potentialSolibParents; + } + + // When -experimental_sibling_repository_layout is enabled, the toolchain solib is located in + // these 2 places: + // 1. The `bin` directory of the repository where the toolchain target is declared (this is the + // parent directory of `toolchainLibrariesSolibDir`). + // 2. In `target.runfiles/` + // + // And the following factors affect what $ORIGIN is resolved to: + // * 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; + // + // 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: + // 2. main, direct, regular file: + // $ORIGIN: $EXECROOT/pkg + // solib root: $EXECROOT/pkg/file.runfiles/ + // 3. main, runfiles, symlink: + // $ORIGIN: $EXECROOT/pkg + // solib root: + // 4. main, runfiles, regular file: + // $ORIGIN: other_target.runfiles/main_repo/pkg + // solib root: other_target.runfiles/ + // 5. external, direct, symlink: + // $ORIGIN: $EXECROOT/../other_repo/pkg + // solib root: + // 6. external, direct, regular file: + // $ORIGIN: $EXECROOT/../other_repo/pkg + // solib root: $EXECROOT/../other_repo/pkg/file.runfiles/ + // 7. external, runfiles, symlink: + // $ORIGIN: $EXECROOT/../other_repo/pkg + // solib root: + // 8. external, runfiles, regular file: + // $ORIGIN: other_target.runfiles/some_repo/pkg + // solib root: other_target.runfiles/ + // + // For cases 1, 3, 5, 7, we need to compute the relative path from the output artifact to + // toolchain repo's bin directory. For 2 and 6, we walk down into `file.runfiles/`. For 4 and 8, we need to compute the relative path from the output runfile to + // under runfiles. + ImmutableList.Builder solibParents = ImmutableList.builder(); + + // Cases 1, 3, 5, 7 + PathFragment toolchainBinExecPath = + toolchainLibrariesSolibDir.getParentDirectory(); + PathFragment binaryOriginExecPath = output.getExecPath().getParentDirectory(); + solibParents.add( + getRelative(binaryOriginExecPath, toolchainBinExecPath).getPathString() + + PathFragment.SEPARATOR_CHAR); + + // Cases 2 and 6 + String toolchainRunfilesRepoName = + getRunfilesRepoName(ccToolchainProvider.getCcToolchainLabel().getRepository()); + solibParents.add( + PATH_JOINER.join(output.getFilename() + ".runfiles", toolchainRunfilesRepoName) + + PathFragment.SEPARATOR_CHAR); + + // Cases 4 and 8 + String binaryRepoName = getRunfilesRepoName( + output.getOwnerLabel().getRepository()); + PathFragment toolchainBinRunfilesPath = PathFragment.create(toolchainRunfilesRepoName); + PathFragment binaryOriginRunfilesPath = PathFragment.create(binaryRepoName) + .getRelative(output.getRepositoryRelativePath()) + .getParentDirectory(); + solibParents.add( + getRelative(binaryOriginRunfilesPath, toolchainBinRunfilesPath).getPathString() + + PathFragment.SEPARATOR_CHAR); + + return solibParents.build(); + } + + private String getRunfilesRepoName(RepositoryName repo) { + if (repo.isMain()) { + return workspaceName; + } + return repo.getName(); + } + + /** + * Returns the relative {@link PathFragment} from "from" to "to". + * + *

Example 1: + * getRelative({@link PathFragment}.create("foo"), {@link PathFragment}.create("foo/bar/wiz")) + * returns "bar/wiz". + * + *

Example 2: + * getRelative({@link PathFragment}.create("foo/bar/wiz"), + * {@link PathFragment}.create("foo/wiz")) + * returns "../../wiz". + * + *

The following requirements / assumptions are made: 1) paths must be both relative; 2) they + * are assumed to be relative to the same location; 3) when the {@code from} path starts with + * {@code ..} prefixes, the prefix length must not exceed {@code ..} prefixes of the {@code to} + * path. + */ + static PathFragment getRelative(PathFragment from, PathFragment to) { + if (from.isAbsolute() || to.isAbsolute()) { + throw new IllegalArgumentException("Paths must be both relative."); + } + + final ImmutableList fromSegments = from.splitToListOfSegments(); + final ImmutableList toSegments = to.splitToListOfSegments(); + final int fromSegCount = fromSegments.size(); + final int toSegCount = toSegments.size(); + + int commonSegCount = 0; + while (commonSegCount < fromSegCount + && commonSegCount < toSegCount + && OS.equals(fromSegments.get(commonSegCount), toSegments.get(commonSegCount))) { + commonSegCount++; + } + + if (commonSegCount < fromSegCount && fromSegments.get(commonSegCount).equals("..")) { + throw new IllegalArgumentException( + "Unable to compute relative path from \"" + + from.getPathString() + + "\" to \"" + + to.getPathString() + + "\": too many leading \"..\" segments in from path."); + } + PathFragment relativePath = PathFragment.create( + PATH_JOINER.join(Collections.nCopies(fromSegCount - commonSegCount, ".."))); + if (commonSegCount < toSegCount) { + relativePath = relativePath.getRelative(to.subFragment(commonSegCount, toSegCount)); + } + return relativePath; + } + /** * When linking a shared library fully or mostly static then we need to link in *all* dependent * files, not just what the shared library needs for its own code. This is done by wrapping all @@ -530,7 +680,7 @@ private void addStaticInputLinkOptions( LinkerInputs.simpleLinkerInput( member, ArtifactCategory.OBJECT_FILE, - /* disableWholeArchive = */ false, + /* disableWholeArchive= */ false, member.getRootRelativePathString())); } ImmutableList nonLtoArchiveMembers = nonLtoArchiveMembersBuilder.build(); diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD b/src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD index 22d443fd9b147e..a860d97fb3a717 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD @@ -727,3 +727,20 @@ java_test( "//third_party:truth", ], ) + +java_test( + name = "LibrariesToLinkCollectorTest", + srcs = ["LibrariesToLinkCollectorTest.java"], + deps = [ + "//src/main/java/com/google/devtools/build/lib/actions:artifacts", + "//src/main/java/com/google/devtools/build/lib/analysis:configured_target", + "//src/main/java/com/google/devtools/build/lib/rules/cpp", + "//src/main/java/com/google/devtools/build/lib/vfs", + "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", + "//src/test/java/com/google/devtools/build/lib/analysis/util", + "//src/test/java/com/google/devtools/build/lib/packages:testutil", + "//src/test/java/com/google/devtools/build/lib/testutil:TestConstants", + "//third_party:junit4", + "//third_party:truth", + ], +) diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollectorTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollectorTest.java new file mode 100644 index 00000000000000..56bd2f84d77dd3 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollectorTest.java @@ -0,0 +1,241 @@ +// Copyright 2022 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.rules.cpp; + +import static com.google.common.truth.Truth.assertThat; +import static com.google.devtools.build.lib.rules.cpp.LibrariesToLinkCollector.getRelative; +import static com.google.devtools.build.lib.vfs.PathFragment.create; +import static org.junit.Assert.assertThrows; + +import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.analysis.ConfiguredTarget; +import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; +import com.google.devtools.build.lib.packages.util.Crosstool.CcToolchainConfig; +import com.google.devtools.build.lib.packages.util.ResourceLoader; +import com.google.devtools.build.lib.testutil.TestConstants; +import com.google.devtools.build.lib.vfs.FileSystemUtils; +import java.util.List; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public final class LibrariesToLinkCollectorTest extends BuildViewTestCase { + + @Test + public void getRalitive_returnsRelativePaths() { + assertThat(getRelative(create("foo"), create("foo/bar/baz"))).isEqualTo(create("bar/baz")); + assertThat(getRelative(create("foo/bar"), create("foo/bar/baz"))).isEqualTo(create("baz")); + assertThat(getRelative(create(""), create("foo"))).isEqualTo(create("foo")); + assertThat(getRelative(create(""), create("foo/bar"))).isEqualTo(create("foo/bar")); + assertThat(getRelative(create("foo/bar"), create("foo/bar")).getPathString()).isEmpty(); + + assertThat(getRelative(create("foo/bar/baz"), create("foo"))).isEqualTo(create("../..")); + assertThat(getRelative(create("foo/bar/baz"), create("foo/bar"))).isEqualTo(create("..")); + assertThat(getRelative(create("foo"), create(""))).isEqualTo(create("..")); + assertThat(getRelative(create("foo/bar"), create(""))).isEqualTo(create("../..")); + assertThat(getRelative(create("foo/baz"), create("foo/bar"))).isEqualTo(create("../bar")); + assertThat(getRelative(create("bar"), create("foo"))).isEqualTo(create("../foo")); + + assertThat(getRelative(create("foo"), create("../foo"))).isEqualTo(create("../../foo")); + assertThat(getRelative(create(".."), create("../../foo"))).isEqualTo(create("../foo")); + assertThat(getRelative(create("../bar"), create("../../foo"))).isEqualTo(create("../../foo")); + } + + @Test + public void getRelative_throwsOnInvalidCases() { + assertThrows(IllegalArgumentException.class, () -> getRelative(create("/bar"), create("/foo"))); + assertThrows(IllegalArgumentException.class, () -> getRelative(create(".."), create(""))); + assertThrows( + IllegalArgumentException.class, () -> getRelative(create("../../bar"), create("../foo"))); + } + + /* TODO: Add an integration test (maybe in cc_integration_test.sh) when a modular toolchain config + is available.*/ + @Test + public void dynamicLink_siblingLayout_externalBinary_rpath() throws Exception { + FileSystemUtils.appendIsoLatin1( + scratch.resolve("WORKSPACE"), "local_repository(name = 'src', path = 'src')"); + invalidatePackages(); + + scratch.file("src/WORKSPACE"); + scratch.file( + "src/test/BUILD", + "cc_binary(", + " name = 'foo',", + " srcs = ['some-dir/bar.so', 'some-other-dir/qux.so'],", + ")"); + scratch.file("src/test/some-dir/bar.so"); + scratch.file("src/test/some-other-dir/qux.so"); + + analysisMock.ccSupport().setupCcToolchainConfig(mockToolsConfig, CcToolchainConfig.builder()); + mockToolsConfig.create( + "toolchain/cc_toolchain_config.bzl", + ResourceLoader.readFromResources( + "com/google/devtools/build/lib/analysis/mock/cc_toolchain_config.bzl")); + scratch.file( + "toolchain/BUILD", + "load(':cc_toolchain_config.bzl', 'cc_toolchain_config')", + "filegroup(", + " name = 'empty',", + ")", + "filegroup(", + " name = 'static_runtime',", + " srcs = ['librt.a'],", + ")", + "filegroup(", + " name = 'dynamic_runtime',", + " srcs = ['librt.so'],", + ")", + "filegroup(", + " name = 'files',", + " srcs = ['librt.a', 'librt.so'],", + ")", + "cc_toolchain(", + " name = 'c_toolchain',", + " toolchain_identifier = 'toolchain-identifier-k8',", + " toolchain_config = ':k8-compiler_config',", + " all_files = ':files',", + " ar_files = ':empty',", + " as_files = ':empty',", + " compiler_files = ':empty',", + " dwp_files = ':empty',", + " linker_files = ':empty',", + " strip_files = ':empty',", + " objcopy_files = ':empty',", + " dynamic_runtime_lib = ':dynamic_runtime',", + " static_runtime_lib = ':static_runtime',", + ")", + CcToolchainConfig.builder() + .withFeatures( + CppRuleClasses.STATIC_LINK_CPP_RUNTIMES, CppRuleClasses.SUPPORTS_DYNAMIC_LINKER, + "runtime_library_search_directories") + .build() + .getCcToolchainConfigRule(), + "toolchain(", + " name = 'toolchain',", + " toolchain_type = '" + TestConstants.TOOLS_REPOSITORY + "//tools/cpp:toolchain_type',", + " toolchain = ':c_toolchain',", + ")"); + scratch.file("toolchain/librt.a"); + scratch.file("toolchain/librt.so"); + analysisMock.ccSupport().setupCcToolchainConfig(mockToolsConfig, CcToolchainConfig.builder()); + + setBuildLanguageOptions("--experimental_sibling_repository_layout"); + useConfiguration("--extra_toolchains=//toolchain:toolchain", "--dynamic_mode=fully", + "--incompatible_enable_cc_toolchain_resolution"); + + ConfiguredTarget target = getConfiguredTarget("@src//test:foo"); + assertThat(target).isNotNull(); + Artifact binary = getExecutable(target); + CppLinkAction linkAction = (CppLinkAction) getGeneratingAction(binary); + assertThat(linkAction).isNotNull(); + + List linkArgs = linkAction.getArguments(); + assertThat(linkArgs).contains( + "--runtime_library=../../../../k8-fastbuild/bin/_solib__toolchain_Cc_Utoolchain/"); + assertThat(linkArgs).contains( + "--runtime_library=foo.runfiles/__main__/_solib__toolchain_Cc_Utoolchain/"); + assertThat(linkArgs).contains( + "--runtime_library=../../__main__/_solib__toolchain_Cc_Utoolchain/"); + } + + @Test + public void dynamicLink_siblingLayout_externalToolchain_rpath() throws Exception { + FileSystemUtils.appendIsoLatin1( + scratch.resolve("WORKSPACE"), "local_repository(name = 'toolchain', path = 'toolchain')"); + invalidatePackages(); + + scratch.file( + "src/test/BUILD", + "cc_binary(", + " name = 'foo',", + " srcs = ['some-dir/bar.so', 'some-other-dir/qux.so'],", + ")"); + scratch.file("src/test/some-dir/bar.so"); + scratch.file("src/test/some-other-dir/qux.so"); + + analysisMock.ccSupport().setupCcToolchainConfig(mockToolsConfig, CcToolchainConfig.builder()); + scratch.file("toolchain/WORKSPACE"); + mockToolsConfig.create( + "toolchain/cc_toolchain_config.bzl", + ResourceLoader.readFromResources( + "com/google/devtools/build/lib/analysis/mock/cc_toolchain_config.bzl")); + scratch.file( + "toolchain/BUILD", + "load(':cc_toolchain_config.bzl', 'cc_toolchain_config')", + "filegroup(", + " name = 'empty',", + ")", + "filegroup(", + " name = 'static_runtime',", + " srcs = ['librt.a'],", + ")", + "filegroup(", + " name = 'dynamic_runtime',", + " srcs = ['librt.so'],", + ")", + "filegroup(", + " name = 'files',", + " srcs = ['librt.a', 'librt.so'],", + ")", + "cc_toolchain(", + " name = 'c_toolchain',", + " toolchain_identifier = 'toolchain-identifier-k8',", + " toolchain_config = ':k8-compiler_config',", + " all_files = ':files',", + " ar_files = ':empty',", + " as_files = ':empty',", + " compiler_files = ':empty',", + " dwp_files = ':empty',", + " linker_files = ':empty',", + " strip_files = ':empty',", + " objcopy_files = ':empty',", + " dynamic_runtime_lib = ':dynamic_runtime',", + " static_runtime_lib = ':static_runtime',", + ")", + CcToolchainConfig.builder() + .withFeatures( + CppRuleClasses.STATIC_LINK_CPP_RUNTIMES, CppRuleClasses.SUPPORTS_DYNAMIC_LINKER, + "runtime_library_search_directories") + .build() + .getCcToolchainConfigRule(), + "toolchain(", + " name = 'toolchain',", + " toolchain_type = '" + TestConstants.TOOLS_REPOSITORY + "//tools/cpp:toolchain_type',", + " toolchain = ':c_toolchain',", + ")"); + scratch.file("toolchain/librt.a"); + scratch.file("toolchain/librt.so"); + analysisMock.ccSupport().setupCcToolchainConfig(mockToolsConfig, CcToolchainConfig.builder()); + + setBuildLanguageOptions("--experimental_sibling_repository_layout"); + useConfiguration("--extra_toolchains=@toolchain//:toolchain", "--dynamic_mode=fully", + "--incompatible_enable_cc_toolchain_resolution"); + + ConfiguredTarget target = getConfiguredTarget("//src/test:foo"); + assertThat(target).isNotNull(); + Artifact binary = getExecutable(target); + CppLinkAction linkAction = (CppLinkAction) getGeneratingAction(binary); + assertThat(linkAction).isNotNull(); + + List linkArgs = linkAction.getArguments(); + assertThat(linkArgs).contains( + "--runtime_library=../../../../toolchain/k8-fastbuild/bin/_solib___Cc_Utoolchain/"); + assertThat(linkArgs).contains( + "--runtime_library=foo.runfiles/toolchain/_solib___Cc_Utoolchain/"); + assertThat(linkArgs).contains( + "--runtime_library=../../../toolchain/_solib___Cc_Utoolchain/"); + } +}