Add --experimental_check_external_other_files option

src/main/java/com/google/devtools/build/lib/pkgcache/PackageOptions.java

@@ -188,6 +188,15 @@ public ParallelismConverter() throws OptionsParsingException {
               + "previous run's cache.")
   public boolean checkOutputFiles;
 
+  @Option(
+          name = "experimental_check_external_other_files",
+          defaultValue = "true",
+          documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+          effectTags = {OptionEffectTag.UNKNOWN},
+          help =
+                  "Check for modifications made to the non-output, non-repo external files, e.g. host files.")
+  public boolean checkExternalOtherFiles;
+
   /** A converter from strings containing comma-separated names of packages to lists of strings. */
   public static class CommaSeparatedPackageNameListConverter
       extends Converter.Contextless<List<PackageIdentifier>> {

src/main/java/com/google/devtools/build/lib/skyframe/DirtinessCheckerUtils.java

@@ -150,7 +150,7 @@ public SkyValueDirtinessChecker.DirtyResult check(
     private static boolean isCacheableType(FileType fileType) {
       switch (fileType) {
         case INTERNAL:
-        case EXTERNAL:
+        case EXTERNAL_OTHER:
         case BUNDLED:
           return true;
         case EXTERNAL_REPO:

src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java

@@ -48,11 +48,11 @@ public class ExternalFilesHelper {
   // These variables are set to true from multiple threads, but only read in the main thread.
   // So volatility or an AtomicBoolean is not needed.
   private boolean anyOutputFilesSeen = false;
-  private boolean tooManyNonOutputExternalFilesSeen = false;
+  private boolean tooManyExternalOtherFilesSeen = false;
   private boolean anyFilesInExternalReposSeen = false;
 
-  // This is a set of external files that are not in external repositories.
-  private Set<RootedPath> nonOutputExternalFilesSeen = Sets.newConcurrentHashSet();
+  // This is the set of EXTERNAL_OTHER files.
+  private Set<RootedPath> externalOtherFilesSeen = Sets.newConcurrentHashSet();
 
   private ExternalFilesHelper(
       AtomicReference<PathPackageLocator> pkgLocator,
@@ -103,7 +103,7 @@ public enum ExternalFileAction {
     DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_PATHS,
 
     /**
-     * For paths of type {@link FileType#EXTERNAL} or {@link FileType#OUTPUT}, assume the path does
+     * For paths of type {@link FileType#EXTERNAL_OTHER} or {@link FileType#OUTPUT}, assume the path does
      * not exist and will never exist.
      */
     ASSUME_NON_EXISTENT_AND_IMMUTABLE_FOR_EXTERNAL_PATHS,
@@ -121,12 +121,6 @@ public enum FileType {
     /** A path inside the package roots. */
     INTERNAL,
 
-    /**
-     * A non {@link #EXTERNAL_REPO} path outside the package roots about which we may make no other
-     * assumptions.
-     */
-    EXTERNAL,
-
     /**
      * A path in Bazel's output tree that's a proper output of an action (*not* a source file in an
      * external repository). Such files are theoretically mutable, but certain Bazel flags may tell
@@ -153,6 +147,14 @@ public enum FileType {
      * RepositoryDirectoryValue is computed.
      */
     EXTERNAL_REPO,
+
+    /**
+     * None of the above. We encounter these paths when outputs, source files or external repos symlink
+     * to files outside aforementioned Bazel-managed directories. For example, C compilation by the
+     * host compiler may depend on /usr/bin/gcc. Bazel makes a best-effort attempt to detect changes
+     * in such files.
+     */
+    EXTERNAL_OTHER,
   }
 
   /**
@@ -164,37 +166,37 @@ static class NonexistentImmutableExternalFileException extends Exception {
 
   static class ExternalFilesKnowledge {
     final boolean anyOutputFilesSeen;
-    final Set<RootedPath> nonOutputExternalFilesSeen;
+    final Set<RootedPath> externalOtherFilesSeen;
     final boolean anyFilesInExternalReposSeen;
-    final boolean tooManyNonOutputExternalFilesSeen;
+    final boolean tooManyExternalOtherFilesSeen;
 
     private ExternalFilesKnowledge(
         boolean anyOutputFilesSeen,
-        Set<RootedPath> nonOutputExternalFilesSeen,
+        Set<RootedPath> externalOtherFilesSeen,
         boolean anyFilesInExternalReposSeen,
-        boolean tooManyNonOutputExternalFilesSeen) {
+        boolean tooManyExternalOtherFilesSeen) {
       this.anyOutputFilesSeen = anyOutputFilesSeen;
-      this.nonOutputExternalFilesSeen = nonOutputExternalFilesSeen;
+      this.externalOtherFilesSeen = externalOtherFilesSeen;
       this.anyFilesInExternalReposSeen = anyFilesInExternalReposSeen;
-      this.tooManyNonOutputExternalFilesSeen = tooManyNonOutputExternalFilesSeen;
+      this.tooManyExternalOtherFilesSeen = tooManyExternalOtherFilesSeen;
     }
   }
 
   @ThreadCompatible
   ExternalFilesKnowledge getExternalFilesKnowledge() {
     return new ExternalFilesKnowledge(
         anyOutputFilesSeen,
-        nonOutputExternalFilesSeen,
+        externalOtherFilesSeen,
         anyFilesInExternalReposSeen,
-        tooManyNonOutputExternalFilesSeen);
+            tooManyExternalOtherFilesSeen);
   }
 
   @ThreadCompatible
   void setExternalFilesKnowledge(ExternalFilesKnowledge externalFilesKnowledge) {
     anyOutputFilesSeen = externalFilesKnowledge.anyOutputFilesSeen;
-    nonOutputExternalFilesSeen = externalFilesKnowledge.nonOutputExternalFilesSeen;
+    externalOtherFilesSeen = externalFilesKnowledge.externalOtherFilesSeen;
     anyFilesInExternalReposSeen = externalFilesKnowledge.anyFilesInExternalReposSeen;
-    tooManyNonOutputExternalFilesSeen = externalFilesKnowledge.tooManyNonOutputExternalFilesSeen;
+    tooManyExternalOtherFilesSeen = externalFilesKnowledge.tooManyExternalOtherFilesSeen;
   }
 
   ExternalFilesHelper cloneWithFreshExternalFilesKnowledge() {
@@ -208,11 +210,11 @@ public FileType getAndNoteFileType(RootedPath rootedPath) {
 
   private Pair<FileType, RepositoryName> getFileTypeAndRepository(RootedPath rootedPath) {
     FileType fileType = detectFileType(rootedPath);
-    if (FileType.EXTERNAL == fileType) {
-      if (nonOutputExternalFilesSeen.size() >= MAX_EXTERNAL_FILES_TO_TRACK) {
-        tooManyNonOutputExternalFilesSeen = true;
+    if (FileType.EXTERNAL_OTHER == fileType) {
+      if (externalOtherFilesSeen.size() >= MAX_EXTERNAL_FILES_TO_TRACK) {
+        tooManyExternalOtherFilesSeen = true;
       } else {
-        nonOutputExternalFilesSeen.add(rootedPath);
+        externalOtherFilesSeen.add(rootedPath);
       }
     }
     if (FileType.EXTERNAL_REPO == fileType) {
@@ -239,7 +241,7 @@ private FileType detectFileType(RootedPath rootedPath) {
     // The outputBase may be null if we're not actually running a build.
     Path outputBase = packageLocator.getOutputBase();
     if (outputBase == null) {
-      return FileType.EXTERNAL;
+      return FileType.EXTERNAL_OTHER;
     }
     if (rootedPath.asPath().startsWith(outputBase)) {
       Path externalRepoDir = outputBase.getRelative(LabelConstants.EXTERNAL_REPOSITORY_LOCATION);
@@ -249,7 +251,7 @@ private FileType detectFileType(RootedPath rootedPath) {
         return FileType.OUTPUT;
       }
     }
-    return FileType.EXTERNAL;
+    return FileType.EXTERNAL_OTHER;
   }
 
   /**
@@ -269,7 +271,7 @@ FileType maybeHandleExternalFile(RootedPath rootedPath, SkyFunction.Environment
       case BUNDLED:
       case INTERNAL:
         break;
-      case EXTERNAL:
+      case EXTERNAL_OTHER:
         if (numExternalFilesLogged.incrementAndGet() < maxNumExternalFilesToLog) {
           logger.atInfo().log("Encountered an external path %s", rootedPath);
         }

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java

@@ -3445,17 +3445,21 @@ public boolean hasDiffAwareness() {
     return diffAwarenessManager != null;
   }
 
+  @VisibleForTesting
+  public void handleDiffsForTesting(ExtendedEventHandler eventHandler) throws InterruptedException, AbruptExitException {
+    handleDiffsForTesting(eventHandler, Options.getDefaults(PackageOptions.class));
+  }
+
   /** Uses diff awareness on all the package paths to invalidate changed files. */
   @VisibleForTesting
-  public void handleDiffsForTesting(ExtendedEventHandler eventHandler)
+  public void handleDiffsForTesting(ExtendedEventHandler eventHandler, PackageOptions packageOptions)
       throws InterruptedException, AbruptExitException {
     if (lastAnalysisDiscarded) {
       // Values were cleared last build, but they couldn't be deleted because they were needed for
       // the execution phase. We can delete them now.
       dropConfiguredTargetsNow(eventHandler);
       lastAnalysisDiscarded = false;
     }
-    PackageOptions packageOptions = Options.getDefaults(PackageOptions.class);
     packageOptions.checkOutputFiles = false;
     ClassToInstanceMap<OptionsBase> options =
         ImmutableClassToInstanceMap.of(PackageOptions.class, packageOptions);
@@ -3558,12 +3562,14 @@ protected WorkspaceInfoFromDiff handleDiffs(
     }
     RepositoryOptions repoOptions = options.getOptions(RepositoryOptions.class);
     try (SilentCloseable c = Profiler.instance().profile("handleDiffsWithMissingDiffInformation")) {
+      PackageOptions packageOptions = options.getOptions(PackageOptions.class);
       handleDiffsWithMissingDiffInformation(
           eventHandler,
           tsgm,
           pathEntriesWithoutDiffInformation,
-          options.getOptions(PackageOptions.class).checkOutputFiles,
-          repoOptions != null && repoOptions.checkExternalRepositoryFiles,
+          packageOptions.checkOutputFiles,
+	  repoOptions != null && repoOptions.checkExternalRepositoryFiles,
+          packageOptions.checkExternalOtherFiles,
           fsvcThreads);
     }
     handleClientEnvironmentChanges();
@@ -3640,6 +3646,7 @@ protected void handleDiffsWithMissingDiffInformation(
       Set<Pair<Root, ProcessableModifiedFileSet>> pathEntriesWithoutDiffInformation,
       boolean checkOutputFiles,
       boolean checkExternalRepositoryFiles,
+      boolean checkNonoutputExternalFiles,
       int fsvcThreads)
       throws InterruptedException, AbruptExitException {
 
@@ -3648,7 +3655,7 @@ protected void handleDiffsWithMissingDiffInformation(
         || (checkOutputFiles && externalFilesKnowledge.anyOutputFilesSeen)
         || (checkExternalRepositoryFiles && repositoryHelpersHolder != null)
         || (checkExternalRepositoryFiles && externalFilesKnowledge.anyFilesInExternalReposSeen)
-        || externalFilesKnowledge.tooManyNonOutputExternalFilesSeen) {
+        || (checkNonoutputExternalFiles && externalFilesKnowledge.tooManyExternalOtherFilesSeen)) {
       // We freshly compute knowledge of the presence of external files in the skyframe graph. We
       // use a fresh ExternalFilesHelper instance and only set the real instance's knowledge *after*
       // we are done with the graph scan, lest an interrupt during the graph scan causes us to
@@ -3692,9 +3699,9 @@ protected void handleDiffsWithMissingDiffInformation(
       if (checkExternalRepositoryFiles) {
         fileTypesToCheck = EnumSet.of(FileType.EXTERNAL_REPO);
       }
-      if (externalFilesKnowledge.tooManyNonOutputExternalFilesSeen
-          || !externalFilesKnowledge.nonOutputExternalFilesSeen.isEmpty()) {
-        fileTypesToCheck.add(FileType.EXTERNAL);
+      if (checkNonoutputExternalFiles && (externalFilesKnowledge.tooManyExternalOtherFilesSeen
+          || !externalFilesKnowledge.externalOtherFilesSeen.isEmpty())) {
+        fileTypesToCheck.add(FileType.EXTERNAL_OTHER);
       }
       // See the comment for FileType.OUTPUT for why we need to consider output files here.
       if (checkOutputFiles) {
@@ -3727,11 +3734,11 @@ protected void handleDiffsWithMissingDiffInformation(
       // once an external file gets into the Skyframe graph, we'll overly-conservatively always
       // think the graph needs to be scanned.
       externalFilesHelper.setExternalFilesKnowledge(
-          tmpExternalFilesHelper.getExternalFilesKnowledge());
-    } else if (!externalFilesKnowledge.nonOutputExternalFilesSeen.isEmpty()) {
+           tmpExternalFilesHelper.getExternalFilesKnowledge());
+    } else if (checkNonoutputExternalFiles && !externalFilesKnowledge.externalOtherFilesSeen.isEmpty()) {
       logger.atInfo().log(
           "About to scan %d external files",
-          externalFilesKnowledge.nonOutputExternalFilesSeen.size());
+          externalFilesKnowledge.externalOtherFilesSeen.size());
       FilesystemValueChecker fsvc =
           new FilesystemValueChecker(
               tsgm,
@@ -3743,7 +3750,7 @@ protected void handleDiffsWithMissingDiffInformation(
       ImmutableBatchDirtyResult batchDirtyResult;
       try (SilentCloseable c = Profiler.instance().profile("fsvc.getDirtyExternalKeys")) {
         Map<SkyKey, SkyValue> externalDirtyNodes = new ConcurrentHashMap<>();
-        for (RootedPath path : externalFilesKnowledge.nonOutputExternalFilesSeen) {
+        for (RootedPath path : externalFilesKnowledge.externalOtherFilesSeen) {
           SkyKey key = FileStateValue.key(path);
           SkyValue value = memoizingEvaluator.getExistingValue(key);
           if (value != null) {
@@ -3758,7 +3765,7 @@ protected void handleDiffsWithMissingDiffInformation(
         batchDirtyResult =
             fsvc.getDirtyKeys(
                 externalDirtyNodes,
-                new ExternalDirtinessChecker(externalFilesHelper, EnumSet.of(FileType.EXTERNAL)));
+                new ExternalDirtinessChecker(externalFilesHelper, EnumSet.of(FileType.EXTERNAL_OTHER)));
       }
       handleChangedFiles(
           ImmutableList.of(), batchDirtyResult, batchDirtyResult.getNumKeysChecked());

src/test/java/com/google/devtools/build/lib/pkgcache/IncrementalLoadingTest.java

@@ -406,8 +406,13 @@ def b():
     tester.addSymlink("a/b.bzl", "/b.bzl");
     tester.sync();
     tester.getTarget("//a:BUILD");
+    PackageOptions packageOptions = Options.getDefaults(PackageOptions.class);
+    packageOptions.checkExternalOtherFiles = false;
     tester.modifyFile("/b.bzl", "ERROR ERROR");
-    tester.sync();
+    tester.syncWithOptions(packageOptions);
+    tester.getTarget("//a:BUILD");
+    packageOptions.checkExternalOtherFiles = true;
+    tester.syncWithOptions(packageOptions);
 
     assertThrows(NoSuchThingException.class, () -> tester.getTarget("//a:BUILD"));
   }
@@ -618,10 +623,13 @@ private ModifiedFileSet getModifiedFileSet() {
     }
 
     void sync() throws InterruptedException, AbruptExitException {
+      syncWithOptions(Options.getDefaults(PackageOptions.class));
+    }
+
+    void syncWithOptions(PackageOptions packageOptions) throws InterruptedException, AbruptExitException {
       clock.advanceMillis(1);
 
       modifiedFileSet = getModifiedFileSet();
-      PackageOptions packageOptions = Options.getDefaults(PackageOptions.class);
       packageOptions.defaultVisibility = RuleVisibility.PUBLIC;
       packageOptions.showLoadingProgress = true;
       packageOptions.globbingThreads = 7;
@@ -642,7 +650,7 @@ void sync() throws InterruptedException, AbruptExitException {
       skyframeExecutor.invalidateFilesUnderPathForTesting(
           new Reporter(new EventBus()), modifiedFileSet, Root.fromPath(workspace));
       ((SequencedSkyframeExecutor) skyframeExecutor)
-          .handleDiffsForTesting(new Reporter(new EventBus()));
+          .handleDiffsForTesting(new Reporter(new EventBus()), packageOptions);
 
       changes.clear();
     }

src/test/java/com/google/devtools/build/lib/skyframe/DirtinessCheckerUtilsTest.java

@@ -105,7 +105,7 @@ public void check_usesSyscallCache_andReturnsNewValue(
             ? new DirtinessCheckerUtils.ExternalDirtinessChecker(
                 externalFilesHelper,
                 EnumSet.of(
-                    ExternalFilesHelper.FileType.INTERNAL, ExternalFilesHelper.FileType.EXTERNAL))
+                    ExternalFilesHelper.FileType.INTERNAL, ExternalFilesHelper.FileType.EXTERNAL_OTHER))
             : createMissingDiffChecker();
 
     boolean shouldCheck = underTest.applies(rootedPath);