Migrate C++ link action .params files to the Blaze-standard ParameterFileWriteAction.
Performance changes:
- output files of actions require an extra system call
+ incremental builds no longer require re-writing the .param file (typically)
--
MOS_MIGRATED_REVID=95842983
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ParameterFile.java b/src/main/java/com/google/devtools/build/lib/actions/ParameterFile.java
index 80df9e2..1528eff 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ParameterFile.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ParameterFile.java
@@ -14,16 +14,9 @@
package com.google.devtools.build.lib.actions;
import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.Preconditions;
import com.google.devtools.build.lib.util.FileType;
-import com.google.devtools.build.lib.util.ShellEscaper;
-import com.google.devtools.build.lib.vfs.FileSystemUtils;
-import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
-import java.io.IOException;
-import java.nio.charset.Charset;
-import java.util.List;
/**
* Support for parameter file generation (as used by gcc and other tools, e.g.
@@ -62,27 +55,14 @@
SHELL_QUOTED;
}
- // Parameter file location.
- private final Path execRoot;
- private final PathFragment execPath;
- private final Charset charset;
- private final ParameterFileType type;
-
@VisibleForTesting
public static final FileType PARAMETER_FILE = FileType.of(".params");
/**
* Creates a parameter file with the given parameters.
*/
- public ParameterFile(Path execRoot, PathFragment execPath, Charset charset,
- ParameterFileType type) {
- Preconditions.checkNotNull(type);
- this.execRoot = execRoot;
- this.execPath = execPath;
- this.charset = Preconditions.checkNotNull(charset);
- this.type = Preconditions.checkNotNull(type);
+ private ParameterFile() {
}
-
/**
* Derives an exec path from a given exec path by appending <code>".params"</code>.
*/
@@ -90,25 +70,4 @@
return original.replaceName(original.getBaseName() + "-2.params");
}
- /**
- * Returns the path for the parameter file.
- */
- public Path getPath() {
- return execRoot.getRelative(execPath);
- }
-
- /**
- * Writes the arguments from the list into the parameter file according to
- * the style selected in the constructor.
- */
- public void writeContent(List<String> arguments) throws ExecException {
- Iterable<String> actualArgs = (type == ParameterFileType.SHELL_QUOTED) ?
- ShellEscaper.escapeAll(arguments) : arguments;
- Path file = getPath();
- try {
- FileSystemUtils.writeLinesAs(file, charset, actualArgs);
- } catch (IOException e) {
- throw new EnvironmentalExecException("could not write param file '" + file + "'", e);
- }
- }
}
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 97db66e..4d63994 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
@@ -25,21 +25,21 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.actions.AbstractAction;
+import com.google.devtools.build.lib.actions.Action;
import com.google.devtools.build.lib.actions.ActionExecutionContext;
import com.google.devtools.build.lib.actions.ActionExecutionException;
import com.google.devtools.build.lib.actions.ActionOwner;
import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.actions.EnvironmentalExecException;
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.Executor;
import com.google.devtools.build.lib.actions.ParameterFile;
-import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType;
import com.google.devtools.build.lib.actions.ResourceSet;
import com.google.devtools.build.lib.actions.extra.CppLinkInfo;
import com.google.devtools.build.lib.actions.extra.ExtraActionInfo;
import com.google.devtools.build.lib.analysis.AnalysisEnvironment;
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.TransitiveInfoProvider;
+import com.google.devtools.build.lib.analysis.actions.ParameterFileWriteAction;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.collect.CollectionUtils;
import com.google.devtools.build.lib.collect.ImmutableIterable;
@@ -197,58 +197,25 @@
}
/**
- * Prepares and returns the command line specification for this link.
- * Splits appropriate parts into a .params file and adds any required
- * linkstamp compilation steps.
+ * Returns the command line specification for this link, included any required linkstamp
+ * compilation steps. The command line may refer to a .params file.
*
* @return a finalized command line suitable for execution
*/
- public final List<String> prepareCommandLine(Path execRoot, List<String> inputFiles)
+ public final List<String> getCommandLine()
throws ExecException {
List<String> commandlineArgs;
// Try to shorten the command line by use of a parameter file.
// This makes the output with --subcommands (et al) more readable.
if (linkCommandLine.canBeSplit()) {
- PathFragment paramExecPath = ParameterFile.derivePath(
- outputLibrary.getArtifact().getExecPath());
- Pair<List<String>, List<String>> split = linkCommandLine.splitCommandline(paramExecPath);
+ Pair<List<String>, List<String>> split = linkCommandLine.splitCommandline();
commandlineArgs = split.first;
- writeToParamFile(execRoot, paramExecPath, split.second);
- if (inputFiles != null) {
- inputFiles.add(paramExecPath.getPathString());
- }
} else {
commandlineArgs = linkCommandLine.getRawLinkArgv();
}
return linkCommandLine.finalizeWithLinkstampCommands(commandlineArgs);
}
- private static void writeToParamFile(Path workingDir, PathFragment paramExecPath,
- List<String> paramFileArgs) throws ExecException {
- // Create parameter file.
- ParameterFile paramFile = new ParameterFile(workingDir, paramExecPath, ISO_8859_1,
- ParameterFileType.UNQUOTED);
- Path paramFilePath = paramFile.getPath();
- try {
- // writeContent() fails for existing files that are marked readonly.
- paramFilePath.delete();
- } catch (IOException e) {
- throw new EnvironmentalExecException("could not delete file '" + paramFilePath + "'", e);
- }
- paramFile.writeContent(paramFileArgs);
-
- // Normally Blaze chmods all output files automatically (see
- // SkyframeActionExecutor#setOutputsReadOnlyAndExecutable), but this params file is created
- // out-of-band and is not declared as an output. By chmodding the file, other processes
- // can observe this file being created.
- try {
- paramFilePath.setWritable(false);
- paramFilePath.setExecutable(true); // for consistency with other action outputs
- } catch (IOException e) {
- throw new EnvironmentalExecException("could not chmod param file '" + paramFilePath + "'", e);
- }
- }
-
@Override
@ThreadCompatible
public void execute(
@@ -655,6 +622,11 @@
interfaceOutputLibrary == null ? null : interfaceOutputLibrary.getArtifact(),
symbolCountOutput);
+ final PathFragment paramFileFragment =
+ supportsParamFiles
+ ? ParameterFile.derivePath(outputLibrary.getArtifact().getExecPath())
+ : null;
+
LinkCommandLine linkCommandLine = new LinkCommandLine.Builder(configuration, getOwner())
.setOutput(outputLibrary.getArtifact())
.setInterfaceOutput(interfaceOutput)
@@ -673,7 +645,7 @@
.setUseTestOnlyFlags(useTestOnlyFlags)
.setNeedWholeArchive(needWholeArchive)
.setInterfaceSoBuilder(getInterfaceSoBuilder())
- .setSupportsParamFiles(supportsParamFiles)
+ .setParamFileFragment(paramFileFragment)
.build();
// Compute the set of inputs - we only need stable order here.
@@ -691,16 +663,24 @@
needWholeArchive, cppConfiguration.archiveType()));
// getPrimaryInput returns the first element, and that is a public interface - therefore the
// order here is important.
- Iterable<Artifact> inputs = IterablesChain.<Artifact>builder()
+ IterablesChain.Builder<Artifact> inputsBuilder = IterablesChain.<Artifact>builder()
.add(ImmutableList.copyOf(LinkerInputs.toLibraryArtifacts(nonLibraries)))
.add(dependencyInputsBuilder.build())
- .add(ImmutableIterable.from(expandedInputs))
- .deduplicate()
- .build();
+ .add(ImmutableIterable.from(expandedInputs));
+
+ if (linkCommandLine.canBeSplit()) {
+ Artifact paramFile = createArtifact(
+ ParameterFile.derivePath(outputLibrary.getArtifact().getRootRelativePath()));
+ inputsBuilder.add(ImmutableList.of(paramFile));
+ Action parameterFileWriteAction = new ParameterFileWriteAction(
+ getOwner(), paramFile, linkCommandLine.paramCmdLine(),
+ ParameterFile.ParameterFileType.UNQUOTED, ISO_8859_1);
+ analysisEnvironment.registerAction(parameterFileWriteAction);
+ }
return new CppLinkAction(
getOwner(),
- inputs,
+ inputsBuilder.deduplicate().build(),
actionOutputs,
cppConfiguration,
outputLibrary,
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 99568c9..f8fc37e 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
@@ -74,7 +74,7 @@
private final boolean nativeDeps;
private final boolean useTestOnlyFlags;
private final boolean needWholeArchive;
- private final boolean supportsParamFiles;
+ private final PathFragment paramFileExecPath;
@Nullable private final Artifact interfaceSoBuilder;
private LinkCommandLine(
@@ -96,7 +96,7 @@
boolean nativeDeps,
boolean useTestOnlyFlags,
boolean needWholeArchive,
- boolean supportsParamFiles,
+ PathFragment paramFileFragment,
Artifact interfaceSoBuilder) {
Preconditions.checkArgument(linkTargetType != LinkTargetType.INTERFACE_DYNAMIC_LIBRARY,
"you can't link an interface dynamic library directly");
@@ -143,7 +143,7 @@
this.nativeDeps = nativeDeps;
this.useTestOnlyFlags = useTestOnlyFlags;
this.needWholeArchive = needWholeArchive;
- this.supportsParamFiles = supportsParamFiles;
+ this.paramFileExecPath = paramFileFragment;
// For now, silently ignore interfaceSoBuilder if we don't build an interface dynamic library.
this.interfaceSoBuilder =
((linkTargetType == LinkTargetType.DYNAMIC_LIBRARY) && (interfaceOutput != null))
@@ -256,7 +256,7 @@
* @throws IllegalStateException if the command-line cannot be split
*/
@VisibleForTesting
- final Pair<List<String>, List<String>> splitCommandline(PathFragment paramExecPath) {
+ final Pair<List<String>, List<String>> splitCommandline() {
Preconditions.checkState(canBeSplit());
List<String> args = getRawLinkArgv();
if (linkTargetType.isStaticLibraryLink()) {
@@ -265,7 +265,7 @@
List<String> commandlineArgs = new ArrayList<>();
commandlineArgs.add(args.get(0));
- commandlineArgs.add("@" + paramExecPath.getPathString());
+ commandlineArgs.add("@" + paramFileExecPath.getPathString());
return Pair.of(commandlineArgs, paramFileArgs);
} else {
// Gcc link commands tend to generate humongous commandlines for some targets, which may
@@ -275,13 +275,28 @@
List<String> commandlineArgs = new ArrayList<>();
extractArgumentsForParamFile(args, commandlineArgs, paramFileArgs);
- commandlineArgs.add("-Wl,@" + paramExecPath.getPathString());
+ commandlineArgs.add("-Wl,@" + paramFileExecPath.getPathString());
return Pair.of(commandlineArgs, paramFileArgs);
}
}
+ /**
+ * Returns just the .params file portion of the command-line as a {@link CommandLine}.
+ *
+ * @throws IllegalStateException if the command-line cannot be split
+ */
+ CommandLine paramCmdLine() {
+ Preconditions.checkState(canBeSplit());
+ return new CommandLine() {
+ @Override
+ public Iterable<String> arguments() {
+ return splitCommandline().getSecond();
+ }
+ };
+ }
+
boolean canBeSplit() {
- if (!supportsParamFiles) {
+ if (paramFileExecPath == null) {
return false;
}
switch (linkTargetType) {
@@ -931,7 +946,7 @@
private boolean nativeDeps;
private boolean useTestOnlyFlags;
private boolean needWholeArchive;
- private boolean supportsParamFiles;
+ private PathFragment paramFileFragment;
@Nullable private Artifact interfaceSoBuilder;
public Builder(BuildConfiguration configuration, ActionOwner owner) {
@@ -954,7 +969,7 @@
return new LinkCommandLine(configuration, owner, output, interfaceOutput,
symbolCountsOutput, buildInfoHeaderArtifacts, linkerInputs, runtimeInputs, linkTargetType,
linkStaticness, linkopts, features, linkstamps, actualLinkstampCompileOptions,
- runtimeSolibDir, nativeDeps, useTestOnlyFlags, needWholeArchive, supportsParamFiles,
+ runtimeSolibDir, nativeDeps, useTestOnlyFlags, needWholeArchive, paramFileFragment,
interfaceSoBuilder);
}
@@ -1116,8 +1131,8 @@
return this;
}
- public Builder setSupportsParamFiles(boolean supportsParamFiles) {
- this.supportsParamFiles = supportsParamFiles;
+ public Builder setParamFileFragment(PathFragment paramFragment) {
+ this.paramFileFragment = paramFragment;
return this;
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LocalLinkStrategy.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LocalLinkStrategy.java
index 3e7c863..bce40d5 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LocalLinkStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LocalLinkStrategy.java
@@ -43,8 +43,7 @@
public void exec(CppLinkAction action, ActionExecutionContext actionExecutionContext)
throws ExecException, ActionExecutionException, InterruptedException {
Executor executor = actionExecutionContext.getExecutor();
- List<String> argv =
- action.prepareCommandLine(executor.getExecRoot(), null);
+ List<String> argv = action.getCommandLine();
executor.getSpawnActionContext(action.getMnemonic()).exec(
new BaseSpawn.Local(argv, ImmutableMap.<String, String>of(), action),
actionExecutionContext);
