Create injectable semantics for include validation, to allow it to be turned

off in the objc case.

--
MOS_MIGRATED_REVID=131943500

7 files changed, 62 insertions, 30 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCppSemantics.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCppSemantics.java
index 737e69e4c6..a037904d44 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCppSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCppSemantics.java
@@ -67,4 +67,9 @@ public class BazelCppSemantics implements CppSemantics {
   @Override
   public void validateAttributes(RuleContext ruleContext) {
   }
+
+  @Override
+  public boolean needsIncludeValidation() {
+    return 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
index 0debf56da0..308b7a0686 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
@@ -176,6 +176,7 @@ public class CppCompileAction extends AbstractAction
   private final FeatureConfiguration featureConfiguration;
   protected final Class<? extends CppCompileActionContext> actionContext;
   private final SpecialInputsHandler specialInputsHandler;
+  private final CppSemantics semantics;
 
   /**
    * Identifier for the actual execution time behavior of the action.
@@ -202,32 +203,29 @@ public class CppCompileAction extends AbstractAction
   /**
    * Creates a new action to compile C/C++ source files.
    *
-   * @param owner the owner of the action, usually the configured target that
-   *        emitted it
-   * @param sourceFile the source file that should be compiled. {@code mandatoryInputs} must
-   *        contain this file
-   * @param shouldScanIncludes a boolean indicating whether scanning of {@code sourceFile}
-   *        is to be performed looking for inclusions.
+   * @param owner the owner of the action, usually the configured target that emitted it
+   * @param sourceFile the source file that should be compiled. {@code mandatoryInputs} must contain
+   *     this file
+   * @param shouldScanIncludes a boolean indicating whether scanning of {@code sourceFile} is to be
+   *     performed looking for inclusions.
    * @param sourceLabel the label of the rule the source file is generated by
-   * @param mandatoryInputs any additional files that need to be present for the
-   *        compilation to succeed, can be empty but not null, for example, extra sources for FDO.
-   * @param outputFile the object file that is written as result of the
-   *        compilation, or the fake object for {@link FakeCppCompileAction}s
-   * @param dotdFile the .d file that is generated as a side-effect of
-   *        compilation
-   * @param gcnoFile the coverage notes that are written in coverage mode, can
-   *        be null
-   * @param dwoFile the .dwo output file where debug information is stored for Fission
-   *        builds (null if Fission mode is disabled)
+   * @param mandatoryInputs any additional files that need to be present for the compilation to
+   *     succeed, can be empty but not null, for example, extra sources for FDO.
+   * @param outputFile the object file that is written as result of the compilation, or the fake
+   *     object for {@link FakeCppCompileAction}s
+   * @param dotdFile the .d file that is generated as a side-effect of compilation
+   * @param gcnoFile the coverage notes that are written in coverage mode, can be null
+   * @param dwoFile the .dwo output file where debug information is stored for Fission builds (null
+   *     if Fission mode is disabled)
    * @param optionalSourceFile an additional optional source file (null if unneeded)
    * @param configuration the build configurations
    * @param context the compilation context
    * @param copts options for the compiler
    * @param coptsFilter regular expression to remove options from {@code copts}
    * @param executionRequirements out-of-band hints to be passed to the execution backend to signal
-   *        platform requirements
+   *     platform requirements
    * @param actionName a string giving the name of this action for the purpose of toolchain
-   *        evaluation
+   *     evaluation
    */
   protected CppCompileAction(
       ActionOwner owner,
@@ -258,7 +256,8 @@ public class CppCompileAction extends AbstractAction
       ImmutableSet<String> executionRequirements,
       ImmutableMap<String, String> environment,
       String actionName,
-      RuleContext ruleContext) {
+      RuleContext ruleContext,
+      CppSemantics semantics) {
     super(
         owner,
         createInputs(
@@ -297,6 +296,7 @@ public class CppCompileAction extends AbstractAction
     // artifact and will definitely exist prior to this action execution.
     this.mandatoryInputs = mandatoryInputs;
     this.builtinIncludeFiles = CppHelper.getToolchain(ruleContext).getBuiltinIncludeFiles();
+    this.semantics = semantics;
     verifyIncludePaths(ruleContext);
   }
 
@@ -994,7 +994,10 @@ public class CppCompileAction extends AbstractAction
           problems.add(execPathFragment.getPathString());
         }
       }
-      problems.assertProblemFree(this, getSourceFile());
+      //TODO(b/22551695): Remove in favor of seperate implementations.
+      if (semantics.needsIncludeValidation()) {
+        problems.assertProblemFree(this, getSourceFile());
+      }
     } catch (IOException e) {
       // Some kind of IO or parse exception--wrap & rethrow it to stop the build.
       throw new ActionExecutionException("error while parsing .d file", e, this, false);
@@ -1196,11 +1199,14 @@ public class CppCompileAction extends AbstractAction
       updateActionInputs(discoveredInputs);
     }
 
-    // hdrs_check: This cannot be switched off, because doing so would allow for incorrect builds.
-    validateInclusions(
-        discoveredInputs,
-        actionExecutionContext.getArtifactExpander(),
-        executor.getEventHandler());
+    // hdrs_check: Turning this off opens the door to incorrect builds.  However, we allow it
+    // to accommodate the current behavior in the objc rules.
+    if (semantics.needsIncludeValidation()) {
+      validateInclusions(
+          discoveredInputs,
+          actionExecutionContext.getArtifactExpander(),
+          executor.getEventHandler());
+    }
   }
 
   /**
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 0fe142b181..260cc66b37 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
@@ -79,6 +79,7 @@ public class CppCompileActionBuilder {
   private RuleContext ruleContext = null;
   private Boolean shouldScanIncludes;
   private Map<String, String> environment = new LinkedHashMap<>();
+  private CppSemantics cppSemantics;
   // New fields need to be added to the copy constructor.
 
   /**
@@ -147,6 +148,7 @@ public class CppCompileActionBuilder {
     this.ruleContext = other.ruleContext;
     this.shouldScanIncludes = other.shouldScanIncludes;
     this.environment = new LinkedHashMap<>(other.environment);
+    this.cppSemantics = other.cppSemantics;
   }
 
   public PathFragment getTempOutputFile() {
@@ -289,7 +291,8 @@ public class CppCompileActionBuilder {
           actionContext,
           ImmutableList.copyOf(copts),
           getNocoptPredicate(nocopts),
-          ruleContext);
+          ruleContext,
+          cppSemantics);
     } else {
       NestedSet<Artifact> realMandatoryInputs = realMandatoryInputsBuilder.build();
 
@@ -320,7 +323,8 @@ public class CppCompileActionBuilder {
           executionRequirements,
           ImmutableMap.copyOf(environment),
           getActionName(),
-          ruleContext);
+          ruleContext,
+          cppSemantics);
     }
   }
   
@@ -481,6 +485,12 @@ public class CppCompileActionBuilder {
     return this;
   }
 
+  /** Sets the CppSemantics for this compile. */
+  public CppCompileActionBuilder setSemantics(CppSemantics semantics) {
+    this.cppSemantics = semantics;
+    return this;
+  }
+  
   public void setShouldScanIncludes(boolean shouldScanIncludes) {
     this.shouldScanIncludes = shouldScanIncludes;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
index 432dc3ca15..798d90998b 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
@@ -520,6 +520,8 @@ public final class CppModel {
       CppCompileActionBuilder builder =
           initializeCompileAction(sourceArtifact, sourceLabel, /*forInterface=*/ false);
 
+      builder.setSemantics(semantics);
+      
       if (CppFileTypes.CPP_HEADER.matches(source.getSource().getExecPath())) {
         createHeaderAction(outputName, result, env, builder,
             CppFileTypes.mustProduceDotdFile(sourceArtifact.getFilename()));
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppSemantics.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppSemantics.java
index 62f0e69da9..ec224b2d34 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppSemantics.java
@@ -60,4 +60,7 @@ public interface CppSemantics {
   boolean needsIncludeScanning(RuleContext ruleContext);
 
   void validateAttributes(RuleContext ruleContext);
+
+  /** Returns true iff this build requires include validation. */
+  boolean needsIncludeValidation();
 }
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 4b68bcb229..e82f352ce9 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
@@ -42,7 +42,6 @@ import com.google.devtools.build.lib.util.ShellEscaper;
 import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.PathFragment;
-
 import java.io.IOException;
 import java.util.UUID;
 import java.util.logging.Logger;
@@ -78,7 +77,8 @@ public class FakeCppCompileAction extends CppCompileAction {
       Class<? extends CppCompileActionContext> actionContext,
       ImmutableList<String> copts,
       Predicate<String> nocopts,
-      RuleContext ruleContext) {
+      RuleContext ruleContext,
+      CppSemantics semantics) {
     super(
         owner,
         features,
@@ -113,7 +113,8 @@ public class FakeCppCompileAction extends CppCompileAction {
         ImmutableSet.<String>of(),
         ImmutableMap.<String, String>of(),
         CppCompileAction.CPP_COMPILE,
-        ruleContext);
+        ruleContext,
+        semantics);
     this.tempOutputFile = Preconditions.checkNotNull(tempOutputFile);
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCppSemantics.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCppSemantics.java
index effd78fe7f..7e408c4bb9 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCppSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCppSemantics.java
@@ -86,4 +86,9 @@ public class ObjcCppSemantics implements CppSemantics {
   @Override
   public void validateAttributes(RuleContext ruleContext) {
   }
+
+  @Override
+  public boolean needsIncludeValidation() {
+    return false;
+  }
 }