From c64421bc35214f0414e4f4226cc953e8c55fa0d2 Mon Sep 17 00:00:00 2001 From: ulfjack Date: Fri, 26 Oct 2018 00:56:32 -0700 Subject: [PATCH] For source directory action inputs, depend on all files in the subtree 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 > $@", ) 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 --- .../build/lib/actions/FileArtifactValue.java | 63 +++++++++++++++++++ .../lib/rules/genrule/GenRuleAction.java | 5 +- .../build/lib/skyframe/ArtifactFunction.java | 53 +++++++++++++++- .../skyframe/TrackSourceDirectoriesFlag.java | 37 +++++++++++ 4 files changed, 154 insertions(+), 4 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/skyframe/TrackSourceDirectoriesFlag.java 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 index 3059e627d0a0e5..71ba6bcde77f52 100644 --- 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 index ad91b7291423fe..d5e70c35cc309b 100644 --- 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 internalExecute(ActionExecutionContext actionExecutionContext) throws ExecException, InterruptedException { EventHandler reporter = actionExecutionContext.getEventHandler(); - checkInputsForDirectories(reporter, actionExecutionContext.getMetadataProvider()); + if (!TrackSourceDirectoriesFlag.trackSourceDirectories()) { + checkInputsForDirectories(reporter, actionExecutionContext.getMetadataProvider()); + } List 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 index 0171573203af61..9689116221c6d7 100644 --- 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 new file mode 100644 index 00000000000000..a4fae03cbb9ace --- /dev/null +++ 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() { + } +}