ObjcCompileAction does not signal to skyframe that it discovers inputs.

Instead, it skips discovery (include scanning), but provides all headers to
action
execution to allow for re-adding pruned sources in a sandbox.

This means that mis-capitalization errors will only break a build if --objc_use_dotd_pruning is one.

--
MOS_MIGRATED_REVID=139456194

4 files changed, 81 insertions, 9 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java
index b452b470ba..16c8ba95fb 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java
@@ -426,14 +426,20 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie
   }
 
   /**
-   * The Spawn which this SpawnAction will execute.
+   * A spawn instance that is tied to a specific SpawnAction.
    */
-  private class ActionSpawn extends BaseSpawn {
+  public class ActionSpawn extends BaseSpawn {
 
     private final List<Artifact> filesets = new ArrayList<>();
 
     private final ImmutableMap<String, String> effectiveEnvironment;
 
+    /**
+     * Creates an ActionSpawn with the given environment variables.
+     * 
+     * <p>Subclasses of ActionSpawn may subclass in order to provide action-specific values for
+     * environment variables or action inputs.
+     */
     public ActionSpawn(Map<String, String> clientEnv) {
       super(ImmutableList.copyOf(argv.arguments()),
           ImmutableMap.<String, String>of(),
@@ -460,6 +466,9 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie
       effectiveEnvironment = ImmutableMap.copyOf(env);
     }
 
+    /**
+     * Creates an ActionSpawn with no environment variables.
+     */
     public ActionSpawn() {
       this(null);
     }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppFileTypes.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppFileTypes.java
index 394a925190..a20ed3d09a 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppFileTypes.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppFileTypes.java
@@ -33,6 +33,9 @@ public final class CppFileTypes {
       FileTypeSet.of(CppFileTypes.CPP_SOURCE, CppFileTypes.C_SOURCE);
 
   public static final FileType CPP_HEADER = FileType.of(".h", ".hh", ".hpp", ".hxx", ".inc");
+  public static final FileType PCH = FileType.of(".pch");
+  public static final FileTypeSet OBJC_HEADER = FileTypeSet.of(CPP_HEADER, PCH);
+  
   public static final FileType CPP_TEXTUAL_INCLUDE = FileType.of(".inc");
 
   public static final FileType PIC_PREPROCESSED_C = FileType.of(".pic.i");
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/LegacyCompilationSupport.java b/src/main/java/com/google/devtools/build/lib/rules/objc/LegacyCompilationSupport.java
index 4658c5bd15..42596b5efb 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/LegacyCompilationSupport.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/LegacyCompilationSupport.java
@@ -467,12 +467,13 @@ public class LegacyCompilationSupport extends CompilationSupport {
                 appleConfiguration, appleConfiguration.getSingleArchPlatform())
             .setDotdPruningPlan(objcConfiguration.getDotdPruningPlan())
             .setSourceFile(sourceFile)
+            .addTransitiveHeaders(objcProvider.get(HEADER))
+            .addHeaders(compilationArtifacts.getPrivateHdrs())  
             .addMandatoryInputs(swiftHeader.asSet())
             .addTransitiveMandatoryInputs(moduleMapInputs)
             .addTransitiveMandatoryInputs(objcProvider.get(STATIC_FRAMEWORK_FILE))
             .addTransitiveMandatoryInputs(objcProvider.get(DYNAMIC_FRAMEWORK_FILE))
             .setDotdFile(dotdFile)
-            .addInputs(compilationArtifacts.getPrivateHdrs())
             .addInputs(compilationArtifacts.getPchFile().asSet())
             .setMnemonic("ObjcCompile")
             .setExecutable(xcrunwrapper(ruleContext))
@@ -480,7 +481,6 @@ public class LegacyCompilationSupport extends CompilationSupport {
             .addOutput(objFile)
             .addOutputs(gcnoFile.asSet())
             .addOutput(dotdFile.artifact())
-            .addTransitiveInputs(objcProvider.get(HEADER))
             .build(ruleContext));
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCompileAction.java
index d41b1570b2..faad216147 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCompileAction.java
@@ -23,6 +23,7 @@ import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.ActionExecutionException;
+import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ActionOwner;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactResolver;
@@ -30,6 +31,7 @@ import com.google.devtools.build.lib.actions.Executor;
 import com.google.devtools.build.lib.actions.PackageRootResolutionException;
 import com.google.devtools.build.lib.actions.PackageRootResolver;
 import com.google.devtools.build.lib.actions.ResourceSet;
+import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.analysis.actions.CommandLine;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
@@ -40,6 +42,7 @@ import com.google.devtools.build.lib.profiler.ProfilerTask;
 import com.google.devtools.build.lib.rules.apple.AppleConfiguration;
 import com.google.devtools.build.lib.rules.apple.Platform;
 import com.google.devtools.build.lib.rules.cpp.CppCompileAction.DotdFile;
+import com.google.devtools.build.lib.rules.cpp.CppFileTypes;
 import com.google.devtools.build.lib.rules.cpp.HeaderDiscovery;
 import com.google.devtools.build.lib.rules.cpp.HeaderDiscovery.DotdPruningMode;
 import com.google.devtools.build.lib.rules.cpp.IncludeScanningContext;
@@ -66,10 +69,30 @@ import javax.annotation.concurrent.GuardedBy;
  */
 public class ObjcCompileAction extends SpawnAction {
 
+ /**
+   * A spawn that provides all headers to sandboxed execution to allow pruned headers to be
+   * re-introduced into action inputs.
+   */
+  public class ObjcCompileActionSpawn extends ActionSpawn {
+    
+    public ObjcCompileActionSpawn(Map<String, String> clientEnv) {
+      super(clientEnv);
+    }
+
+    @Override
+    public Iterable<? extends ActionInput> getInputFiles() {
+      return ImmutableList.<ActionInput>builder()
+          .addAll(super.getInputFiles())
+          .addAll(filterHeaderFiles())
+          .build();      
+    }
+  }
+  
   private final DotdFile dotdFile;
   private final Artifact sourceFile;
   private final NestedSet<Artifact> mandatoryInputs;
   private final HeaderDiscovery.DotdPruningMode dotdPruningPlan;
+  private final NestedSet<Artifact> headers;
 
   // This can be read/written from multiple threads, so accesses must be synchronized.
   @GuardedBy("this")
@@ -94,7 +117,8 @@ public class ObjcCompileAction extends SpawnAction {
       DotdFile dotdFile,
       Artifact sourceFile,
       NestedSet<Artifact> mandatoryInputs,
-      HeaderDiscovery.DotdPruningMode dotdPruningPlan) {
+      HeaderDiscovery.DotdPruningMode dotdPruningPlan,
+      NestedSet<Artifact> headers) {
     super(
         owner,
         tools,
@@ -116,6 +140,18 @@ public class ObjcCompileAction extends SpawnAction {
     this.mandatoryInputs = mandatoryInputs;
     this.dotdPruningPlan = dotdPruningPlan;
     this.inputsKnown = (dotdPruningPlan == DotdPruningMode.DO_NOT_USE);
+    this.headers = headers;
+  }
+
+  private Iterable<Artifact> filterHeaderFiles() {
+    ImmutableList.Builder<Artifact> inputs = ImmutableList.<Artifact>builder();
+
+    for (Artifact headerArtifact : headers) {
+      if (CppFileTypes.OBJC_HEADER.matches(headerArtifact.getFilename())) {
+          inputs.add(headerArtifact);
+        }
+    }
+    return inputs.build();    
   }
 
   /** Returns the DotdPruningPlan for this compile */
@@ -130,15 +166,24 @@ public class ObjcCompileAction extends SpawnAction {
   }
   
   @Override
+  public final Spawn getSpawn(Map<String, String> clientEnv) {
+    return new ObjcCompileActionSpawn(clientEnv);
+  }
+  
+  @Override
   public boolean discoversInputs() {
     return true;
   }
 
   @Override
   public Iterable<Artifact> discoverInputs(ActionExecutionContext actionExecutionContext) {
-    // We do not use include scanning for objc
     return null;
   }
+
+  @Override
+  public Iterable<Artifact> getInputsWhenSkippingInputDiscovery() {
+    return filterHeaderFiles();
+  }
  
   // Keep in sync with {@link CppCompileAction#resolveInputsFromCache}
   @Override
@@ -148,8 +193,7 @@ public class ObjcCompileAction extends SpawnAction {
       Collection<PathFragment> inputPaths)
       throws PackageRootResolutionException, InterruptedException {
     // Note that this method may trigger a violation of the desirable invariant that getInputs()
-    // is a superset of getMandatoryInputs(). See bug about an "action not in canonical form"
-    // error message and the integration test test_crosstool_change_and_failure().
+    // is a superset of getMandatoryInputs().
     Map<PathFragment, Artifact> allowedDerivedInputsMap = getAllowedDerivedInputsMap();
     List<Artifact> inputs = new ArrayList<>();
     List<PathFragment> unresolvedPaths = new ArrayList<>();
@@ -296,6 +340,7 @@ public class ObjcCompileAction extends SpawnAction {
     private Artifact sourceFile;
     private final NestedSetBuilder<Artifact> mandatoryInputs = new NestedSetBuilder<>(STABLE_ORDER);
     private HeaderDiscovery.DotdPruningMode dotdPruningPlan;
+    private final NestedSetBuilder<Artifact> headers = NestedSetBuilder.stableOrder();
 
     /**
      * Creates a new compile action builder with apple environment variables set that are typically
@@ -363,6 +408,20 @@ public class ObjcCompileAction extends SpawnAction {
       this.dotdPruningPlan = dotdPruningPlan;
       return this;
     }
+    
+    /** Adds to the set of all possible headers that could be required by this compile action. */
+    public Builder addTransitiveHeaders(NestedSet<Artifact> headers) {
+      this.headers.addTransitive(Preconditions.checkNotNull(headers));
+      this.addTransitiveInputs(headers);
+      return this;
+    }
+
+    /** Adds to the set of all possible headers that could be required by this compile action. */
+    public Builder addHeaders(Iterable<Artifact> headers) {
+      this.headers.addAll(Preconditions.checkNotNull(headers));
+      this.addInputs(headers);
+      return this;
+    }
 
     @Override
     protected SpawnAction createSpawnAction(
@@ -395,7 +454,8 @@ public class ObjcCompileAction extends SpawnAction {
           dotdFile,
           sourceFile,
           mandatoryInputs.build(),
-          dotdPruningPlan);
+          dotdPruningPlan,
+          headers.build());
     }
   }
 }