Change CppLinkAction to SpawnAction
Change LinkCommandLine to return CommandLines, method getCommandLines (similarly to Starlark actions).
Remove param file writing from CppLinkActionBuilder. CommandLines create the file automatically.
Pass the command lines into SpawnAction.
Remove unnecessary fields from fingerprint computation (most of them should already be removed in previous changes). toolchainEnv needs to stay, because it's used in getEffectiveEnvironment.
Most of the remaining public methods in LinkCommandLine and CppLinkAction should be removed. ATM they are still used in tests.
RELNOTES[INC]: CppLinkAction returns 2 args to aspects that have correct quoting set (before it was always 1 args object defaulting to bash escaping)
PiperOrigin-RevId: 598606687
Change-Id: Ib428b10c336c2f211fd5a4599d6d6ec57fb3f668
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 25577e9..55f759c 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
@@ -14,67 +14,46 @@
package com.google.devtools.build.lib.rules.cpp;
-import static com.google.devtools.build.lib.actions.ActionAnalysisMetadata.mergeMaps;
-
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableCollection;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Maps;
-import com.google.common.flogger.GoogleLogger;
-import com.google.devtools.build.lib.actions.AbstractAction;
import com.google.devtools.build.lib.actions.ActionEnvironment;
-import com.google.devtools.build.lib.actions.ActionExecutionContext;
-import com.google.devtools.build.lib.actions.ActionExecutionException;
import com.google.devtools.build.lib.actions.ActionKeyContext;
import com.google.devtools.build.lib.actions.ActionOwner;
-import com.google.devtools.build.lib.actions.ActionResult;
import com.google.devtools.build.lib.actions.Artifact;
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.CommandLineExpansionException;
-import com.google.devtools.build.lib.actions.CommandLines.CommandLineAndParamFileInfo;
+import com.google.devtools.build.lib.actions.EmptyRunfilesSupplier;
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.ExecutionRequirements;
import com.google.devtools.build.lib.actions.ResourceSet;
import com.google.devtools.build.lib.actions.ResourceSetOrBuilder;
-import com.google.devtools.build.lib.actions.SimpleSpawn;
-import com.google.devtools.build.lib.actions.Spawn;
-import com.google.devtools.build.lib.actions.SpawnResult;
import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext;
+import com.google.devtools.build.lib.analysis.actions.SpawnAction;
import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
-import com.google.devtools.build.lib.analysis.starlark.Args;
+import com.google.devtools.build.lib.analysis.config.CoreOptions.OutputPathsMode;
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.collect.nestedset.Order;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible;
-import com.google.devtools.build.lib.exec.SpawnStrategyResolver;
+import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException;
import com.google.devtools.build.lib.rules.cpp.CcLinkingContext.Linkstamp;
import com.google.devtools.build.lib.rules.cpp.LinkerInputs.LibraryToLink;
-import com.google.devtools.build.lib.server.FailureDetails.CppLink;
-import com.google.devtools.build.lib.server.FailureDetails.CppLink.Code;
-import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
-import com.google.devtools.build.lib.starlarkbuildapi.CommandLineArgsApi;
-import com.google.devtools.build.lib.util.DetailedExitCode;
import com.google.devtools.build.lib.util.Fingerprint;
import com.google.devtools.build.lib.util.OS;
-import com.google.devtools.build.lib.util.ShellEscaper;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.LinkedHashMap;
-import java.util.List;
import java.util.Map;
import javax.annotation.Nullable;
-import net.starlark.java.eval.EvalException;
-import net.starlark.java.eval.Sequence;
-import net.starlark.java.eval.StarlarkList;
/** Action that represents a linking step. */
@ThreadCompatible
-public final class CppLinkAction extends AbstractAction implements CommandAction {
-
- private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
-
+public final class CppLinkAction extends SpawnAction {
/**
* An abstraction for creating intermediate and output artifacts for C++ linking.
*
@@ -161,20 +140,11 @@
private static final String LINK_GUID = "58ec78bd-1176-4e36-8143-439f656b181d";
- @Nullable private final String mnemonic;
private final LibraryToLink outputLibrary;
- private final Artifact linkOutput;
private final LibraryToLink interfaceOutputLibrary;
private final ImmutableMap<String, String> toolchainEnv;
- private final ImmutableMap<String, String> executionRequirements;
private final ImmutableMap<Linkstamp, Artifact> linkstamps;
-
private final LinkCommandLine linkCommandLine;
- private final ActionEnvironment env;
-
- private final boolean isLtoIndexing;
-
- private final String ldExecutable;
/**
* Use {@link CppLinkActionBuilder} to create instances of this class. Also see there for the
@@ -189,55 +159,44 @@
NestedSet<Artifact> inputs,
ImmutableSet<Artifact> outputs,
LibraryToLink outputLibrary,
- Artifact linkOutput,
LibraryToLink interfaceOutputLibrary,
boolean isLtoIndexing,
ImmutableMap<Linkstamp, Artifact> linkstamps,
LinkCommandLine linkCommandLine,
ActionEnvironment env,
ImmutableMap<String, String> toolchainEnv,
- ImmutableMap<String, String> executionRequirements,
- String ldExecutable) {
- super(owner, inputs, outputs);
- this.mnemonic = getMnemonic(mnemonic, isLtoIndexing);
+ ImmutableMap<String, String> executionRequirements)
+ throws RuleErrorException {
+ super(
+ owner,
+ /* tools= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
+ inputs,
+ outputs,
+ /* resourceSetOrBuilder= */ new LinkResourceSetBuilder(),
+ /* commandLines= */ linkCommandLine.getCommandLines(),
+ /* env= */ env,
+ /* executionInfo= */ executionRequirements,
+ /* progressMessage= */ (isLtoIndexing ? "LTO indexing %{output}" : "Linking %{output}"),
+ /* runfilesSupplier= */ EmptyRunfilesSupplier.INSTANCE,
+ /* mnemonic= */ getMnemonic(mnemonic, isLtoIndexing),
+ /* outputPathsMode= */ OutputPathsMode.OFF);
+
this.outputLibrary = outputLibrary;
- this.linkOutput = linkOutput;
this.interfaceOutputLibrary = interfaceOutputLibrary;
- this.isLtoIndexing = isLtoIndexing;
this.linkstamps = linkstamps;
this.linkCommandLine = linkCommandLine;
- this.env = env;
this.toolchainEnv = toolchainEnv;
- this.executionRequirements = executionRequirements;
- this.ldExecutable = ldExecutable;
- }
-
- @Override
- @VisibleForTesting
- public NestedSet<Artifact> getPossibleInputsForTesting() {
- return getInputs();
- }
-
- @Override
- public ActionEnvironment getEnvironment() {
- return env;
- }
-
- @Override
- @VisibleForTesting
- public ImmutableMap<String, String> getIncompleteEnvironmentForTesting() {
- return getEffectiveEnvironment(ImmutableMap.of());
}
@Override
public ImmutableMap<String, String> getEffectiveEnvironment(Map<String, String> clientEnv) {
LinkedHashMap<String, String> result =
- Maps.newLinkedHashMapWithExpectedSize(env.estimatedSize());
- env.resolve(result, clientEnv);
+ Maps.newLinkedHashMapWithExpectedSize(getEnvironment().estimatedSize());
+ getEnvironment().resolve(result, clientEnv);
result.putAll(toolchainEnv);
- if (!executionRequirements.containsKey(ExecutionRequirements.REQUIRES_DARWIN)) {
+ if (!getExecutionInfo().containsKey(ExecutionRequirements.REQUIRES_DARWIN)) {
// This prevents gcc from writing the unpredictable (and often irrelevant)
// value of getcwd() into the debug info.
result.put("PWD", "/proc/self/cwd");
@@ -262,170 +221,33 @@
return interfaceOutputLibrary;
}
- @Override
- public ImmutableMap<String, String> getExecutionInfo() {
- return mergeMaps(super.getExecutionInfo(), executionRequirements);
- }
-
- @Override
- public Sequence<CommandLineArgsApi> getStarlarkArgs() {
- ImmutableSet<Artifact> directoryInputs =
- getInputs().toList().stream()
- .filter(Artifact::isDirectory)
- .collect(ImmutableSet.toImmutableSet());
- CommandLineAndParamFileInfo commandLineAndParamFileInfo =
- new CommandLineAndParamFileInfo(linkCommandLine, /* paramFileInfo= */ null);
-
- Args args = Args.forRegisteredAction(commandLineAndParamFileInfo, directoryInputs);
-
- return StarlarkList.immutableCopyOf(ImmutableList.of(args));
- }
-
- @Override
- public List<String> getArguments() throws CommandLineExpansionException {
- return linkCommandLine.arguments();
- }
-
- /**
- * Returns the command line specification for this link, included any required linkstamp
- * compilation steps. The command line may refer to a .params file.
- *
- * @param expander ArtifactExpander for expanding TreeArtifacts.
- * @return a finalized command line suitable for execution
- */
- public List<String> getCommandLine(@Nullable ArtifactExpander expander)
- throws CommandLineExpansionException {
- return linkCommandLine.getCommandLine(expander);
- }
-
/**
* Returns a (possibly empty) list of linkstamp object files.
*
* <p>This is used to embed various values from the build system into binaries to identify their
* provenance.
*/
+ @VisibleForTesting
ImmutableList<Artifact> getLinkstampObjects() {
return linkstamps.keySet().stream()
.map(CcLinkingContext.Linkstamp::getArtifact)
.collect(ImmutableList.toImmutableList());
}
+ @VisibleForTesting
ImmutableCollection<Artifact> getLinkstampObjectFileInputs() {
return linkstamps.values();
}
@Override
- public ActionResult execute(ActionExecutionContext actionExecutionContext)
- throws ActionExecutionException, InterruptedException {
- ResourceSetOrBuilder localResources = new LinkResourceSetBuilder();
-
- try {
- int inputsCount = getInputs().memoizedFlattenAndGetSize();
- ResourceSet resourceSet = localResources.buildResourceSet(OS.getCurrent(), inputsCount);
- Spawn spawn = createSpawn(actionExecutionContext, resourceSet);
- ImmutableList<SpawnResult> spawnResults =
- actionExecutionContext
- .getContext(SpawnStrategyResolver.class)
- .exec(spawn, actionExecutionContext);
-
- // As per the documentation of SpawnStrategy.exec(), the first entry is always the result of
- // the successful execution
- SpawnResult result = spawnResults.get(0);
- Long consumedMemoryInKb = result.getMemoryInKb();
- if (consumedMemoryInKb != null) {
- logger.atFine().log(
- "Linker metrics: inputs_count=%d,inputs_mb=%.2f,estimated_mb=%.2f,consumed_mb=%.2f",
- inputsCount,
- 0d, // Because inputs sizes weren't used, we don't compute them
- resourceSet.getMemoryMb(),
- ((double) consumedMemoryInKb) / 1024);
- }
-
- return ActionResult.create(spawnResults);
- } catch (ExecException e) {
- throw ActionExecutionException.fromExecException(e, CppLinkAction.this);
- }
- }
-
- private Spawn createSpawn(
- ActionExecutionContext actionExecutionContext, ResourceSet localResources)
- throws ActionExecutionException {
- try {
- ArtifactExpander actionContextExpander = actionExecutionContext.getArtifactExpander();
- ArtifactExpander expander = actionContextExpander;
- return new SimpleSpawn(
- this,
- ImmutableList.copyOf(getCommandLine(expander)),
- getEffectiveEnvironment(actionExecutionContext.getClientEnv()),
- getExecutionInfo(),
- getInputs(),
- getOutputs(),
- localResources);
- } catch (CommandLineExpansionException e) {
- String message =
- String.format(
- "failed to generate link command for rule '%s: %s",
- getOwner().getLabel(), e.getMessage());
- DetailedExitCode code = createDetailedExitCode(message, Code.COMMAND_GENERATION_FAILURE);
- throw new ActionExecutionException(message, this, /*catastrophe=*/ false, code);
- }
- }
-
- @Override
protected void computeKey(
ActionKeyContext actionKeyContext,
- @Nullable Artifact.ArtifactExpander artifactExpander,
+ @Nullable ArtifactExpander artifactExpander,
Fingerprint fp)
- throws CommandLineExpansionException {
+ throws CommandLineExpansionException, InterruptedException {
fp.addString(LINK_GUID);
- fp.addString(ldExecutable);
- fp.addStrings(linkCommandLine.arguments());
+ super.computeKey(actionKeyContext, artifactExpander, fp);
fp.addStringMap(toolchainEnv);
- fp.addStrings(getExecutionInfo().keySet());
-
- // TODO(bazel-team): For correctness, we need to ensure the invariant that all values accessed
- // during the execution phase are also covered by the key. Above, we add the argv to the key,
- // which covers most cases. Unfortunately, the extra action method above also
- // sometimes directly accesses settings from the link configuration that may or may not affect
- // the
- // key. We either need to change the code to cover them in the key computation, or change the
- // LinkConfiguration to disallow the combinations where the value of a setting does not affect
- // the argv.
- fp.addBoolean(linkCommandLine.isNativeDeps());
- fp.addBoolean(linkCommandLine.useTestOnlyFlags());
- if (linkCommandLine.getToolchainLibrariesSolibDir() != null) {
- fp.addPath(linkCommandLine.getToolchainLibrariesSolibDir());
- }
- fp.addBoolean(isLtoIndexing);
- }
-
- @Override
- public String describeKey() {
- StringBuilder message = new StringBuilder();
- message.append(getProgressMessage());
- message.append('\n');
- message.append(" Command: ");
- message.append(ShellEscaper.escapeString(linkCommandLine.getLinkerPathString()));
- message.append('\n');
- // Outputting one argument per line makes it easier to diff the results.
- try {
- List<String> arguments = linkCommandLine.arguments();
- for (String argument : ShellEscaper.escapeAll(arguments)) {
- message.append(" Argument: ");
- message.append(argument);
- message.append('\n');
- }
- } catch (CommandLineExpansionException e) {
- message.append(" Could not expand command line: ");
- message.append(e);
- message.append('\n');
- }
- return message.toString();
- }
-
- @Override
- public String getMnemonic() {
- return mnemonic;
}
static String getMnemonic(String mnemonic, boolean isLtoIndexing) {
@@ -435,11 +257,6 @@
return mnemonic;
}
- @Override
- protected String getRawProgressMessage() {
- return (isLtoIndexing ? "LTO indexing " : "Linking ") + linkOutput.prettyPrint();
- }
-
/** Estimates resource consumption when this action is executed locally. */
@VisibleForTesting
static class LinkResourceSetBuilder implements ResourceSetOrBuilder {
@@ -465,21 +282,4 @@
return resourceSet;
}
}
-
- @Override
- public Sequence<String> getStarlarkArgv() throws EvalException {
- try {
- return StarlarkList.immutableCopyOf(getArguments());
- } catch (CommandLineExpansionException ex) {
- throw new EvalException(ex);
- }
- }
-
- private static DetailedExitCode createDetailedExitCode(String message, Code detailedCode) {
- return DetailedExitCode.of(
- FailureDetail.newBuilder()
- .setMessage(message)
- .setCppLink(CppLink.newBuilder().setCode(detailedCode))
- .build());
- }
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
index 7599c72..b8b8094 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
@@ -21,15 +21,12 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Iterables;
-import com.google.devtools.build.lib.actions.Action;
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;
import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
import com.google.devtools.build.lib.analysis.config.CoreOptions;
import com.google.devtools.build.lib.analysis.config.PerLabelOptions;
@@ -44,7 +41,6 @@
import com.google.devtools.build.lib.rules.cpp.CcLinkingContext.Linkstamp;
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
import com.google.devtools.build.lib.rules.cpp.CcToolchainVariables.VariablesExtension;
-import com.google.devtools.build.lib.rules.cpp.CppConfiguration.Tool;
import com.google.devtools.build.lib.rules.cpp.CppLinkAction.LinkArtifactFactory;
import com.google.devtools.build.lib.rules.cpp.LibrariesToLinkCollector.CollectedLibrariesToLink;
import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType;
@@ -755,16 +751,6 @@
/* disableWholeArchive= */ true)))
.build();
- PathFragment paramRootPath =
- ParameterFile.derivePath(outputRootPath, isLtoIndexing ? "lto-index" : "2");
-
- @Nullable
- final Artifact paramFile =
- canSplitCommandLine()
- ? linkArtifactFactory.create(
- actionConstructionContext, repositoryName, configuration, paramRootPath)
- : null;
-
// Add build variables necessary to template link args into the crosstool.
CcToolchainVariables ccToolchainVariables;
Preconditions.checkArgument(actionConstructionContext instanceof RuleContext);
@@ -838,7 +824,7 @@
/* preserveName= */ false,
actionConstructionContext.getConfiguration().getMnemonic()),
linkType.equals(LinkTargetType.DYNAMIC_LIBRARY),
- paramFile != null ? paramFile.getExecPathString() : null,
+ canSplitCommandLine() ? "LINKER_PARAM_FILE_PLACEHOLDER" : null,
thinltoParamFile != null ? thinltoParamFile.getExecPathString() : null,
thinltoMergedObjectFile != null ? thinltoMergedObjectFile.getExecPathString() : null,
mustKeepDebug,
@@ -894,13 +880,11 @@
new LinkCommandLine.Builder()
.setActionName(getActionName())
.setLinkTargetType(linkType)
- .setToolchainLibrariesSolibDir(
- linkType.linkerOrArchiver() == LinkerOrArchiver.ARCHIVER
- ? null
- : toolchainLibrariesSolibDir)
- .setNativeDeps(isNativeDeps)
- .setUseTestOnlyFlags(useTestOnlyFlags)
- .setParamFile(paramFile)
+ .setSplitCommandLine(canSplitCommandLine())
+ .setParameterFileType(
+ featureConfiguration.isEnabled(CppRuleClasses.GCC_QUOTING_FOR_PARAM_FILES)
+ ? ParameterFile.ParameterFileType.GCC_QUOTED
+ : ParameterFile.ParameterFileType.UNQUOTED)
.setFeatureConfiguration(featureConfiguration);
// TODO(b/62693279): Cleanup once internal crosstools specify ifso building correctly.
@@ -938,28 +922,6 @@
if (thinltoParamFile != null && !isLtoIndexing) {
inputsBuilder.add(thinltoParamFile);
}
- if (linkCommandLine.getParamFile() != null) {
- inputsBuilder.add(linkCommandLine.getParamFile());
- // Pass along tree artifacts, so they can be properly expanded.
- NestedSet<Artifact> paramFileActionInputs =
- NestedSetBuilder.wrap(
- Order.STABLE_ORDER,
- Iterables.filter(expandedLinkerArtifacts.toList(), Artifact::isTreeArtifact));
-
- ParameterFile.ParameterFileType quoting =
- featureConfiguration.isEnabled(CppRuleClasses.GCC_QUOTING_FOR_PARAM_FILES)
- ? ParameterFile.ParameterFileType.GCC_QUOTED
- : ParameterFile.ParameterFileType.UNQUOTED;
-
- Action parameterFileWriteAction =
- new ParameterFileWriteAction(
- getOwner(),
- paramFileActionInputs,
- paramFile,
- linkCommandLine.paramCmdLine(),
- quoting);
- actionConstructionContext.registerAction(parameterFileWriteAction);
- }
ImmutableMap<String, String> toolchainEnv =
CppHelper.getEnvironmentVariables(
@@ -1025,20 +987,13 @@
inputsBuilder.build(),
actionOutputs,
outputLibrary,
- output,
interfaceOutputLibrary,
isLtoIndexing,
linkstampMap,
linkCommandLine,
configuration.getActionEnvironment(),
toolchainEnv,
- ImmutableMap.copyOf(executionInfo),
- CcToolchainProvider.getToolPathString(
- toolchain.getToolPaths(),
- Tool.LD,
- toolchain.getCcToolchainLabel(),
- toolchain.getToolchainIdentifier(),
- ruleErrorConsumer));
+ ImmutableMap.copyOf(executionInfo));
}
/** We're doing 4-phased lto build, and this is the final link action (4-th phase). */
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 76c3369..d87b962 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
@@ -20,19 +20,21 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
-import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
import com.google.devtools.build.lib.actions.CommandLine;
import com.google.devtools.build.lib.actions.CommandLineExpansionException;
+import com.google.devtools.build.lib.actions.CommandLines;
+import com.google.devtools.build.lib.actions.ParamFileInfo;
+import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType;
import com.google.devtools.build.lib.actions.PathMapper;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
+import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException;
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.vfs.PathFragment;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
-import java.util.ArrayList;
import java.util.List;
+import java.util.Optional;
import javax.annotation.Nullable;
/**
@@ -40,27 +42,23 @@
* as well as static libraries.
*/
@Immutable
-public final class LinkCommandLine extends CommandLine {
+public final class LinkCommandLine {
private final String actionName;
private final String forcedToolPath;
private final CcToolchainVariables variables;
// The feature config can be null for tests.
@Nullable private final FeatureConfiguration featureConfiguration;
private final LinkTargetType linkTargetType;
- @Nullable private final PathFragment toolchainLibrariesSolibDir;
- private final boolean nativeDeps;
- private final boolean useTestOnlyFlags;
- @Nullable private final Artifact paramFile;
+ private final boolean splitCommandLine;
+ private final ParameterFileType parameterFileType;
private LinkCommandLine(
String actionName,
String forcedToolPath,
LinkTargetType linkTargetType,
- @Nullable PathFragment toolchainLibrariesSolibDir,
- boolean nativeDeps,
- boolean useTestOnlyFlags,
- @Nullable Artifact paramFile,
+ boolean splitCommandLine,
+ ParameterFileType parameterFileType,
CcToolchainVariables variables,
@Nullable FeatureConfiguration featureConfiguration) {
@@ -69,15 +67,8 @@
this.variables = variables;
this.featureConfiguration = featureConfiguration;
this.linkTargetType = Preconditions.checkNotNull(linkTargetType);
- this.toolchainLibrariesSolibDir = toolchainLibrariesSolibDir;
- this.nativeDeps = nativeDeps;
- this.useTestOnlyFlags = useTestOnlyFlags;
- this.paramFile = paramFile;
- }
-
- @Nullable
- public Artifact getParamFile() {
- return paramFile;
+ this.splitCommandLine = splitCommandLine;
+ this.parameterFileType = parameterFileType;
}
public String getActionName() {
@@ -85,94 +76,27 @@
}
/** Returns the path to the linker. */
- public String getLinkerPathString() {
+ public String getLinkerPathString() throws RuleErrorException {
if (forcedToolPath != null) {
return forcedToolPath;
} else {
- Preconditions.checkArgument(
- featureConfiguration.actionIsConfigured(actionName),
- "Expected action_config for '%s' to be configured",
- actionName);
+ if (!featureConfiguration.actionIsConfigured(actionName)) {
+ throw new RuleErrorException(
+ String.format("Expected action_config for '%s' to be configured", actionName));
+ }
return featureConfiguration.getToolPathForAction(linkTargetType.getActionName());
}
}
- /**
- * Returns the location of the C++ runtime solib symlinks. If null, the C++ dynamic runtime
- * libraries either do not exist (because they do not come from the depot) or they are in the
- * regular solib directory.
- */
- @Nullable
- public PathFragment getToolchainLibrariesSolibDir() {
- return toolchainLibrariesSolibDir;
- }
-
- /** Returns true for libraries linked as native dependencies for other languages. */
- public boolean isNativeDeps() {
- return nativeDeps;
- }
-
- /**
- * Returns true if this link should use test-specific flags (e.g. $EXEC_ORIGIN as the root for
- * finding shared libraries or lazy binding); false by default. See bug "Please use $EXEC_ORIGIN
- * instead of $ORIGIN when linking cc_tests" for further context.
- */
- public boolean useTestOnlyFlags() {
- return useTestOnlyFlags;
- }
-
/** Returns the build variables used to template the crosstool for this linker invocation. */
@VisibleForTesting
public CcToolchainVariables getBuildVariables() {
return this.variables;
}
- /** Returns just the .params file portion of the command-line as a {@link CommandLine}. */
- CommandLine paramCmdLine() {
- Preconditions.checkNotNull(paramFile);
- return new CommandLine() {
- @Override
- public Iterable<String> arguments() throws CommandLineExpansionException {
- return getParamCommandLine(null);
- }
-
- @Override
- public List<String> arguments(ArtifactExpander expander, PathMapper pathMapper)
- throws CommandLineExpansionException {
- return getParamCommandLine(expander);
- }
- };
- }
-
- public List<String> getCommandLine(@Nullable ArtifactExpander expander)
+ public ImmutableList<String> getParamCommandLine(@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<String> 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 {
- argv.addAll(featureConfiguration.getCommandLine(actionName, variables, expander));
- }
- } catch (ExpansionException e) {
- throw new CommandLineExpansionException(e.getMessage());
- }
- return argv;
- }
-
- public List<String> getParamCommandLine(@Nullable ArtifactExpander expander)
- throws CommandLineExpansionException {
- List<String> argv = new ArrayList<>();
+ ImmutableList.Builder<String> argv = ImmutableList.builder();
try {
if (variables.isAvailable(LINKER_PARAM_FILE.getVariableName())) {
// Filter out linker_param_file
@@ -190,17 +114,61 @@
} catch (ExpansionException e) {
throw new CommandLineExpansionException(e.getMessage());
}
- return argv;
+ return argv.build();
}
- @Override
- public List<String> arguments() throws CommandLineExpansionException {
+ CommandLines getCommandLines() throws RuleErrorException {
+ CommandLines.Builder builder = CommandLines.builder();
+ builder.addSingleArgument(getLinkerPathString());
+
+ CommandLine cmdLine =
+ new CommandLine() {
+ @Override
+ public ImmutableList<String> arguments() throws CommandLineExpansionException {
+ return arguments(null, PathMapper.NOOP);
+ }
+
+ @Override
+ public ImmutableList<String> arguments(
+ ArtifactExpander artifactExpander, PathMapper pathMapper)
+ throws CommandLineExpansionException {
+ return getParamCommandLine(artifactExpander);
+ }
+ };
+ ParamFileInfo paramFileInfo = null;
+ if (splitCommandLine) {
+ try {
+ Optional<String> formatString =
+ featureConfiguration.getCommandLine(actionName, variables, null).stream()
+ .filter(s -> s.contains("LINKER_PARAM_FILE_PLACEHOLDER"))
+ .findAny();
+ if (formatString.isPresent()) {
+ paramFileInfo =
+ ParamFileInfo.builder(parameterFileType)
+ .setFlagFormatString(
+ formatString
+ .get()
+ .replace("%", "%%")
+ .replace("LINKER_PARAM_FILE_PLACEHOLDER", "%s"))
+ .setUseAlways(true)
+ .build();
+ }
+ } catch (ExpansionException e) {
+ throw new RuleErrorException(e);
+ }
+ }
+
+ builder.addCommandLine(cmdLine, paramFileInfo);
+
+ return builder.build();
+ }
+
+ public List<String> arguments() throws CommandLineExpansionException, RuleErrorException {
return arguments(null, null);
}
- @Override
public List<String> arguments(ArtifactExpander artifactExpander, PathMapper pathMapper)
- throws CommandLineExpansionException {
+ throws CommandLineExpansionException, RuleErrorException {
return ImmutableList.<String>builder()
.add(getLinkerPathString())
.addAll(getParamCommandLine(artifactExpander))
@@ -212,10 +180,8 @@
private String forcedToolPath;
@Nullable private LinkTargetType linkTargetType;
- @Nullable private PathFragment toolchainLibrariesSolibDir;
- private boolean nativeDeps;
- private boolean useTestOnlyFlags;
- @Nullable private Artifact paramFile;
+ private boolean splitCommandLine;
+ private ParameterFileType parameterFileType = ParameterFileType.UNQUOTED;
private CcToolchainVariables variables;
private FeatureConfiguration featureConfiguration;
private String actionName;
@@ -229,10 +195,8 @@
actionName,
forcedToolPath,
linkTargetType,
- toolchainLibrariesSolibDir,
- nativeDeps,
- useTestOnlyFlags,
- paramFile,
+ splitCommandLine,
+ parameterFileType,
variables,
featureConfiguration);
}
@@ -264,30 +228,15 @@
return this;
}
- /**
- * Whether the resulting library is intended to be used as a native library from another
- * programming language. This influences the rpath. The {@link #build} method throws an
- * exception if this is true for a static link (see {@link LinkTargetType#linkerOrArchiver()}}).
- */
@CanIgnoreReturnValue
- public Builder setNativeDeps(boolean nativeDeps) {
- this.nativeDeps = nativeDeps;
- return this;
- }
-
- /**
- * Sets whether to use test-specific linker flags, e.g. {@code $EXEC_ORIGIN} instead of {@code
- * $ORIGIN} in the rpath or lazy binding.
- */
- @CanIgnoreReturnValue
- public Builder setUseTestOnlyFlags(boolean useTestOnlyFlags) {
- this.useTestOnlyFlags = useTestOnlyFlags;
+ public Builder setSplitCommandLine(boolean splitCommandLine) {
+ this.splitCommandLine = splitCommandLine;
return this;
}
@CanIgnoreReturnValue
- public Builder setParamFile(Artifact paramFile) {
- this.paramFile = paramFile;
+ public Builder setParameterFileType(ParameterFileType parameterFileType) {
+ this.parameterFileType = parameterFileType;
return this;
}
@@ -298,12 +247,6 @@
}
@CanIgnoreReturnValue
- public Builder setToolchainLibrariesSolibDir(PathFragment toolchainLibrariesSolibDir) {
- this.toolchainLibrariesSolibDir = toolchainLibrariesSolibDir;
- return this;
- }
-
- @CanIgnoreReturnValue
public Builder setActionName(String actionName) {
this.actionName = actionName;
return this;
diff --git a/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/starlark_tests.bzl b/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/starlark_tests.bzl
index 79e4880..04e1596 100644
--- a/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/starlark_tests.bzl
+++ b/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/starlark_tests.bzl
@@ -47,11 +47,11 @@
if env.ctx.target_platform_has_constraint(env.ctx.attr._is_linux[platform_common.ConstraintValueInfo]):
target_action = None
for action in target.actions:
- if action.mnemonic == "FileWrite":
+ if action.mnemonic == "CppLink":
target_action = action
break
- args = target_action.content.split("\n")
+ args = target_action.argv
user_libs = [paths.basename(arg) for arg in args if arg.endswith(".o")]
env.expect.that_collection(user_libs).contains_at_least_predicates([
@@ -268,11 +268,11 @@
def _check_linking_action_lib_parameters_test_impl(env, target):
target_action = None
for action in target.actions:
- if action.mnemonic == "FileWrite":
+ if action.mnemonic == "CppLink":
target_action = action
break
- args = target_action.content.split("\n")
+ args = target_action.argv
for arg in args:
for bad_lib_entry in env.ctx.attr._libs_that_shouldnt_be_present:
env.expect.where(
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AutoExecGroupsTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AutoExecGroupsTest.java
index 93ae96b..9b04e9c 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/AutoExecGroupsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/AutoExecGroupsTest.java
@@ -25,7 +25,6 @@
import com.google.devtools.build.lib.actions.Action;
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
import com.google.devtools.build.lib.analysis.actions.LazyWritePathsFileAction;
-import com.google.devtools.build.lib.analysis.actions.ParameterFileWriteAction;
import com.google.devtools.build.lib.analysis.config.ToolchainTypeRequirement;
import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
import com.google.devtools.build.lib.analysis.platform.ToolchainInfo;
@@ -1634,52 +1633,6 @@
}
@Test
- public void ccCommonLink_fileWriteActionExecutesOnFirstPlatform() throws Exception {
- scratch.file(
- "test/defs.bzl",
- "def _use_cpp_toolchain():",
- " return [",
- " config_common.toolchain_type('"
- + TestConstants.CPP_TOOLCHAIN_TYPE
- + "', mandatory = True),",
- " ]",
- "def _impl(ctx):",
- " cc_toolchain = ctx.toolchains['" + TestConstants.CPP_TOOLCHAIN_TYPE + "'].cc",
- " feature_configuration = cc_common.configure_features(",
- " ctx = ctx,",
- " cc_toolchain = cc_toolchain,",
- " requested_features = ctx.features,",
- " unsupported_features = ctx.disabled_features,",
- " )",
- " linking_outputs = cc_common.link(",
- " name = ctx.label.name,",
- " actions = ctx.actions,",
- " feature_configuration = feature_configuration,",
- " cc_toolchain = cc_toolchain,",
- " )",
- " return []",
- "custom_rule = rule(",
- " implementation = _impl,",
- " toolchains = ['//rule:toolchain_type_2'] + _use_cpp_toolchain(),",
- " fragments = ['cpp']",
- ")");
- scratch.file(
- "test/BUILD",
- "load('//test:defs.bzl', 'custom_rule')",
- "custom_rule(name = 'custom_rule_name')");
- useConfiguration(
- "--incompatible_auto_exec_groups",
- "--toolchain_resolution_debug='//tools/cpp:toolchain_type'");
-
- ImmutableList<Action> actions =
- getActions("//test:custom_rule_name", ParameterFileWriteAction.class);
-
- assertThat(actions).hasSize(1);
- assertThat(actions.get(0).getOwner().getExecutionPlatform().label())
- .isEqualTo(Label.parseCanonical("//platforms:platform_1"));
- }
-
- @Test
public void ccCommonLink_cppLinkExecGroupNotDefined_cppLinkActionExecutesOnFirstPlatform()
throws Exception {
scratch.file(
diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidPlatformsTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidPlatformsTest.java
index fb88e45..0f15330 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidPlatformsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidPlatformsTest.java
@@ -119,8 +119,7 @@
CppLinkAction x86Link =
(CppLinkAction) getGeneratingAction(getPrerequisiteArtifacts(x86Binary, "deps").get(0));
// TODO(blaze-team): replace with the commented line below when platform-based resolution works.
- assertThat(x86Link.getLinkCommandLineForTesting().getLinkerPathString())
- .isEqualTo("/usr/bin/mock-ar");
+ assertThat(x86Link.getArguments().get(0)).isEqualTo("/usr/bin/mock-ar");
// assertThat(cppLinkAction.getLinkCommandLine().getLinkerPathString())
// .isEqualTo("android/crosstool/x86/bin/i686-linux-android-ar");
@@ -131,8 +130,7 @@
CppLinkAction armLink =
(CppLinkAction) getGeneratingAction(getPrerequisiteArtifacts(armBinary, "deps").get(0));
// TODO(blaze-team): replace with the commented line below when platform-based resolution works.
- assertThat(armLink.getLinkCommandLineForTesting().getLinkerPathString())
- .isEqualTo("/usr/bin/mock-ar");
+ assertThat(armLink.getArguments().get(0)).isEqualTo("/usr/bin/mock-ar");
// assertThat(cppLinkAction.getLinkCommandLine().getLinkerPathString())
// .isEqualTo("android/crosstool/arm/bin/arm-linux-androideabi-ar");
}
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcBinaryThinLtoObjDirTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcBinaryThinLtoObjDirTest.java
index 7878ee9..fc8ff67 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcBinaryThinLtoObjDirTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcBinaryThinLtoObjDirTest.java
@@ -298,7 +298,6 @@
.containsAtLeast(
"bin pkg/bin.lto-obj/" + rootExecPath + "/pkg/_objs/bin/binfile.pic.o",
"bin pkg/bin.lto-obj/" + rootExecPath + "/pkg/_objs/lib/libfile.pic.o",
- "bin pkg/bin-2.params",
"bin pkg/bin-lto-final.params");
LtoBackendAction backendAction =
@@ -438,7 +437,6 @@
.containsAtLeast(
"bin pkg/bin.lto-obj/" + rootExecPath + "/pkg/_objs/bin/binfile.pic.o",
"bin _solib_k8/libpkg_Sliblib.ifso",
- "bin pkg/bin-2.params",
"bin pkg/bin-lto-final.params");
SolibSymlinkAction solibSymlinkAction =
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcBinaryThinLtoTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcBinaryThinLtoTest.java
index f0b47dd..53a1fb9 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcBinaryThinLtoTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcBinaryThinLtoTest.java
@@ -186,7 +186,6 @@
.containsAtLeast(
"bin pkg/bin.lto/" + rootExecPath + "/pkg/_objs/bin/binfile.pic.o",
"bin pkg/bin.lto/" + rootExecPath + "/pkg/_objs/lib/libfile.pic.o",
- "bin pkg/bin-2.params",
"bin pkg/bin-lto-final.params");
LtoBackendAction backendAction =
@@ -320,7 +319,6 @@
.containsAtLeast(
"bin pkg/bin.lto/" + rootExecPath + "/pkg/_objs/bin/binfile.pic.o",
"bin _solib_k8/libpkg_Sliblib.ifso",
- "bin pkg/bin-2.params",
"bin pkg/bin-lto-final.params");
SolibSymlinkAction solibSymlinkAction =
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java
index cc64c30..0ff0041 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java
@@ -204,7 +204,7 @@
Artifact executable = getExecutable(archiveInSrcsTest);
CppLinkAction linkAction = (CppLinkAction) getGeneratingAction(executable);
- assertThat(linkAction.getLinkCommandLineForTesting().toString()).contains(" -larchive.34 ");
+ assertThat(linkAction.getLinkCommandLineForTesting().arguments()).contains("-larchive.34");
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java
index 6c36504..057afff 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java
@@ -31,13 +31,12 @@
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
import com.google.devtools.build.lib.actions.ArtifactRoot;
import com.google.devtools.build.lib.actions.ArtifactRoot.RootType;
-import com.google.devtools.build.lib.actions.PathMapper;
+import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType;
import com.google.devtools.build.lib.actions.ResourceSet;
import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.RunfilesProvider;
-import com.google.devtools.build.lib.analysis.actions.ParameterFileWriteAction;
import com.google.devtools.build.lib.analysis.util.ActionTester;
import com.google.devtools.build.lib.analysis.util.ActionTester.ActionCombinationFactory;
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
@@ -421,72 +420,7 @@
ImmutableList.of(),
featureConfiguration)
.build();
- assertThat(linkAction.getIncompleteEnvironmentForTesting()).containsEntry("foo", "bar");
- }
-
- private enum NonStaticAttributes {
- OUTPUT_FILE,
- NATIVE_DEPS,
- USE_TEST_ONLY_FLAGS,
- RUNTIME_SOLIB_DIR,
- ENVIRONMENT,
- }
-
- /**
- * This mainly checks that non-static links don't have identical keys. Many options are only
- * allowed on non-static links, and we test several of them here.
- */
- @Test
- public void testComputeKeyNonStatic() throws Exception {
- final RuleContext ruleContext = createDummyRuleContext();
- final PathFragment exeOutputPath = PathFragment.create("dummyRuleContext/output/path");
- final PathFragment dynamicOutputPath = PathFragment.create("dummyRuleContext/output/path.so");
- final Artifact staticOutputFile = getBinArtifactWithNoOwner(exeOutputPath.getPathString());
- final Artifact dynamicOutputFile = getBinArtifactWithNoOwner(dynamicOutputPath.getPathString());
-
- ActionTester.runTest(
- NonStaticAttributes.class,
- new ActionCombinationFactory<NonStaticAttributes>() {
-
- @Override
- public Action generate(ImmutableSet<NonStaticAttributes> attributesToFlip)
- throws InterruptedException, RuleErrorException {
- CcToolchainProvider toolchain = CppHelper.getToolchain(ruleContext);
- CppLinkActionBuilder builder =
- new CppLinkActionBuilder(
- ruleContext,
- ruleContext,
- ruleContext.getLabel(),
- attributesToFlip.contains(NonStaticAttributes.OUTPUT_FILE)
- ? dynamicOutputFile
- : staticOutputFile,
- ruleContext.getConfiguration(),
- toolchain,
- toolchain.getFdoContext(),
- getMockFeatureConfiguration(
- attributesToFlip.contains(NonStaticAttributes.ENVIRONMENT)
- ? ImmutableMap.of("var", "value")
- : ImmutableMap.of()),
- MockCppSemantics.INSTANCE);
- if (attributesToFlip.contains(NonStaticAttributes.OUTPUT_FILE)) {
- builder.setLinkType(LinkTargetType.NODEPS_DYNAMIC_LIBRARY);
- builder.setLibraryIdentifier("foo");
- } else {
- builder.setLinkType(LinkTargetType.EXECUTABLE);
- }
- builder.setLinkingMode(Link.LinkingMode.DYNAMIC);
- builder.setNativeDeps(attributesToFlip.contains(NonStaticAttributes.NATIVE_DEPS));
- builder.setUseTestOnlyFlags(
- attributesToFlip.contains(NonStaticAttributes.USE_TEST_ONLY_FLAGS));
- builder.setToolchainLibrariesSolibDir(
- attributesToFlip.contains(NonStaticAttributes.RUNTIME_SOLIB_DIR)
- ? null
- : PathFragment.create("so1"));
-
- return builder.build();
- }
- },
- actionKeyContext);
+ assertThat(linkAction.getEffectiveEnvironment(ImmutableMap.of())).containsEntry("foo", "bar");
}
private enum StaticKeyAttributes {
@@ -965,7 +899,7 @@
.setLibraryIdentifier("foo")
.setInterfaceOutput(scratchArtifact("FakeInterfaceOutput.ifso"));
- List<String> commandLine = builder.build().getCommandLine(null);
+ List<String> commandLine = builder.build().getArguments();
assertThat(commandLine).hasSize(6);
assertThat(commandLine.get(0)).endsWith("custom/crosstool/scripts/link_dynamic_library.sh");
assertThat(commandLine.get(1)).isEqualTo("yes");
@@ -1117,10 +1051,7 @@
CppLinkAction linkAction = builder.build();
assertThat(
ImmutableList.copyOf(
- linkAction
- .getLinkCommandLineForTesting()
- .paramCmdLine()
- .arguments(expander, PathMapper.NOOP)))
+ linkAction.getLinkCommandLineForTesting().getParamCommandLine(expander)))
.containsAtLeast(
library0.getExecPathString(),
library1.getExecPathString(),
@@ -1270,10 +1201,8 @@
"foo/BUILD", "cc_binary(", " name = 'foo',", " srcs = ['space .cc', 'quote\".cc'],", ")");
ConfiguredTarget configuredTarget = getConfiguredTarget("//foo:foo");
CppLinkAction linkAction = (CppLinkAction) getGeneratingAction(configuredTarget, "foo/foo");
- ParameterFileWriteAction parameterFileWriteAction = paramFileWriteActionForAction(linkAction);
- assertThat(parameterFileWriteAction).isNotNull();
- assertThat(parameterFileWriteAction.getStringContents()).contains("space\\ .o");
- assertThat(parameterFileWriteAction.getStringContents()).contains("quote\\\".o");
+ assertThat(linkAction.getCommandLines().unpack().get(1).paramFileInfo.getFileType())
+ .isEqualTo(ParameterFileType.GCC_QUOTED);
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java
index 5b07cb0..d594fbd 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java
@@ -207,21 +207,6 @@
}
@Test
- public void testLinkerParamFileIsExported() throws Exception {
- useConfiguration();
-
- scratch.file("x/BUILD", "cc_binary(name = 'bin', srcs = ['some-dir/bar.so'])");
- scratch.file("x/some-dir/bar.so");
-
- ConfiguredTarget target = getConfiguredTarget("//x:bin");
- CcToolchainVariables variables = getLinkBuildVariables(target, Link.LinkTargetType.EXECUTABLE);
- String variableValue =
- getVariableValue(
- getRuleContext(), variables, LinkBuildVariables.LINKER_PARAM_FILE.getVariableName());
- assertThat(variableValue).matches(".*bin/x/bin" + "-2.params$");
- }
-
- @Test
public void testInterfaceLibraryBuildingVariablesWhenLegacyGenerationPossible() throws Exception {
AnalysisMock.get()
.ccSupport()
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 f791356..730daf9 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
@@ -25,6 +25,7 @@
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
import com.google.devtools.build.lib.actions.ArtifactRoot;
import com.google.devtools.build.lib.actions.ArtifactRoot.RootType;
+import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType;
import com.google.devtools.build.lib.actions.PathMapper;
import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
@@ -38,7 +39,6 @@
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.CToolchain;
-import java.io.IOException;
import java.util.Arrays;
import java.util.Collection;
import java.util.List;
@@ -53,19 +53,6 @@
@RunWith(JUnit4.class)
public final class LinkCommandLineTest extends BuildViewTestCase {
- private Artifact scratchArtifact(String s) {
- Path execRoot = outputBase.getRelative("exec");
- String outSegment = "root";
- Path outputRoot = execRoot.getRelative(outSegment);
- ArtifactRoot root = ArtifactRoot.asDerivedRoot(execRoot, RootType.Output, outSegment);
- try {
- return ActionsTestUtil.createArtifact(
- root, scratch.overwriteFile(outputRoot.getRelative(s).toString()));
- } catch (IOException e) {
- throw new RuntimeException(e);
- }
- }
-
private CcToolchainVariables.Builder getMockBuildVariables() {
return getMockBuildVariables(ImmutableList.<String>of());
}
@@ -203,14 +190,16 @@
CcToolchainVariables.Builder variables =
getMockBuildVariables()
.addStringVariable(
- LinkBuildVariables.LINKER_PARAM_FILE.getVariableName(), "foo/bar.param");
+ LinkBuildVariables.LINKER_PARAM_FILE.getVariableName(),
+ "LINKER_PARAM_FILE_PLACEHOLDER");
LinkCommandLine linkConfig =
minimalConfiguration(variables)
.setActionName(LinkTargetType.STATIC_LIBRARY.getActionName())
.setLinkTargetType(LinkTargetType.STATIC_LIBRARY)
+ .setSplitCommandLine(true)
.build();
- assertThat(linkConfig.getCommandLine(null)).contains("@foo/bar.param");
+ assertThat(linkConfig.getCommandLines().unpack().get(1).paramFileInfo.always()).isTrue();
}
@Test
@@ -218,14 +207,16 @@
CcToolchainVariables.Builder variables =
getMockBuildVariables()
.addStringVariable(
- LinkBuildVariables.LINKER_PARAM_FILE.getVariableName(), "foo/bar.param");
+ LinkBuildVariables.LINKER_PARAM_FILE.getVariableName(),
+ "LINKER_PARAM_FILE_PLACEHOLDER");
LinkCommandLine linkConfig =
minimalConfiguration(variables)
.setActionName(LinkTargetType.NODEPS_DYNAMIC_LIBRARY.getActionName())
.setLinkTargetType(LinkTargetType.NODEPS_DYNAMIC_LIBRARY)
+ .setSplitCommandLine(true)
.build();
- assertThat(linkConfig.getCommandLine(null)).contains("@foo/bar.param");
+ assertThat(linkConfig.getCommandLines().unpack().get(1).paramFileInfo.always()).isTrue();
}
private List<String> basicArgv(LinkTargetType targetType) throws Exception {
@@ -274,22 +265,24 @@
@Test
public void testSplitStaticLinkCommand() throws Exception {
useConfiguration("--nostart_end_lib");
- Artifact paramFile = scratchArtifact("some/file.params");
LinkCommandLine linkConfig =
minimalConfiguration(
getMockBuildVariables()
.addStringVariable(
LinkBuildVariables.OUTPUT_EXECPATH.getVariableName(), "a/FakeOutput")
.addStringVariable(
- LinkBuildVariables.LINKER_PARAM_FILE.getVariableName(), "some/file.params"))
+ LinkBuildVariables.LINKER_PARAM_FILE.getVariableName(),
+ "LINKER_PARAM_FILE_PLACEHOLDER"))
.setActionName(LinkTargetType.STATIC_LIBRARY.getActionName())
.setLinkTargetType(LinkTargetType.STATIC_LIBRARY)
.forceToolPath("foo/bar/ar")
- .setParamFile(paramFile)
+ .setSplitCommandLine(true)
+ .setParameterFileType(ParameterFileType.UNQUOTED)
.build();
- assertThat(linkConfig.getCommandLine(null))
- .containsExactly("foo/bar/ar", "@some/file.params")
+ assertThat(linkConfig.getCommandLines().unpack().get(0).commandLine.arguments())
+ .containsExactly("foo/bar/ar")
.inOrder();
+ assertThat(linkConfig.getCommandLines().unpack().get(1).paramFileInfo.always()).isTrue();
assertThat(linkConfig.getParamCommandLine(null))
.containsExactly("rcsD", "a/FakeOutput")
.inOrder();
@@ -298,7 +291,6 @@
@Test
public void testSplitDynamicLinkCommand() throws Exception {
useConfiguration("--nostart_end_lib");
- Artifact paramFile = scratchArtifact("some/file.params");
LinkCommandLine linkConfig =
minimalConfiguration(
getMockBuildVariables()
@@ -311,11 +303,10 @@
.setActionName(LinkTargetType.DYNAMIC_LIBRARY.getActionName())
.setLinkTargetType(LinkTargetType.DYNAMIC_LIBRARY)
.forceToolPath("foo/bar/linker")
- .setParamFile(paramFile)
+ .setSplitCommandLine(true)
.build();
- assertThat(linkConfig.getCommandLine(null))
- .containsExactly("foo/bar/linker", "@some/file.params")
- .inOrder();
+ assertThat(linkConfig.getCommandLines().unpack().get(0).commandLine.arguments())
+ .containsExactly("foo/bar/linker");
assertThat(linkConfig.getParamCommandLine(null))
.containsExactly("-shared", "-o", "a/FakeOutput", "")
.inOrder();
@@ -351,18 +342,16 @@
.addValue(LibraryToLinkValue.forObjectFile("foo.o", false))
.addValue(LibraryToLinkValue.forObjectFile("bar.o", false)));
- Artifact paramFile = scratchArtifact("some/file.params");
LinkCommandLine linkConfig =
minimalConfiguration(variables)
.setActionName(LinkTargetType.ALWAYS_LINK_STATIC_LIBRARY.getActionName())
.setLinkTargetType(LinkTargetType.ALWAYS_LINK_STATIC_LIBRARY)
.forceToolPath("foo/bar/ar")
- .setParamFile(paramFile)
+ .setSplitCommandLine(true)
.build();
- assertThat(linkConfig.getCommandLine(null))
- .containsExactly("foo/bar/ar", "@some/file.params")
- .inOrder();
+ assertThat(linkConfig.getCommandLines().unpack().get(0).commandLine.arguments())
+ .containsExactly("foo/bar/ar");
assertThat(linkConfig.getParamCommandLine(null))
.containsExactly("rcsD", "a/FakeOutput", "foo.o", "bar.o")
.inOrder();
@@ -406,8 +395,6 @@
Iterable<String> treeFileArtifactsPaths =
ImmutableList.of(library0.getExecPathString(), library1.getExecPathString());
- Artifact paramFile = scratchArtifact("some/file.params");
-
LinkCommandLine linkConfig =
minimalConfiguration(
getMockBuildVariables()
@@ -422,7 +409,7 @@
.forceToolPath("foo/bar/gcc")
.setActionName(LinkTargetType.STATIC_LIBRARY.getActionName())
.setLinkTargetType(LinkTargetType.STATIC_LIBRARY)
- .setParamFile(paramFile)
+ .setSplitCommandLine(true)
.build();
// Should only reference the tree artifact.
@@ -430,9 +417,7 @@
linkConfig.arguments(null, PathMapper.NOOP), treeArtifactsPaths, treeFileArtifactsPaths);
verifyArguments(linkConfig.arguments(), treeArtifactsPaths, treeFileArtifactsPaths);
verifyArguments(
- linkConfig.paramCmdLine().arguments(null, PathMapper.NOOP),
- treeArtifactsPaths,
- treeFileArtifactsPaths);
+ linkConfig.getParamCommandLine(null), treeArtifactsPaths, treeFileArtifactsPaths);
// Should only reference tree file artifacts.
verifyArguments(
@@ -442,8 +427,6 @@
verifyArguments(
linkConfig.arguments(expander, null), treeFileArtifactsPaths, treeArtifactsPaths);
verifyArguments(
- linkConfig.paramCmdLine().arguments(expander, PathMapper.NOOP),
- treeFileArtifactsPaths,
- treeArtifactsPaths);
+ linkConfig.getParamCommandLine(expander), treeFileArtifactsPaths, treeArtifactsPaths);
}
}
diff --git a/src/test/py/bazel/bazel_windows_cpp_test.py b/src/test/py/bazel/bazel_windows_cpp_test.py
index e34da3e..542d9ac 100644
--- a/src/test/py/bazel/bazel_windows_cpp_test.py
+++ b/src/test/py/bazel/bazel_windows_cpp_test.py
@@ -376,7 +376,7 @@
bazel_output, 'x64_windows-fastbuild/bin/_objs/A/a.obj.params'
)
link_params = os.path.join(
- bazel_output, 'x64_windows-fastbuild/bin/A_0.dll-2.params'
+ bazel_output, 'x64_windows-fastbuild/bin/A_0.dll-0.params'
)
self.AssertExitCode(exit_code, 0, stderr)
self.AssertFileContentContains(compile_params, '/MD')
@@ -391,7 +391,7 @@
bazel_output, 'x64_windows-dbg/bin/_objs/A/a.obj.params'
)
link_params = os.path.join(
- bazel_output, 'x64_windows-dbg/bin/A_0.dll-2.params'
+ bazel_output, 'x64_windows-dbg/bin/A_0.dll-0.params'
)
self.AssertExitCode(exit_code, 0, stderr)
self.AssertFileContentContains(compile_params, '/MDd')
@@ -412,7 +412,7 @@
bazel_output, 'x64_windows-fastbuild/bin/_objs/A/a.obj.params'
)
link_params = os.path.join(
- bazel_output, 'x64_windows-fastbuild/bin/A_0.dll-2.params'
+ bazel_output, 'x64_windows-fastbuild/bin/A_0.dll-0.params'
)
self.AssertExitCode(exit_code, 0, stderr)
self.AssertFileContentNotContains(compile_params, '/MD')
@@ -429,7 +429,7 @@
bazel_output, 'x64_windows-dbg/bin/_objs/A/a.obj.params'
)
link_params = os.path.join(
- bazel_output, 'x64_windows-dbg/bin/A_0.dll-2.params'
+ bazel_output, 'x64_windows-dbg/bin/A_0.dll-0.params'
)
self.AssertExitCode(exit_code, 0, stderr)
self.AssertFileContentNotContains(compile_params, '/MDd')
@@ -651,7 +651,7 @@
# Test specifying DEF file in cc_binary
exit_code, _, stderr = self.RunBazel(['build', '//:lib_dy.dll', '-s'])
self.AssertExitCode(exit_code, 0, stderr)
- filepath = bazel_bin + '/lib_dy.dll-2.params'
+ filepath = bazel_bin + '/lib_dy.dll-0.params'
with open(filepath, 'r', encoding='latin-1') as param_file:
self.assertIn('/DEF:my_lib.def', param_file.read())
@@ -1027,7 +1027,7 @@
])
bazel_output = self.getBazelInfo('output_path')
- paramfile = 'x64_windows-%s/bin/main.exe-2.params'
+ paramfile = 'x64_windows-%s/bin/main.exe-0.params'
# Test build without debug and optimize modes.
exit_code, _, stderr = self.RunBazel([
diff --git a/src/test/shell/bazel/cc_integration_test.sh b/src/test/shell/bazel/cc_integration_test.sh
index 8a6ecde..def6a43 100755
--- a/src/test/shell/bazel/cc_integration_test.sh
+++ b/src/test/shell/bazel/cc_integration_test.sh
@@ -629,14 +629,11 @@
default = "//${package}:write.sh")})
def _actions_test_impl(target, ctx):
- action = target.actions[1]
+ action = target.actions[0]
if action.mnemonic != "CppArchive":
- fail("Expected the second action to be CppArchive.")
+ fail("Expected the first action to be CppArchive.")
aspect_out = ctx.actions.declare_file('aspect_out')
- ctx.actions.run_shell(inputs = action.inputs,
- outputs = [aspect_out],
- command = "echo \$@ > " + aspect_out.path,
- arguments = action.args)
+ ctx.actions.write(aspect_out, action.args[1])
return [OutputGroupInfo(out=[aspect_out])]
actions_test_aspect = aspect(implementation = _actions_test_impl)
@@ -660,7 +657,9 @@
cat "bazel-bin/${package}/aspect_out" | grep "\(ar\|libtool\)" \
|| fail "args didn't contain the tool path"
- cat "bazel-bin/${package}/aspect_out" | grep "a.*o .*b.*o .*c.*o" \
+ cat "bazel-bin/${package}/aspect_out" | grep "/a.*o" \
+ || fail "args didn't contain tree artifact paths"
+ cat "bazel-bin/${package}/aspect_out" | grep "/b.*o" \
|| fail "args didn't contain tree artifact paths"
}
@@ -813,56 +812,36 @@
),
)
- archive_args = ctx.actions.declare_file("archive_args")
- ctx.actions.run_shell(
- outputs = [archive_args],
- command = "echo \$@ > " + archive_args.path,
- arguments = archive_action.args,
- )
-
archive_out = ctx.actions.declare_file("archive_out.a")
- archive_param_file = None
- for i in archive_action.inputs.to_list():
- if i.path.endswith("params"):
- archive_param_file = i
ctx.actions.run_shell(
- inputs = depset(direct = [archive_args], transitive = [archive_action.inputs]),
+ inputs = archive_action.inputs,
mnemonic = "RecreatedCppArchive",
outputs = [archive_out],
env = archive_action.env,
- command = "\$(cat %s) && cp %s %s" % (
- archive_args.path,
+ command = "\$@ && cp %s %s" % (
archive_action.outputs.to_list()[0].path,
archive_out.path,
),
- )
-
- link_args = ctx.actions.declare_file("link_args")
- ctx.actions.run_shell(
- outputs = [link_args],
- command = "echo \$@ > " + link_args.path,
- arguments = link_action.args,
+ arguments = archive_action.args,
)
link_out = ctx.actions.declare_file("link_out.so")
ctx.actions.run_shell(
- inputs = depset(direct = [link_args], transitive = [link_action.inputs]),
+ inputs = link_action.inputs,
mnemonic = "RecreatedCppLink",
outputs = [link_out],
env = link_action.env,
- command = "\$(cat %s) && cp %s %s" % (
- link_args.path,
+ command = "\$@ && cp %s %s" % (
link_action.outputs.to_list()[0].path,
link_out.path,
),
+ arguments = link_action.args,
)
return [OutputGroupInfo(out = [
compile_args,
compile_out,
- archive_args,
archive_out,
- link_args,
link_out,
])]
@@ -1221,30 +1200,21 @@
),
)
- archive_args = ctx.actions.declare_file("archive_args")
- ctx.actions.run_shell(
- outputs = [archive_args],
- command = "echo \$@ > " + archive_args.path,
- arguments = archive_action.args,
- )
-
archive_out = ctx.actions.declare_file("archive_out.a")
ctx.actions.run_shell(
- inputs = [archive_args],
shadowed_action = archive_action,
mnemonic = "RecreatedCppArchive",
outputs = [archive_out],
- command = "\$(cat %s | sed 's|%s|%s|g')" % (
- archive_args.path,
+ command = "\$@ && cp %s %s" % (
archive_action.outputs.to_list()[0].path,
archive_out.path,
),
+ arguments = archive_action.args,
)
return [OutputGroupInfo(out = [
compile_args,
compile_out,
- archive_args,
archive_out,
])]
diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh
index 453e0bc..c9bcd00 100755
--- a/src/test/shell/bazel/remote/remote_execution_test.sh
+++ b/src/test/shell/bazel/remote/remote_execution_test.sh
@@ -301,7 +301,7 @@
--remote_executor=grpc://localhost:${worker_port} \
//a:test >& $TEST_log \
|| fail "Failed to build //a:test with remote execution"
- expect_log "7 processes: 5 internal, 2 remote"
+ expect_log "6 processes: 4 internal, 2 remote"
diff bazel-bin/a/test ${TEST_TMPDIR}/test_expected \
|| fail "Remote execution generated different result"
}
diff --git a/src/test/shell/integration/aquery_test.sh b/src/test/shell/integration/aquery_test.sh
index ed7172e..0edba69 100755
--- a/src/test/shell/integration/aquery_test.sh
+++ b/src/test/shell/integration/aquery_test.sh
@@ -788,30 +788,6 @@
assert_not_contains "Outputs: " output
}
-function test_aquery_include_param_file_cc_binary() {
- local pkg="${FUNCNAME[0]}"
- mkdir -p "$pkg" || fail "mkdir -p $pkg"
- cat > "$pkg/BUILD" <<'EOF'
-cc_binary(
- name='foo',
- srcs=['foo.cc']
-)
-EOF
-
- # cc_binary targets write param files and use them in CppLinkActions.
- QUERY="//$pkg:foo"
-
- bazel aquery --output=text --include_param_files ${QUERY} > output 2> "$TEST_log" \
- || fail "Expected success"
- cat output >> "$TEST_log"
-
- assert_contains "Command Line: " output
- assert_contains "Params File Content (.*-2.params):" output
-
- bazel aquery --output=textproto --include_param_files ${QUERY} > output \
- 2> "$TEST_log" || fail "Expected success"
-}
-
function test_aquery_include_param_file_starlark_rule() {
local pkg="${FUNCNAME[0]}"
mkdir -p "$pkg" || fail "mkdir -p $pkg"
