For source directory action inputs, depend on all files in the subtree

ulfjack · Copybara-Service · commit c64421bc3521 · 2018-10-26T00:58:01.000-07:00
This fixes a number of long-standing correctness issues where Bazel did not
notice if files within the purview of an action changed.

Consider this BUILD rule:
genrule(
  name = "bad",
  outs = ["ls.txt"],
  srcs = ["dir"],
  cmd = "ls -l test/dir &gt; $@",
)

If "dir" is a directory, then Bazel does not notice changes to files
underneath dir, and also does not give an error message. It only emits a
warning like this:
"WARNING: /path/to/BUILD:1:1: input 'test/dir' to //test:bad is a directory; dependency checking of directories is unsound"

We use a startup flag for rollout since the new code requires additional
Skyframe dependencies. Use like this:
bazel --host_jvm_args=-DBAZEL_TRACK_SOURCE_DIRECTORIES=1 build ...

With this change, Bazel assumes that all the files underneath dir/ are action
inputs (i.e., it implicitly globs all the files) and reruns the action
whenever any of these files change. The main differences to glob(["dir/**"])
are:
- the file names do not have to be valid labels; the label syntax currently
  restricts the allowed characters to a small subset of UTF-8 (mainly ASCII)
- the files cannot be directly referenced from another package unless they
  are also explicitly mentioned in the package
- globbing happens during execution, not loading; this can have a significant
  effect on performance for packages that have a lot of globs / directory
  references - globs are evaluated eagerly during loading, even if only a
  single rule from that package is needed

This change does not affect remote execution, which continues to not allow
such inputs, although this could be changed subsequently.

We may in the future require that directory references in BUILD files end with
a trailing forward slash ('/') to indicate this fact to a reader of the BUILD
file.

PiperOrigin-RevId: 218817101
diff --git a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java
@@ -248,6 +248,10 @@ public static FileArtifactValue createNormalFile(byte[] digest, long size) {
     return createNormalFile(digest, /*proxy=*/ null, size);
   }
 
+  public static FileArtifactValue createDirectoryWithHash(byte[] digest) {
+    return new HashedDirectoryArtifactValue(digest);
+  }
+
   public static FileArtifactValue createDirectory(long mtime) {
     return new DirectoryArtifactValue(mtime);
   }
@@ -300,6 +304,65 @@ public String toString() {
     }
   }
 
+  private static final class HashedDirectoryArtifactValue extends FileArtifactValue {
+    private final byte[] digest;
+
+    private HashedDirectoryArtifactValue(byte[] digest) {
+      this.digest = digest;
+    }
+
+    @Override
+    public FileStateType getType() {
+      return FileStateType.DIRECTORY;
+    }
+
+    @Nullable
+    @Override
+    public byte[] getDigest() {
+      return digest;
+    }
+
+    @Override
+    public long getModifiedTime() {
+      return 0;
+    }
+
+    @Override
+    public long getSize() {
+      return 0;
+    }
+
+    @Override
+    public boolean wasModifiedSinceDigest(Path path) throws IOException {
+      // TODO(ulfjack): Ideally, we'd attempt to detect intra-build modifications here. I'm
+      // consciously deferring work here as this code will most likely change again, and we're
+      // already doing better than before by detecting inter-build modifications.
+      return false;
+    }
+
+    @Override
+    public String toString() {
+      return MoreObjects.toStringHelper(this).add("digest", digest).toString();
+    }
+
+    @Override
+    public boolean equals(Object o) {
+      if (this == o) {
+        return true;
+      }
+      if (!(o instanceof HashedDirectoryArtifactValue)) {
+        return false;
+      }
+      HashedDirectoryArtifactValue r = (HashedDirectoryArtifactValue) o;
+      return Arrays.equals(digest, r.digest);
+    }
+
+    @Override
+    public int hashCode() {
+      return Arrays.hashCode(digest);
+    }
+  }
+
   private static final class RegularFileArtifactValue extends FileArtifactValue {
     private final byte[] digest;
     @Nullable private final FileContentsProxy proxy;
diff --git a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleAction.java b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleAction.java
@@ -30,6 +30,7 @@
 import com.google.devtools.build.lib.actions.SpawnResult;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction;
 import com.google.devtools.build.lib.events.EventHandler;
+import com.google.devtools.build.lib.skyframe.TrackSourceDirectoriesFlag;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import java.util.List;
 
@@ -74,7 +75,9 @@ public GenRuleAction(
   protected List<SpawnResult> internalExecute(ActionExecutionContext actionExecutionContext)
       throws ExecException, InterruptedException {
     EventHandler reporter = actionExecutionContext.getEventHandler();
-    checkInputsForDirectories(reporter, actionExecutionContext.getMetadataProvider());
+    if (!TrackSourceDirectoriesFlag.trackSourceDirectories()) {
+      checkInputsForDirectories(reporter, actionExecutionContext.getMetadataProvider());
+    }
     List<SpawnResult> spawnResults = ImmutableList.of();
     try {
       spawnResults = super.internalExecute(actionExecutionContext);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
@@ -33,10 +33,16 @@
 import com.google.devtools.build.lib.actions.ArtifactSkyKey;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.actions.FileValue;
+import com.google.devtools.build.lib.actions.FilesetTraversalParams.DirectTraversalRoot;
+import com.google.devtools.build.lib.actions.FilesetTraversalParams.PackageBoundaryMode;
 import com.google.devtools.build.lib.actions.MissingInputFileException;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
+import com.google.devtools.build.lib.skyframe.RecursiveFilesystemTraversalFunction.RecursiveFilesystemTraversalException;
+import com.google.devtools.build.lib.skyframe.RecursiveFilesystemTraversalValue.ResolvedFile;
+import com.google.devtools.build.lib.skyframe.RecursiveFilesystemTraversalValue.TraversalRequest;
+import com.google.devtools.build.lib.util.Fingerprint;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.vfs.RootedPath;
 import com.google.devtools.build.skyframe.SkyFunction;
@@ -73,6 +79,8 @@ public SkyValue compute(SkyKey skyKey, Environment env)
         // the above side effect of posting an event to the EventBus. Importantly, that event
         // is potentially used to report root causes.
         throw new ArtifactFunctionException(e, Transience.TRANSIENT);
+      } catch (IOException e) {
+        throw new ArtifactFunctionException(e, Transience.TRANSIENT);
       }
     }
 
@@ -217,9 +225,9 @@ public TreeFileArtifact apply(Artifact artifact) {
   }
 
   private FileArtifactValue createSourceValue(Artifact artifact, boolean mandatory, Environment env)
-      throws MissingInputFileException, InterruptedException {
-    SkyKey fileSkyKey =
-        FileValue.key(RootedPath.toRootedPath(artifact.getRoot().getRoot(), artifact.getPath()));
+      throws MissingInputFileException, IOException, InterruptedException {
+    RootedPath path = RootedPath.toRootedPath(artifact.getRoot().getRoot(), artifact.getPath());
+    SkyKey fileSkyKey = FileValue.key(path);
     FileValue fileValue;
     try {
       fileValue = (FileValue) env.getValueOrThrow(fileSkyKey, IOException.class);
@@ -236,6 +244,45 @@ private FileArtifactValue createSourceValue(Artifact artifact, boolean mandatory
         throw makeMissingInputFileException(artifact, mandatory, null, env.getListener());
       }
     }
+    // For directory artifacts that are not Filesets, we initiate a directory traversal here, and
+    // compute a hash from the directory structure.
+    if (fileValue.isDirectory() && TrackSourceDirectoriesFlag.trackSourceDirectories()) {
+      // We rely on the guarantees of RecursiveFilesystemTraversalFunction for correctness.
+      //
+      // This approach may have unexpected interactions with --package_path. In particular, the exec
+      // root is setup from the loading / analysis phase, and it is now too late to change it;
+      // therefore, this may traverse a different set of files depending on which targets are built
+      // at the same time and what the package-path layout is (this may be moot if there is only one
+      // entry). Or this may return a set of files that's inconsistent with those actually available
+      // to the action (for local execution).
+      //
+      // In the future, we need to make this result the source of truth for the files available to
+      // the action so that we at least have consistency.
+      TraversalRequest request = TraversalRequest.create(
+          DirectTraversalRoot.forRootedPath(path),
+          /*isRootGenerated=*/ false,
+          PackageBoundaryMode.CROSS,
+          /*strictOutputFiles=*/ true,
+          /*skipTestingForSubpackage=*/ true,
+          /*errorInfo=*/ null);
+      RecursiveFilesystemTraversalValue value;
+      try {
+        value =
+            (RecursiveFilesystemTraversalValue) env.getValueOrThrow(
+                request, RecursiveFilesystemTraversalException.class);
+      } catch (RecursiveFilesystemTraversalException e) {
+        throw new IOException(e);
+      }
+      if (value == null) {
+        return null;
+      }
+      Fingerprint fp = new Fingerprint();
+      for (ResolvedFile file : value.getTransitiveFiles()) {
+        fp.addString(file.getNameInSymlinkTree().getPathString());
+        fp.addInt(file.getMetadata().hashCode());
+      }
+      return FileArtifactValue.createDirectoryWithHash(fp.digestAndReset());
+    }
     try {
       return FileArtifactValue.create(artifact, fileValue);
     } catch (IOException e) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TrackSourceDirectoriesFlag.java b/src/main/java/com/google/devtools/build/lib/skyframe/TrackSourceDirectoriesFlag.java
@@ -0,0 +1,37 @@
+// Copyright 2018 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.skyframe;
+
+/**
+ * A flag to enable / disable tracking of source directories. Uses a system property which can be
+ * set via a startup flag. The intention is for this code to be temporary, so I didn't want to add
+ * a permanent flag to startup options (and there's already --host_jvm_args, which we can use to
+ * roll this out). The flag affects Skyframe dependencies, so it needs to clear the Skyframe graph -
+ * the easiest way to do that is to restart the server, which is done when --host_jvm_args changes.
+ */
+public class TrackSourceDirectoriesFlag {
+  private static final boolean TRACK_SOURCE_DIRECTORIES;
+
+  static {
+    TRACK_SOURCE_DIRECTORIES = "1".equals(System.getProperty("BAZEL_TRACK_SOURCE_DIRECTORIES"));
+  }
+
+  public static boolean trackSourceDirectories() {
+    return TRACK_SOURCE_DIRECTORIES;
+  }
+
+  // Private constructor to prevent instantiation.
+  private TrackSourceDirectoriesFlag() {
+  }
+}
