diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java index c6ab67616171ff..25577e927b4c50 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java @@ -34,7 +34,6 @@ import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.CommandAction; -import com.google.devtools.build.lib.actions.CommandLine; import com.google.devtools.build.lib.actions.CommandLineExpansionException; import com.google.devtools.build.lib.actions.CommandLines.CommandLineAndParamFileInfo; import com.google.devtools.build.lib.actions.ExecException; @@ -274,11 +273,8 @@ public Sequence getStarlarkArgs() { getInputs().toList().stream() .filter(Artifact::isDirectory) .collect(ImmutableSet.toImmutableSet()); - - CommandLine commandLine = linkCommandLine.getCommandLineForStarlark(); - CommandLineAndParamFileInfo commandLineAndParamFileInfo = - new CommandLineAndParamFileInfo(commandLine, /* paramFileInfo= */ null); + new CommandLineAndParamFileInfo(linkCommandLine, /* paramFileInfo= */ null); Args args = Args.forRegisteredAction(commandLineAndParamFileInfo, directoryInputs); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java index 00d8b9d8002d89..76c33697072c86 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java @@ -14,6 +14,9 @@ package com.google.devtools.build.lib.rules.cpp; +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.devtools.build.lib.rules.cpp.LinkBuildVariables.LINKER_PARAM_FILE; + import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; @@ -26,7 +29,6 @@ import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.ExpansionException; import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration; import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType; -import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.errorprone.annotations.CanIgnoreReturnValue; import java.util.ArrayList; @@ -84,7 +86,15 @@ public String getActionName() { /** Returns the path to the linker. */ public String getLinkerPathString() { - return featureConfiguration.getToolPathForAction(linkTargetType.getActionName()); + if (forcedToolPath != null) { + return forcedToolPath; + } else { + Preconditions.checkArgument( + featureConfiguration.actionIsConfigured(actionName), + "Expected action_config for '%s' to be configured", + actionName); + return featureConfiguration.getToolPathForAction(linkTargetType.getActionName()); + } } /** @@ -117,195 +127,84 @@ public CcToolchainVariables getBuildVariables() { return this.variables; } - /** - * Splits the link command-line into a part to be written to a parameter file, and the remaining - * actual command line to be executed (which references the parameter file). Should only be used - * if getParamFile() is not null. - */ - @VisibleForTesting - final Pair, List> splitCommandline() throws CommandLineExpansionException { - return splitCommandline(paramFile, getRawLinkArgv(null)); - } - - @VisibleForTesting - final Pair, List> splitCommandline(@Nullable ArtifactExpander expander) - throws CommandLineExpansionException { - return splitCommandline(paramFile, getRawLinkArgv(expander)); - } - - private static Pair, List> splitCommandline( - Artifact paramFile, List args) { + /** Returns just the .params file portion of the command-line as a {@link CommandLine}. */ + CommandLine paramCmdLine() { Preconditions.checkNotNull(paramFile); - - // Gcc and Ar link commands tend to generate humongous commandlines for some targets, which may - // not fit on some remote execution machines. To work around this we will employ the help of - // a parameter file and pass any linker options through it. - List paramFileArgs = new ArrayList<>(); - List commandlineArgs = new ArrayList<>(); - extractArguments(args, commandlineArgs, paramFileArgs); - return Pair.of(commandlineArgs, paramFileArgs); - } - - public CommandLine getCommandLineForStarlark() { return new CommandLine() { @Override public Iterable arguments() throws CommandLineExpansionException { - return arguments(/* artifactExpander= */ null, PathMapper.NOOP); + return getParamCommandLine(null); } @Override - public ImmutableList arguments( - ArtifactExpander artifactExpander, PathMapper pathMapper) + public List arguments(ArtifactExpander expander, PathMapper pathMapper) throws CommandLineExpansionException { - if (paramFile == null) { - return ImmutableList.copyOf(getRawLinkArgv(artifactExpander)); - } else { - return ImmutableList.builder() - .add(getLinkerPathString()) - .addAll(splitCommandline(artifactExpander).getSecond()) - .build(); - } + return getParamCommandLine(expander); } }; } - /** - * A {@link CommandLine} implementation that returns the command line args pertaining to the - * .params file. - */ - private static class ParamFileCommandLine extends CommandLine { - private final Artifact paramsFile; - private final LinkTargetType linkTargetType; - private final String forcedToolPath; - private final FeatureConfiguration featureConfiguration; - private final String actionName; - private final CcToolchainVariables variables; - - ParamFileCommandLine( - Artifact paramsFile, - LinkTargetType linkTargetType, - String forcedToolPath, - FeatureConfiguration featureConfiguration, - String actionName, - CcToolchainVariables variables) { - this.paramsFile = paramsFile; - this.linkTargetType = linkTargetType; - this.forcedToolPath = forcedToolPath; - this.featureConfiguration = featureConfiguration; - this.actionName = actionName; - this.variables = variables; - } - - @Override - public Iterable arguments() throws CommandLineExpansionException { - List argv = - getRawLinkArgv( - null, forcedToolPath, featureConfiguration, actionName, linkTargetType, variables); - return splitCommandline(paramsFile, argv).getSecond(); - } - - @Override - public Iterable arguments(ArtifactExpander expander, PathMapper pathMapper) - throws CommandLineExpansionException { - List argv = - getRawLinkArgv( - expander, - forcedToolPath, - featureConfiguration, - actionName, - linkTargetType, - variables); - return splitCommandline(paramsFile, argv).getSecond(); - } - } - - /** Returns just the .params file portion of the command-line as a {@link CommandLine}. */ - CommandLine paramCmdLine() { - Preconditions.checkNotNull(paramFile); - return new ParamFileCommandLine( - paramFile, linkTargetType, forcedToolPath, featureConfiguration, actionName, variables); - } - - private static void extractArguments( - List args, List commandlineArgs, List paramFileArgs) { - commandlineArgs.add(args.get(0)); // ar command, must not be moved! - int argsSize = args.size(); - for (int i = 1; i < argsSize; i++) { - String arg = args.get(i); - if (isLikelyParamFile(arg)) { - commandlineArgs.add(arg); // params file, keep it in the command line + public List getCommandLine(@Nullable ArtifactExpander expander) + throws CommandLineExpansionException { + // Try to shorten the command line by use of a parameter file. + // This makes the output with --subcommands (et al) more readable. + List argv = new ArrayList<>(); + argv.add(getLinkerPathString()); + try { + if (paramFile != null) { + // Retrieve only reference to linker_param_file from the command line. + String linkerParamFile = + variables + .getVariable(LINKER_PARAM_FILE.getVariableName()) + .getStringValue(LINKER_PARAM_FILE.getVariableName()); + argv.addAll( + featureConfiguration.getCommandLine(actionName, variables, expander).stream() + .filter(s -> s.contains(linkerParamFile)) + .collect(toImmutableList())); } else { - paramFileArgs.add(arg); // the rest goes to the params file + argv.addAll(featureConfiguration.getCommandLine(actionName, variables, expander)); } + } catch (ExpansionException e) { + throw new CommandLineExpansionException(e.getMessage()); } + return argv; } - private static boolean isLikelyParamFile(String arg) { - return arg.startsWith("@") - && !arg.startsWith("@rpath") - && !arg.startsWith("@loader_path") - && !arg.startsWith("@executable_path"); - } - - /** - * Returns a raw link command for the given link invocation, including both command and arguments - * (argv). - * - * @param expander ArtifactExpander for expanding TreeArtifacts. - * @return raw link command line. - */ - private List getRawLinkArgv(@Nullable ArtifactExpander expander) - throws CommandLineExpansionException { - return getRawLinkArgv( - expander, forcedToolPath, featureConfiguration, actionName, linkTargetType, variables); - } - - private static List getRawLinkArgv( - @Nullable ArtifactExpander expander, - String forcedToolPath, - FeatureConfiguration featureConfiguration, - String actionName, - LinkTargetType linkTargetType, - CcToolchainVariables variables) + public List getParamCommandLine(@Nullable ArtifactExpander expander) throws CommandLineExpansionException { List argv = new ArrayList<>(); - if (forcedToolPath != null) { - argv.add(forcedToolPath); - } else { - Preconditions.checkArgument( - featureConfiguration.actionIsConfigured(actionName), - String.format("Expected action_config for '%s' to be configured", actionName)); - argv.add(featureConfiguration.getToolPathForAction(linkTargetType.getActionName())); - } try { - argv.addAll(featureConfiguration.getCommandLine(actionName, variables, expander)); + if (variables.isAvailable(LINKER_PARAM_FILE.getVariableName())) { + // Filter out linker_param_file + String linkerParamFile = + variables + .getVariable(LINKER_PARAM_FILE.getVariableName()) + .getStringValue(LINKER_PARAM_FILE.getVariableName()); + argv.addAll( + featureConfiguration.getCommandLine(actionName, variables, expander).stream() + .filter(s -> !s.contains(linkerParamFile)) + .collect(toImmutableList())); + } else { + argv.addAll(featureConfiguration.getCommandLine(actionName, variables, expander)); + } } catch (ExpansionException e) { throw new CommandLineExpansionException(e.getMessage()); } return argv; } - List getCommandLine(@Nullable ArtifactExpander expander) - throws CommandLineExpansionException { - // Try to shorten the command line by use of a parameter file. - // This makes the output with --subcommands (et al) more readable. - if (paramFile != null) { - Pair, List> split = splitCommandline(expander); - return split.first; - } else { - return getRawLinkArgv(expander); - } - } - @Override public List arguments() throws CommandLineExpansionException { - return getRawLinkArgv(null); + return arguments(null, null); } @Override public List arguments(ArtifactExpander artifactExpander, PathMapper pathMapper) throws CommandLineExpansionException { - return getRawLinkArgv(artifactExpander); + return ImmutableList.builder() + .add(getLinkerPathString()) + .addAll(getParamCommandLine(artifactExpander)) + .build(); } /** A builder for a {@link LinkCommandLine}. */ diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLineTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLineTest.java index aac913a858ce5d..f79135693e5237 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLineTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLineTest.java @@ -34,7 +34,6 @@ import com.google.devtools.build.lib.rules.cpp.CppActionConfigs.CppPlatform; import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType; import com.google.devtools.build.lib.testutil.TestUtils; -import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; @@ -211,7 +210,7 @@ public void testLinkerParamFileForStaticLibrary() throws Exception { .setActionName(LinkTargetType.STATIC_LIBRARY.getActionName()) .setLinkTargetType(LinkTargetType.STATIC_LIBRARY) .build(); - assertThat(linkConfig.arguments()).contains("@foo/bar.param"); + assertThat(linkConfig.getCommandLine(null)).contains("@foo/bar.param"); } @Test @@ -226,7 +225,7 @@ public void testLinkerParamFileForDynamicLibrary() throws Exception { .setActionName(LinkTargetType.NODEPS_DYNAMIC_LIBRARY.getActionName()) .setLinkTargetType(LinkTargetType.NODEPS_DYNAMIC_LIBRARY) .build(); - assertThat(linkConfig.arguments()).contains("@foo/bar.param"); + assertThat(linkConfig.getCommandLine(null)).contains("@foo/bar.param"); } private List basicArgv(LinkTargetType targetType) throws Exception { @@ -288,9 +287,12 @@ public void testSplitStaticLinkCommand() throws Exception { .forceToolPath("foo/bar/ar") .setParamFile(paramFile) .build(); - Pair, List> result = linkConfig.splitCommandline(); - assertThat(result.first).isEqualTo(Arrays.asList("foo/bar/ar", "@some/file.params")); - assertThat(result.second).isEqualTo(Arrays.asList("rcsD", "a/FakeOutput")); + assertThat(linkConfig.getCommandLine(null)) + .containsExactly("foo/bar/ar", "@some/file.params") + .inOrder(); + assertThat(linkConfig.getParamCommandLine(null)) + .containsExactly("rcsD", "a/FakeOutput") + .inOrder(); } @Test @@ -311,9 +313,12 @@ public void testSplitDynamicLinkCommand() throws Exception { .forceToolPath("foo/bar/linker") .setParamFile(paramFile) .build(); - Pair, List> result = linkConfig.splitCommandline(); - assertThat(result.first).containsExactly("foo/bar/linker", "@some/file.params").inOrder(); - assertThat(result.second).containsExactly("-shared", "-o", "a/FakeOutput", "").inOrder(); + assertThat(linkConfig.getCommandLine(null)) + .containsExactly("foo/bar/linker", "@some/file.params") + .inOrder(); + assertThat(linkConfig.getParamCommandLine(null)) + .containsExactly("-shared", "-o", "a/FakeOutput", "") + .inOrder(); } @Test @@ -354,10 +359,13 @@ public void testSplitAlwaysLinkLinkCommand() throws Exception { .forceToolPath("foo/bar/ar") .setParamFile(paramFile) .build(); - Pair, List> result = linkConfig.splitCommandline(); - assertThat(result.first).isEqualTo(Arrays.asList("foo/bar/ar", "@some/file.params")); - assertThat(result.second).isEqualTo(Arrays.asList("rcsD", "a/FakeOutput", "foo.o", "bar.o")); + assertThat(linkConfig.getCommandLine(null)) + .containsExactly("foo/bar/ar", "@some/file.params") + .inOrder(); + assertThat(linkConfig.getParamCommandLine(null)) + .containsExactly("rcsD", "a/FakeOutput", "foo.o", "bar.o") + .inOrder(); } private SpecialArtifact createTreeArtifact(String name) {