Rework implementation of --nobuild_runfile_manifests.

When --nobuild_runfile_manifests is passed, don't create runfiles
input or output manifests at all. This seems better than creating fake
manifest artifacts that are actually a middleman. Fail fast for local
tests and the run command when --nobuild_runfiles_manifests is
passed. (These cases were failing with obscure errors before under
--nobuild_runfile_manifests-I just improved the messaging. See
https://github.com/bazelbuild/bazel/issues/4177.)

Change-Id: I351d26f746ecbe47016b58e4662768a5b6a72ff2
PiperOrigin-RevId: 180659571

4 files changed, 65 insertions, 62 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java
index 84bb2e63ff..2090ddedbb 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java
@@ -16,7 +16,6 @@ package com.google.devtools.build.lib.analysis;
 
 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.analysis.SourceManifestAction.ManifestType;
 import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext;
@@ -25,7 +24,7 @@ import com.google.devtools.build.lib.analysis.actions.SymlinkTreeAction;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.RunUnder;
 import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget.Mode;
-import com.google.devtools.build.lib.collect.nestedset.NestedSet;
+import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.syntax.Type;
@@ -36,6 +35,7 @@ import java.util.Collection;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import javax.annotation.Nullable;
 
 /**
  * This class manages the creation of the runfiles symlink farms.
@@ -120,15 +120,15 @@ public final class RunfilesSupport {
       throw new IllegalStateException("main program " + executable + " not included in runfiles");
     }
 
-    Artifact artifactsMiddleman = createArtifactsMiddleman(ruleContext, runfiles.getAllArtifacts());
     if (createManifest) {
       runfilesInputManifest = createRunfilesInputManifestArtifact(ruleContext);
       runfilesManifest = createRunfilesAction(ruleContext, runfiles);
     } else {
-      runfilesInputManifest = runfilesManifest =
-          createManifestMiddleman(ruleContext, runfiles, artifactsMiddleman);
+      runfilesInputManifest = null;
+      runfilesManifest = null;
     }
-    runfilesMiddleman = createRunfilesMiddleman(ruleContext, artifactsMiddleman, runfilesManifest);
+    runfilesMiddleman =
+        createRunfilesMiddleman(ruleContext, owningExecutable, runfiles, runfilesManifest);
     sourcesManifest = createSourceManifest(ruleContext, runfiles);
 
     this.args = args;
@@ -162,15 +162,15 @@ public final class RunfilesSupport {
   }
 
   /**
-   * For executable programs, the .runfiles_manifest file outside of the
-   * runfiles symlink farm; otherwise, returns null.
+   * Returns the .runfiles_manifest file outside of the runfiles symlink farm. Returns null if
+   * --nobuild_runfile_manifests is in effect.
    *
-   * <p>The MANIFEST file represents the contents of all of the symlinks in the
-   * symlink farm. For efficiency, Blaze's dependency analysis ignores the
-   * actual symlinks and just looks at the MANIFEST file. It is an invariant
-   * that the MANIFEST file should accurately represent the contents of the
-   * symlinks whenever the MANIFEST file is present.
+   * <p>The MANIFEST file represents the contents of all of the symlinks in the symlink farm. For
+   * efficiency, Blaze's dependency analysis ignores the actual symlinks and just looks at the
+   * MANIFEST file. It is an invariant that the MANIFEST file should accurately represent the
+   * contents of the symlinks whenever the MANIFEST file is present.
    */
+  @Nullable
   public Artifact getRunfilesInputManifest() {
     return runfilesInputManifest;
   }
@@ -187,24 +187,26 @@ public final class RunfilesSupport {
   }
 
   /**
-   * For executable programs, returns the MANIFEST file in the runfiles
-   * symlink farm, if blaze is run with --build_runfile_links; returns
-   * the .runfiles_manifest file outside of the symlink farm, if blaze
-   * is run with --nobuild_runfile_links.
-   * <p>
-   * Beware: In most cases {@link #getRunfilesInputManifest} is the more
-   * appropriate function.
+   * Returns the MANIFEST file in the runfiles symlink farm if Bazel is run with
+   * --build_runfile_links. Returns the .runfiles_manifest file outside of the symlink farm, if
+   * Bazel is run with --nobuild_runfile_links. Returns null if --nobuild_runfile_manifests is
+   * passed.
+   *
+   * <p>Beware: In most cases {@link #getRunfilesInputManifest} is the more appropriate function.
    */
+  @Nullable
   public Artifact getRunfilesManifest() {
     return runfilesManifest;
   }
 
-  /**
-   * For executable programs, the root directory of the runfiles symlink farm;
-   * otherwise, returns null.
-   */
+  /** Returns the root directory of the runfiles symlink farm; otherwise, returns null. */
+  @Nullable
   public Path getRunfilesDirectory() {
-    return FileSystemUtils.replaceExtension(getRunfilesInputManifest().getPath(), RUNFILES_DIR_EXT);
+    Artifact inputManifest = getRunfilesInputManifest();
+    if (inputManifest == null) {
+      return null;
+    }
+    return FileSystemUtils.replaceExtension(inputManifest.getPath(), RUNFILES_DIR_EXT);
   }
 
   /**
@@ -265,21 +267,27 @@ public final class RunfilesSupport {
     return sourcesManifest;
   }
 
-  private Artifact createArtifactsMiddleman(
-      ActionConstructionContext context, NestedSet<Artifact> allRunfilesArtifacts) {
-    return context.getAnalysisEnvironment().getMiddlemanFactory().createRunfilesMiddleman(
-        context.getActionOwner(), owningExecutable, allRunfilesArtifacts,
-        context.getMiddlemanDirectory(),
-        "runfiles_artifacts");
-  }
-
-  private Artifact createRunfilesMiddleman(ActionConstructionContext context,
-      Artifact artifactsMiddleman, Artifact outputManifest) {
-    return context.getAnalysisEnvironment().getMiddlemanFactory().createRunfilesMiddleman(
-        context.getActionOwner(), owningExecutable,
-        ImmutableList.of(artifactsMiddleman, outputManifest),
-        context.getMiddlemanDirectory(),
-        "runfiles");
+  private static Artifact createRunfilesMiddleman(
+      ActionConstructionContext context,
+      Artifact owningExecutable,
+      Runfiles runfiles,
+      @Nullable Artifact runfilesManifest) {
+    NestedSetBuilder<Artifact> deps = NestedSetBuilder.stableOrder();
+    deps.addTransitive(runfiles.getAllArtifacts());
+    if (runfilesManifest != null) {
+      deps.add(runfilesManifest);
+    } else {
+      deps.addAll(SourceManifestAction.getDependencies(runfiles));
+    }
+    return context
+        .getAnalysisEnvironment()
+        .getMiddlemanFactory()
+        .createRunfilesMiddleman(
+            context.getActionOwner(),
+            owningExecutable,
+            deps.build(),
+            context.getMiddlemanDirectory(),
+            "runfiles");
   }
 
   /**
@@ -323,25 +331,6 @@ public final class RunfilesSupport {
   }
 
   /**
-   * Creates a middleman artifact which substitutes for the input and
-   * output manifests when manifest files are disabled.
-   */
-  private Artifact createManifestMiddleman(ActionConstructionContext context, Runfiles runfiles,
-      Artifact artifactsMiddleman) {
-    // Deps which are otherwise omitted when we don't build the manifest.
-    // These may include "pruning manifests" which are needed at execution time.
-    Collection<Artifact> manifestDeps = SourceManifestAction.getDependencies(runfiles);
-    if (manifestDeps.isEmpty()) {
-      // Can't create a runfiles middleman from zero artifacts.
-      return artifactsMiddleman;
-    }
-    return context.getAnalysisEnvironment().getMiddlemanFactory().createRunfilesMiddleman(
-        context.getActionOwner(), owningExecutable, SourceManifestAction.getDependencies(runfiles),
-        context.getMiddlemanDirectory(),
-        "runfiles_manifest");
-  }
-
-  /**
    * Creates an {@link Artifact} which writes the "sources only" manifest file.
    *
    * @param context the owner for the manifest action
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetExecutionSettings.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetExecutionSettings.java
index 581296b49b..9994d3e54b 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetExecutionSettings.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetExecutionSettings.java
@@ -28,6 +28,7 @@ import com.google.devtools.build.lib.analysis.config.RunUnder;
 import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget.Mode;
 import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.vfs.Path;
+import javax.annotation.Nullable;
 
 /**
  * Container for common test execution settings shared by all
@@ -42,7 +43,7 @@ public final class TestTargetExecutionSettings {
   private final Artifact runUnderExecutable;
   private final Artifact executable;
   private final boolean runfilesSymlinksCreated;
-  private final Path runfilesDir;
+  @Nullable private final Path runfilesDir;
   private final Runfiles runfiles;
   private final Artifact runfilesInputManifest;
   private final Artifact instrumentedFileManifest;
@@ -108,7 +109,8 @@ public final class TestTargetExecutionSettings {
     return runfilesSymlinksCreated;
   }
 
-  /** @return the directory of the runfiles */
+  /** @return the directory of the runfiles. */
+  @Nullable
   public Path getRunfilesDir() {
     return runfilesDir;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java
index e251cd60a0..43aa25e34c 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java
@@ -26,6 +26,7 @@ import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.CommandLineExpansionException;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.SpawnResult;
+import com.google.devtools.build.lib.actions.TestExecException;
 import com.google.devtools.build.lib.actions.UserExecException;
 import com.google.devtools.build.lib.analysis.config.BinTools;
 import com.google.devtools.build.lib.analysis.test.TestActionContext;
@@ -342,6 +343,11 @@ public abstract class TestStrategy implements TestActionContext {
       boolean enableRunfiles)
       throws ExecException, InterruptedException {
     TestTargetExecutionSettings execSettings = testAction.getExecutionSettings();
+
+    if (execSettings.getInputManifest() == null) {
+      throw new TestExecException("cannot run local tests with --nobuild_runfile_manifests");
+    }
+
     Path runfilesDir = execSettings.getRunfilesDir();
 
     // If the symlink farm is already created then return the existing directory. If not we
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
index 9a0fba3e6c..0dce9adedf 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
@@ -225,6 +225,12 @@ public class RunCommand implements BlazeCommand  {
       configuration = result.getBuildConfigurationCollection().getTargetConfigurations().get(0);
     }
     Path workingDir;
+    if (!configuration.buildRunfilesManifests()) {
+      env.getReporter()
+          .handle(
+              Event.error("--nobuild_runfile_manifests is incompatible with the \"run\" command"));
+      return ExitCode.COMMAND_LINE_ERROR;
+    }
     try {
       workingDir = ensureRunfilesBuilt(env, targetToRun);
     } catch (CommandException e) {
@@ -365,7 +371,7 @@ public class RunCommand implements BlazeCommand  {
       return env.getWorkingDirectory();
     }
 
-    Artifact manifest = runfilesSupport.getRunfilesManifest();
+    Artifact manifest = Preconditions.checkNotNull(runfilesSupport.getRunfilesManifest());
     PathFragment runfilesDir = runfilesSupport.getRunfilesDirectoryExecPath();
     Path workingDir = env.getExecRoot().getRelative(runfilesDir);
     // On Windows, runfiles tree is disabled.