Remove Constants.ALLOW_CC_INCLUDE_SCANNING and handle the logic in CppSemantics instead.

--
MOS_MIGRATED_REVID=111406721

9 files changed, 58 insertions, 78 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 df70c821f7..4c50b3a1ae 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
@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.bazel.rules.cpp;
 
 import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.Root;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.rules.cpp.CppCompilationContext.Builder;
 import com.google.devtools.build.lib.rules.cpp.CppCompileActionBuilder;
@@ -44,7 +45,10 @@ public class BazelCppSemantics implements CppSemantics {
       RuleContext ruleContext, CppCompileActionBuilder actionBuilder) {
     actionBuilder.setCppConfiguration(ruleContext.getFragment(CppConfiguration.class));
     actionBuilder.setActionContext(CppCompileActionContext.class);
-    actionBuilder.addTransitiveMandatoryInputs(CppHelper.getToolchain(ruleContext).getCompile());
+    // Because Bazel does not support include scanning, we need the entire crosstool filegroup,
+    // including header files, as opposed to just the "compile" filegroup.
+    actionBuilder.addTransitiveMandatoryInputs(CppHelper.getToolchain(ruleContext).getCrosstool());
+    actionBuilder.setShouldScanIncludes(false);
   }
 
   @Override
@@ -55,4 +59,14 @@ public class BazelCppSemantics implements CppSemantics {
   public HeadersCheckingMode determineHeadersCheckingMode(RuleContext ruleContext) {
     return HeadersCheckingMode.STRICT;
   }
+
+  @Override
+  public boolean needsIncludeScanning(RuleContext ruleContext) {
+    return false;
+  }
+
+  @Override
+  public Root getGreppedIncludesDirectory(RuleContext ruleContext) {
+    return null;
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java
index 0e05856495..280ad7b1f8 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java
@@ -764,11 +764,11 @@ public final class CcLibraryHelper {
     contextBuilder.addDeclaredIncludeSrcs(publicTextualHeaders);
     contextBuilder.addDeclaredIncludeSrcs(privateHeaders);
     contextBuilder.addPregreppedHeaderMap(
-        CppHelper.createExtractInclusions(ruleContext, publicHeaders));
+        CppHelper.createExtractInclusions(ruleContext, semantics, publicHeaders));
     contextBuilder.addPregreppedHeaderMap(
-        CppHelper.createExtractInclusions(ruleContext, publicTextualHeaders));
+        CppHelper.createExtractInclusions(ruleContext, semantics, publicTextualHeaders));
     contextBuilder.addPregreppedHeaderMap(
-        CppHelper.createExtractInclusions(ruleContext, privateHeaders));
+        CppHelper.createExtractInclusions(ruleContext, semantics, privateHeaders));
     contextBuilder.addCompilationPrerequisites(prerequisites);
 
     // Add this package's dir to declaredIncludeDirs, & this rule's headers to declaredIncludeSrcs
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java
index 507880a953..6df75aac15 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java
@@ -125,10 +125,7 @@ public final class CcToolchainProvider implements TransitiveInfoProvider {
    * Returns the files necessary for compilation.
    */
   public NestedSet<Artifact> getCompile() {
-    // If include scanning is disabled, we need the entire crosstool filegroup, including header
-    // files. If it is enabled, we use the filegroup without header files - they are found by
-    // include scanning. For go, we also don't need the header files.
-    return cppConfiguration != null && cppConfiguration.shouldScanIncludes() ? compile : crosstool;
+    return compile;
   }
 
   /**
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 1edaa5e01f..a080d9ed37 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 CppConfiguration cppConfiguration;
   private ImmutableMap<Artifact, IncludeScannable> lipoScannableMap;
   private RuleContext ruleContext = null;
+  private Boolean shouldScanIncludes;
   // New fields need to be added to the copy constructor.
 
   /**
@@ -167,6 +168,7 @@ public class CppCompileActionBuilder {
     this.usePic = other.usePic;
     this.lipoScannableMap = other.lipoScannableMap;
     this.ruleContext = other.ruleContext;
+    this.shouldScanIncludes = other.shouldScanIncludes;
   }
 
   public PathFragment getTempOutputFile() {
@@ -245,18 +247,13 @@ public class CppCompileActionBuilder {
    * action).
    */
   public CppCompileAction build() {
+    // This must be set either to false or true by CppSemantics, otherwise someone forgot to call
+    // finalizeCompileActionBuilder on this builder.
+    Preconditions.checkNotNull(shouldScanIncludes);
+
     // Configuration can be null in tests.
     NestedSetBuilder<Artifact> realMandatoryInputsBuilder = NestedSetBuilder.compileOrder();
     realMandatoryInputsBuilder.addTransitive(mandatoryInputsBuilder.build());
-    String filename = sourceFile.getFilename();
-    // Assembler without C preprocessing can use the '.include' pseudo-op which is not
-    // understood by the include scanner, so we'll disable scanning, and instead require
-    // the declared sources to state (possibly overapproximate) the dependencies.
-    // Assembler with preprocessing can also use '.include', but supporting both kinds
-    // of inclusion for that use-case is ridiculous.
-    boolean shouldScanIncludes = !CppFileTypes.ASSEMBLER.matches(filename)
-        && configuration != null
-        && configuration.getFragment(CppConfiguration.class).shouldScanIncludes();
     if (tempOutputFile == null && !shouldScanIncludes) {
       realMandatoryInputsBuilder.addTransitive(context.getDeclaredIncludeSrcs());
     }
@@ -458,4 +455,12 @@ public class CppCompileActionBuilder {
     this.usePic = usePic;
     return this;
   }
+
+  public void setShouldScanIncludes(boolean shouldScanIncludes) {
+    this.shouldScanIncludes = shouldScanIncludes;
+  }
+
+  public boolean getShouldScanIncludes() {
+    return shouldScanIncludes;
+  }
 }
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 2f4c9e61e2..31aa2f0fee 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
@@ -25,7 +25,6 @@ import com.google.common.collect.Iterables;
 import com.google.common.collect.ListMultimap;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Multimap;
-import com.google.devtools.build.lib.Constants;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactFactory;
 import com.google.devtools.build.lib.actions.PackageRootResolutionException;
@@ -48,7 +47,6 @@ import com.google.devtools.build.lib.rules.cpp.CppConfigurationLoader.CppConfigu
 import com.google.devtools.build.lib.rules.cpp.FdoSupport.FdoException;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
-import com.google.devtools.build.lib.util.IncludeScanningUtil;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
@@ -354,7 +352,6 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
    *  CppCompilationContexts, but registering build actions is disabled.
    */
   private final boolean lipoContextCollector;
-  private final Root greppedIncludesDirectory;
 
   protected CppConfiguration(CppConfigurationParameters params)
       throws InvalidConfigurationException {
@@ -372,11 +369,6 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
     this.lipoContextCollector = cppOptions.lipoCollector;
     this.execRoot = params.execRoot;
 
-    // Note that the grepped includes directory is not configuration-specific; the paths of the
-    // files within that directory, however, are configuration-specific.
-    this.greppedIncludesDirectory = Root.asDerivedRoot(execRoot,
-        execRoot.getRelative(IncludeScanningUtil.GREPPED_INCLUDES));
-
     this.crosstoolTopPathFragment = crosstoolTop.getPackageIdentifier().getPathFragment();
 
     try {
@@ -935,13 +927,6 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
     return pathPrefix.getRelative(path);
   }
 
-  /**
-   * Returns the configuration-independent grepped-includes directory.
-   */
-  public Root getGreppedIncludesDirectory() {
-    return greppedIncludesDirectory;
-  }
-
   @VisibleForTesting
   List<String> configureLinkerOptions(
       CompilationMode compilationMode, LipoMode lipoMode, LinkingMode linkingMode,
@@ -1459,10 +1444,6 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
     return commandLineDefines.get(var);
   }
 
-  public boolean shouldScanIncludes() {
-    return Constants.ALLOW_CC_INCLUDE_SCANNING && cppOptions.scanIncludes;
-  }
-
   /**
    * Returns the currently active LIPO compilation mode.
    */
@@ -1583,14 +1564,6 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
   }
 
   /**
-   * Returns true iff this build configuration requires inclusion extraction
-   * (for include scanning) in the action graph.
-   */
-  public boolean needsIncludeScanning() {
-    return cppOptions.extractInclusions;
-  }
-
-  /**
    * Returns true if shared libraries must be compiled with position independent code
    * on this platform or in this configuration.
    */
@@ -1915,9 +1888,6 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
     if (fdoSupport.getFdoRoot() != null) {
       roots.add(fdoSupport.getFdoRoot());
     }
-
-    // Grepped header includes; this root is not configuration specific.
-    roots.add(getGreppedIncludesDirectory());
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java
index 16939f6466..94c689657c 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java
@@ -260,15 +260,15 @@ public class CppHelper {
    * performance problems if many headers are generated.
    */
   @Nullable
-  public static final Map<Artifact, Artifact> createExtractInclusions(RuleContext ruleContext,
-      Iterable<Artifact> prerequisites) {
+  public static final Map<Artifact, Artifact> createExtractInclusions(
+      RuleContext ruleContext, CppSemantics semantics, Iterable<Artifact> prerequisites) {
     Map<Artifact, Artifact> extractions = new HashMap<>();
     for (Artifact prerequisite : prerequisites) {
       if (extractions.containsKey(prerequisite)) {
         // Don't create duplicate actions just because user specified same header file twice.
         continue;
       }
-      Artifact scanned = createExtractInclusions(ruleContext, prerequisite);
+      Artifact scanned = createExtractInclusions(ruleContext, semantics, prerequisite);
       if (scanned != null) {
         extractions.put(prerequisite, scanned);
       }
@@ -284,13 +284,13 @@ public class CppHelper {
    * .cc source in serial. For high-latency file systems, this could cause
    * performance problems if many headers are generated.
    */
-  private static final Artifact createExtractInclusions(RuleContext ruleContext,
-      Artifact prerequisite) {
-    if (ruleContext != null &&
-        ruleContext.getFragment(CppConfiguration.class).needsIncludeScanning() &&
-        !prerequisite.isSourceArtifact() &&
-        CPP_FILETYPES.matches(prerequisite.getFilename())) {
-      Artifact scanned = getIncludesOutput(ruleContext, prerequisite);
+  private static final Artifact createExtractInclusions(
+      RuleContext ruleContext, CppSemantics semantics, Artifact prerequisite) {
+    if (ruleContext != null
+        && semantics.needsIncludeScanning(ruleContext)
+        && !prerequisite.isSourceArtifact()
+        && CPP_FILETYPES.matches(prerequisite.getFilename())) {
+      Artifact scanned = getIncludesOutput(ruleContext, semantics, prerequisite);
       ruleContext.registerAction(
           new ExtractInclusionAction(ruleContext.getActionOwner(), prerequisite, scanned));
       return scanned;
@@ -298,8 +298,9 @@ public class CppHelper {
     return null;
   }
 
-  private static Artifact getIncludesOutput(RuleContext ruleContext, Artifact src) {
-    Root root = ruleContext.getFragment(CppConfiguration.class).getGreppedIncludesDirectory();
+  private static Artifact getIncludesOutput(
+      RuleContext ruleContext, CppSemantics semantics, Artifact src) {
+    Root root = semantics.getGreppedIncludesDirectory(ruleContext);
     PathFragment relOut = IncludeScanningUtil.getRootRelativeOutputPath(src.getExecPath());
     return ruleContext.getShareableArtifact(relOut, root);
   }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java
index bb7bd649a2..c4b240cfb8 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java
@@ -1119,9 +1119,6 @@ public final class CppLinkAction extends AbstractAction {
       this.linkstamps.addAll(linkstamps.keySet());
       // Add inputs for linkstamping.
       if (!linkstamps.isEmpty()) {
-        // This will just be the compiler unless include scanning is disabled, in which case it will
-        // include all header files. Since we insist that linkstamps declare all their headers, all
-        // header files would be overkill, but that only happens when include scanning is disabled.
         addTransitiveCompilationInputs(toolchain.getCompile());
         for (Map.Entry<Artifact, ImmutableList<Artifact>> entry : linkstamps.entrySet()) {
           addCompilationInputs(entry.getValue());
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java
index ee1d3e077c..4733e741d0 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java
@@ -202,20 +202,6 @@ public class CppOptions extends FragmentOptions {
              "All ELF toolchains currently support this setting.")
   public boolean useInterfaceSharedObjects;
 
-  @Option(name = "cc_include_scanning",
-          defaultValue = "true",
-          category = "strategy",
-          help = "Whether to perform include scanning. Without it, your build will most likely "
-              + "fail.")
-  public boolean scanIncludes;
-
-  @Option(name = "extract_generated_inclusions",
-          defaultValue = "true",
-          category = "undocumented",
-          help = "Run grep-includes actions (used for include scanning) over " +
-                 "generated headers and sources.")
-  public boolean extractInclusions;
-
   @Option(name = "fission",
           defaultValue = "no",
           converter = FissionOptionConverter.class,
@@ -513,11 +499,9 @@ public class CppOptions extends FragmentOptions {
 
     host.useThinArchives = useThinArchives;
     host.useStartEndLib = useStartEndLib;
-    host.extractInclusions = extractInclusions;
     host.stripBinaries = StripMode.ALWAYS;
     host.fdoOptimize = null;
     host.lipoMode = LipoMode.OFF;
-    host.scanIncludes = scanIncludes;
     host.inmemoryDotdFiles = inmemoryDotdFiles;
 
     return host;
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 b09739d598..cc726a51f5 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
@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.rules.cpp;
 
 import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.Root;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.rules.cpp.CppConfiguration.HeadersCheckingMode;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -52,4 +53,15 @@ public interface CppSemantics {
    * Determines the applicable mode of headers checking for the passed in ruleContext.
    */
   HeadersCheckingMode determineHeadersCheckingMode(RuleContext ruleContext);
+
+  /**
+   * Returns true iff this build configuration requires inclusion extraction (for include scanning)
+   * in the action graph.
+   */
+  boolean needsIncludeScanning(RuleContext ruleContext);
+
+  /**
+   * Returns the configuration-independent grepped-includes directory.
+   */
+  Root getGreppedIncludesDirectory(RuleContext ruleContext);
 }