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

4 files changed, 53 insertions, 100 deletions

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 80df9e20ac..1528eff604 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 @@ public class ParameterFile {
     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 @@ public class ParameterFile {
     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 97db66e25b..4d6399457e 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.ImmutableMap;
 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 @@ public final class CppLinkAction extends AbstractAction {
   }
 
   /**
-   * 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 @@ public final class CppLinkAction extends AbstractAction {
           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 @@ public final class CppLinkAction extends AbstractAction {
           .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 @@ public final class CppLinkAction extends AbstractAction {
               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 99568c94a6..f8fc37e0a1 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 @@ public final class LinkCommandLine extends CommandLine {
   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 @@ public final class LinkCommandLine extends CommandLine {
       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 @@ public final class LinkCommandLine extends CommandLine {
     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 @@ public final class LinkCommandLine extends CommandLine {
    * @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 @@ public final class LinkCommandLine extends CommandLine {
       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 @@ public final class LinkCommandLine extends CommandLine {
       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 @@ public final class LinkCommandLine extends CommandLine {
     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 @@ public final class LinkCommandLine extends CommandLine {
       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 @@ public final class LinkCommandLine extends CommandLine {
       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 3e7c863f42..bce40d5716 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 final class LocalLinkStrategy extends LinkStrategy {
   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);