Allow actions to specify if extra actions can attach to them.

--
MOS_MIGRATED_REVID=119203499

8 files changed, 108 insertions, 48 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
index 3196d70530..70dac58587 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
@@ -393,6 +393,11 @@ public abstract class AbstractAction implements Action, SkylarkValue {
   }
 
   @Override
+  public boolean extraActionCanAttach() {
+    return true;
+  }
+
+  @Override
   public ExtraActionInfo.Builder getExtraActionInfo() {
     return ExtraActionInfo.newBuilder()
         .setOwner(getOwner().getLabel().toString())
diff --git a/src/main/java/com/google/devtools/build/lib/actions/Action.java b/src/main/java/com/google/devtools/build/lib/actions/Action.java
index 9f8953b166..e93b37aab1 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/Action.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/Action.java
@@ -158,6 +158,12 @@ public interface Action extends ActionMetadata, Describable {
   boolean showsOutputUnconditionally();
 
   /**
+   * Returns true if an {@link com.google.devtools.build.lib.rules.extra.ExtraAction} action can be
+   * attached to this action. If not, extra actions should not be attached to this action.
+   */
+  boolean extraActionCanAttach();
+
+  /**
    * Called by {@link com.google.devtools.build.lib.rules.extra.ExtraAction} at execution time to
    * extract information from this action into a protocol buffer to be used by extra_action rules.
    *
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ExtraActionUtils.java b/src/main/java/com/google/devtools/build/lib/analysis/ExtraActionUtils.java
index 1bda105f2f..829e637956 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ExtraActionUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ExtraActionUtils.java
@@ -59,12 +59,12 @@ class ExtraActionUtils {
       ExtraActionsVisitor visitor =
           new ExtraActionsVisitor(ruleContext, computeMnemonicsToExtraActionMap(ruleContext));
 
-      // The action list is modified within the body of the loop by the addExtraAction() call,
+      // The action list is modified within the body of the loop by the maybeAddExtraAction() call,
       // thus the copy
-      for (Action action : ImmutableList.copyOf(
-          ruleContext.getAnalysisEnvironment().getRegisteredActions())) {
+      for (Action action :
+          ImmutableList.copyOf(ruleContext.getAnalysisEnvironment().getRegisteredActions())) {
         if (!actionsWithoutExtraAction.contains(action)) {
-          visitor.addExtraAction(action);
+          visitor.maybeAddExtraAction(action);
         }
       }
 
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ExtraActionsVisitor.java b/src/main/java/com/google/devtools/build/lib/analysis/ExtraActionsVisitor.java
index 4f8f3a9373..3b4069ede4 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ExtraActionsVisitor.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ExtraActionsVisitor.java
@@ -48,12 +48,14 @@ final class ExtraActionsVisitor extends ActionGraphVisitor {
     extraArtifacts = Lists.newArrayList();
   }
 
-  public void addExtraAction(Action original) {
-    Collection<ExtraActionSpec> extraActions = mnemonicToExtraActionMap.get(
-        original.getMnemonic());
-    if (extraActions != null) {
-      for (ExtraActionSpec extraAction : extraActions) {
-        extraArtifacts.addAll(extraAction.addExtraAction(ruleContext, original));
+  void maybeAddExtraAction(Action original) {
+    if (original.extraActionCanAttach()) {
+      Collection<ExtraActionSpec> extraActions =
+          mnemonicToExtraActionMap.get(original.getMnemonic());
+      if (extraActions != null) {
+        for (ExtraActionSpec extraAction : extraActions) {
+          extraArtifacts.addAll(extraAction.addExtraAction(ruleContext, original));
+        }
       }
     }
   }
@@ -61,7 +63,7 @@ final class ExtraActionsVisitor extends ActionGraphVisitor {
   @Override
   protected void visitAction(Action action) {
     actions.add(action);
-    addExtraAction(action);
+    maybeAddExtraAction(action);
   }
 
   /** Retrieves the collected artifacts since this method was last called and clears the list. */
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
index 2a18617710..365cc64c84 100644
--- 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
@@ -88,24 +88,31 @@ import javax.annotation.concurrent.GuardedBy;
 @ThreadCompatible
 public class CppCompileAction extends AbstractAction implements IncludeScannable {
   /**
-   * Represents logic that determines which artifacts, if any, should be added to the actual inputs
-   * for each included file (in addition to the included file itself)
+   * Represents logic that determines if an artifact is a special input, meaning that it may require
+   * additional inputs when it is compiled or may not be available to other actions.
    */
-  public interface IncludeResolver {
-    /**
-     * Returns the set of files to be added for an included file (as returned in the .d file)
-     */
+  public interface SpecialInputsHandler {
+    /** Returns if {@code includedFile} is special, so may not be available to other actions. */
+    boolean isSpecialFile(Artifact includedFile);
+
+    /** Returns the set of files to be added for an included file (as returned in the .d file). */
     Collection<Artifact> getInputsForIncludedFile(
         Artifact includedFile, ArtifactResolver artifactResolver);
   }
 
-  public static final IncludeResolver VOID_INCLUDE_RESOLVER = new IncludeResolver() {
-    @Override
-    public Collection<Artifact> getInputsForIncludedFile(Artifact includedFile,
-        ArtifactResolver artifactResolver) {
-      return ImmutableList.of();
-    }
-  };
+  static final SpecialInputsHandler VOID_SPECIAL_INPUTS_HANDLER =
+      new SpecialInputsHandler() {
+        @Override
+        public boolean isSpecialFile(Artifact includedFile) {
+          return false;
+        }
+
+        @Override
+        public Collection<Artifact> getInputsForIncludedFile(
+            Artifact includedFile, ArtifactResolver artifactResolver) {
+          return ImmutableList.of();
+        }
+      };
 
   private static final int VALIDATION_DEBUG = 0;  // 0==none, 1==warns/errors, 2==all
   private static final boolean VALIDATION_DEBUG_WARN = VALIDATION_DEBUG >= 1;
@@ -160,7 +167,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
   @VisibleForTesting
   final CppConfiguration cppConfiguration;
   protected final Class<? extends CppCompileActionContext> actionContext;
-  private final IncludeResolver includeResolver;
+  private final SpecialInputsHandler specialInputsHandler;
 
   /**
    * Identifier for the actual execution time behavior of the action.
@@ -208,7 +215,8 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
    * @param copts options for the compiler
    * @param coptsFilter regular expression to remove options from {@code copts}
    */
-  protected CppCompileAction(ActionOwner owner,
+  protected CppCompileAction(
+      ActionOwner owner,
       // TODO(bazel-team): Eventually we will remove 'features'; all functionality in 'features'
       // will be provided by 'featureConfiguration'.
       ImmutableList<String> features,
@@ -232,7 +240,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
       Predicate<String> coptsFilter,
       ImmutableList<PathFragment> extraSystemIncludePrefixes,
       @Nullable String fdoBuildStamp,
-      IncludeResolver includeResolver,
+      SpecialInputsHandler specialInputsHandler,
       Iterable<IncludeScannable> lipoScannables,
       UUID actionClassId,
       boolean usePic,
@@ -250,7 +258,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
     this.optionalSourceFile = optionalSourceFile;
     this.context = context;
     this.extraSystemIncludePrefixes = extraSystemIncludePrefixes;
-    this.includeResolver = includeResolver;
+    this.specialInputsHandler = specialInputsHandler;
     this.cppConfiguration = cppConfiguration;
     // inputsKnown begins as the logical negation of shouldScanIncludes.
     // When scanning includes, the inputs begin as not known, and become
@@ -413,10 +421,10 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
     ArtifactResolver artifactResolver =
         executor.getContext(IncludeScanningContext.class).getArtifactResolver();
     for (Artifact artifact : initialResult) {
-      result.addAll(includeResolver.getInputsForIncludedFile(artifact, artifactResolver));
+      result.addAll(specialInputsHandler.getInputsForIncludedFile(artifact, artifactResolver));
     }
     for (Artifact artifact : getInputs()) {
-      result.addAll(includeResolver.getInputsForIncludedFile(artifact, artifactResolver));
+      result.addAll(specialInputsHandler.getInputsForIncludedFile(artifact, artifactResolver));
     }
     // TODO(ulfjack): This only works if include scanning is enabled; the cleanup is in progress,
     // and this needs to be fixed before we can even consider disabling it.
@@ -651,6 +659,12 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
   }
 
   @Override
+  public boolean extraActionCanAttach() {
+    return cppConfiguration.alwaysAttachExtraActions()
+        || !specialInputsHandler.isSpecialFile(getPrimaryInput());
+  }
+
+  @Override
   public ExtraActionInfo.Builder getExtraActionInfo() {
     CppCompileInfo.Builder info = CppCompileInfo.newBuilder();
     info.setTool(cppConfiguration.getToolPathFragment(Tool.GCC).getPathString());
@@ -946,7 +960,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
           inputs.add(artifact);
           // In some cases, execution backends need extra files for each included file. Add them
           // to the set of actual inputs.
-          inputs.addAll(includeResolver.getInputsForIncludedFile(artifact, artifactResolver));
+          inputs.addAll(specialInputsHandler.getInputsForIncludedFile(artifact, artifactResolver));
         } else {
           // Abort if we see files that we can't resolve, likely caused by
           // undeclared includes or illegal include constructs.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
index 6cd22d4e60..f56291e0d2 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
@@ -33,7 +33,7 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
 import com.google.devtools.build.lib.rules.cpp.CppCompileAction.DotdFile;
-import com.google.devtools.build.lib.rules.cpp.CppCompileAction.IncludeResolver;
+import com.google.devtools.build.lib.rules.cpp.CppCompileAction.SpecialInputsHandler;
 import com.google.devtools.build.lib.util.FileType;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
@@ -73,7 +73,7 @@ public class CppCompileActionBuilder {
   private ImmutableList<PathFragment> extraSystemIncludePrefixes = ImmutableList.of();
   private String fdoBuildStamp;
   private boolean usePic; 
-  private IncludeResolver includeResolver = CppCompileAction.VOID_INCLUDE_RESOLVER;
+  private SpecialInputsHandler specialInputsHandler = CppCompileAction.VOID_SPECIAL_INPUTS_HANDLER;
   private UUID actionClassId = GUID;
   private Class<? extends CppCompileActionContext> actionContext;
   private CppConfiguration cppConfiguration;
@@ -163,7 +163,7 @@ public class CppCompileActionBuilder {
     this.nocopts.addAll(other.nocopts);
     this.analysisEnvironment = other.analysisEnvironment;
     this.extraSystemIncludePrefixes = ImmutableList.copyOf(other.extraSystemIncludePrefixes);
-    this.includeResolver = other.includeResolver;
+    this.specialInputsHandler = other.specialInputsHandler;
     this.actionClassId = other.actionClassId;
     this.actionContext = other.actionContext;
     this.cppConfiguration = other.cppConfiguration;
@@ -282,16 +282,34 @@ public class CppCompileActionBuilder {
     } else {
       NestedSet<Artifact> realMandatoryInputs = realMandatoryInputsBuilder.build();
 
-      return new CppCompileAction(owner, ImmutableList.copyOf(features), featureConfiguration,
-          variables, sourceFile, shouldScanIncludes, sourceLabel, realMandatoryInputs,
-          outputFile, dotdFile, gcnoFile, getDwoFile(ruleContext, outputFile, cppConfiguration),
-          optionalSourceFile, configuration, cppConfiguration, context,
-          actionContext, ImmutableList.copyOf(copts),
+      return new CppCompileAction(
+          owner,
+          ImmutableList.copyOf(features),
+          featureConfiguration,
+          variables,
+          sourceFile,
+          shouldScanIncludes,
+          sourceLabel,
+          realMandatoryInputs,
+          outputFile,
+          dotdFile,
+          gcnoFile,
+          getDwoFile(ruleContext, outputFile, cppConfiguration),
+          optionalSourceFile,
+          configuration,
+          cppConfiguration,
+          context,
+          actionContext,
+          ImmutableList.copyOf(copts),
           ImmutableList.copyOf(pluginOpts),
           getNocoptPredicate(nocopts),
-          extraSystemIncludePrefixes, fdoBuildStamp,
-          includeResolver, getLipoScannables(realMandatoryInputs), actionClassId,
-          usePic, ruleContext);
+          extraSystemIncludePrefixes,
+          fdoBuildStamp,
+          specialInputsHandler,
+          getLipoScannables(realMandatoryInputs),
+          actionClassId,
+          usePic,
+          ruleContext);
     }
   }
   
@@ -312,8 +330,9 @@ public class CppCompileActionBuilder {
     return this;
   }
 
-  public CppCompileActionBuilder setIncludeResolver(IncludeResolver includeResolver) {
-    this.includeResolver = includeResolver;
+  public CppCompileActionBuilder setSpecialInputsHandler(
+      SpecialInputsHandler specialInputsHandler) {
+    this.specialInputsHandler = specialInputsHandler;
     return this;
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
index 7b1a28ace2..d7fe3a931c 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
@@ -1917,6 +1917,10 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
     return getLinkOptions().contains("-static");
   }
 
+  public boolean alwaysAttachExtraActions() {
+    return true;
+  }
+
   /**
    * Returns true if we should share identical native libraries between different targets.
    */
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
index d445dc24ce..dc5b22dad8 100644
--- 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
@@ -82,7 +82,8 @@ public class FakeCppCompileAction extends CppCompileAction {
       @Nullable String fdoBuildStamp,
       RuleContext ruleContext,
       boolean usePic) {
-    super(owner,
+    super(
+        owner,
         features,
         featureConfiguration,
         variables,
@@ -104,9 +105,18 @@ public class FakeCppCompileAction extends CppCompileAction {
         // cc_fake_binary and for the negative compilation tests that depend on
         // the cc_fake_binary, and the runfiles must be determined at analysis
         // time, so they can't depend on the contents of the ".d" file.)
-        CppCompilationContext.disallowUndeclaredHeaders(context), actionContext, copts, pluginOpts,
-        nocopts, extraSystemIncludePrefixes, fdoBuildStamp, VOID_INCLUDE_RESOLVER,
-        ImmutableList.<IncludeScannable>of(), GUID, usePic, ruleContext);
+        CppCompilationContext.disallowUndeclaredHeaders(context),
+        actionContext,
+        copts,
+        pluginOpts,
+        nocopts,
+        extraSystemIncludePrefixes,
+        fdoBuildStamp,
+        VOID_SPECIAL_INPUTS_HANDLER,
+        ImmutableList.<IncludeScannable>of(),
+        GUID,
+        usePic,
+        ruleContext);
     this.tempOutputFile = Preconditions.checkNotNull(tempOutputFile);
   }