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() { + } +}