Some clean-ups to CppCompileAction and discovering inputs in general:…

… always use stable order for CppCompileAction's inputs, and don't allow updating inputs for any AbstractAction unless the action discovers inputs. PiperOrigin-RevId: 227159745
bazelbuild · Dec 28, 2018 · f65239f · f65239f
1 parent f451cd8
commit f65239f
Show file tree

Hide file tree

Showing 3 changed files with 43 additions and 12 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
@@ -215,6 +215,8 @@ public Iterable<Artifact> getAllowedDerivedInputs() {
    */
   @Override
   public synchronized void updateInputs(Iterable<Artifact> inputs) {
+    Preconditions.checkState(
+        discoversInputs(), "Can't update inputs unless discovering: %s %s", this, inputs);
     this.inputs = CollectionUtils.makeImmutable(inputs);
     inputsDiscovered = true;
   }

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
@@ -236,7 +236,7 @@ public class CppCompileAction extends AbstractAction
       @Nullable Artifact grepIncludes) {
     super(
         owner,
-        NestedSetBuilder.fromNestedSet(mandatoryInputs).addAll(inputsForInvalidation).build(),
+        createInputsBuilder(mandatoryInputs, inputsForInvalidation).build(),
         CollectionUtils.asSetWithoutNulls(
             outputFile,
             dotdFile == null ? null : dotdFile.artifact(),
@@ -941,16 +941,22 @@ private static boolean isDeclaredIn(
     }
   }
 
-  /** Recalculates this action's live input collection, including sources, middlemen. */
+  /**
+   * Recalculates this action's live input collection, including sources, middlemen.
+   *
+   * <p>Can only be called if {@link #discoversInputs}, and must be called after execution in that
+   * case.
+   */
   @VisibleForTesting // productionVisibility = Visibility.PRIVATE
   @ThreadCompatible
-  public final void updateActionInputs(NestedSet<Artifact> discoveredInputs) {
-    NestedSetBuilder<Artifact> inputs = NestedSetBuilder.stableOrder();
+  final void updateActionInputs(NestedSet<Artifact> discoveredInputs) {
+    Preconditions.checkState(
+        discoversInputs(), "Can't call if not discovering inputs: %s %s", discoveredInputs, this);
     try (SilentCloseable c = Profiler.instance().profile(ProfilerTask.ACTION_UPDATE, describe())) {
-      inputs.addTransitive(mandatoryInputs);
-      inputs.addAll(inputsForInvalidation);
-      inputs.addTransitive(discoveredInputs);
-      super.updateInputs(inputs.build());
+      super.updateInputs(
+          createInputsBuilder(mandatoryInputs, inputsForInvalidation)
+              .addTransitive(discoveredInputs)
+              .build());
     }
   }
 
@@ -1209,9 +1215,17 @@ public void close() throws IOException {
     }
     reply = null; // Clear in-memory .d files early.
 
-    // Post-execute "include scanning", which modifies the action inputs to match what the compile
-    // action actually used by incorporating the results of .d file parsing.
-    updateActionInputs(discoveredInputs);
+    if (discoversInputs()) {
+      // Post-execute "include scanning", which modifies the action inputs to match what the compile
+      // action actually used by incorporating the results of .d file parsing.
+      updateActionInputs(discoveredInputs);
+    } else {
+      Preconditions.checkState(
+          discoveredInputs.isEmpty(),
+          "Discovered inputs without discovering inputs? %s %s",
+          discoveredInputs,
+          this);
+    }
 
     // hdrs_check: This cannot be switched off for C++ build actions,
     // because doing so would allow for incorrect builds.
@@ -1487,6 +1501,13 @@ private static ActionLookupData lookupDataFromModule(
             module));
   }
 
+  private static NestedSetBuilder<Artifact> createInputsBuilder(
+      NestedSet<Artifact> mandatoryInputs, Iterable<Artifact> inputsForInvalidation) {
+    return NestedSetBuilder.<Artifact>stableOrder()
+        .addTransitive(mandatoryInputs)
+        .addAll(inputsForInvalidation);
+  }
+
   /**
    * A reference to a .d file. There are two modes:
    *

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java
@@ -193,7 +193,15 @@ public ActionResult execute(ActionExecutionContext actionExecutionContext)
                   e.getMessage() + ";\n  this warning may eventually become an error"));
     }
 
-    updateActionInputs(discoveredInputs);
+    if (discoversInputs()) {
+      updateActionInputs(discoveredInputs);
+    } else {
+      Preconditions.checkState(
+          discoveredInputs.isEmpty(),
+          "Discovered inputs without discovering inputs? %s %s",
+          discoveredInputs,
+          this);
+    }
 
     // Generate a fake ".o" file containing the command line needed to generate
     // the real object file.