Respect --noexperimental_check_output_files in FileSystemValueChecker. FileStateValues for output files can make their way into the Skyframe graph if a source file is symlink to an output file.
Also fix a bug where ExternalFilesHelper#isExternalFileSeen would always return true after returning true once in the past. This meant if an external file ever made its way into the Skyframe graph, we would always do a full graph scan at the beginning of each build (iow, we would always waste some CPU time doing nothing interesting).
--
MOS_MIGRATED_REVID=118190190
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java b/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java
index 6126197..d55a029 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ExternalFilesHelper.java
@@ -14,8 +14,11 @@
package com.google.devtools.build.lib.skyframe;
import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible;
+import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
import com.google.devtools.build.lib.rules.repository.RepositoryFunction;
+import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.RootedPath;
@@ -28,9 +31,10 @@
private final AtomicReference<PathPackageLocator> pkgLocator;
private final ExternalFileAction externalFileAction;
- // This variable is set to true from multiple threads, but only read once, in the main thread.
+ // 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 externalFileSeen = false;
+ private boolean anyOutputFilesSeen = false;
+ private boolean anyNonOutputExternalFilesSeen = false;
/**
* @param pkgLocator an {@link AtomicReference} to a {@link PathPackageLocator} used to
@@ -39,32 +43,106 @@
*/
public ExternalFilesHelper(
AtomicReference<PathPackageLocator> pkgLocator, boolean errorOnExternalFiles) {
+ this(
+ pkgLocator,
+ errorOnExternalFiles
+ ? ExternalFileAction.ERROR_OUT
+ : ExternalFileAction.DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_FILES);
+ }
+
+ private ExternalFilesHelper(AtomicReference<PathPackageLocator> pkgLocator,
+ ExternalFileAction externalFileAction) {
this.pkgLocator = pkgLocator;
- if (errorOnExternalFiles) {
- this.externalFileAction = ExternalFileAction.ERROR_OUT;
- } else {
- this.externalFileAction = ExternalFileAction.DEPEND_ON_EXTERNAL_PKG;
- }
+ this.externalFileAction = externalFileAction;
}
private enum ExternalFileAction {
- // Re-check the files when the WORKSPACE file changes.
- DEPEND_ON_EXTERNAL_PKG,
+ /** Re-check the files when the WORKSPACE file changes. */
+ DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_FILES,
- // Throw an exception if there is an external file.
+ /** Throw an exception if there is an external file. */
ERROR_OUT,
}
- boolean isExternalFileSeen() {
- return externalFileSeen;
+ enum FileType {
+ /** A file inside the package roots or in an external repository. */
+ INTERNAL,
+
+ /** A file outside the package roots about which we may make no other assumptions. */
+ EXTERNAL_MUTABLE,
+
+ /**
+ * A file 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 Blaze flags may tell
+ * Blaze to assume these files are immutable.
+ *
+ * Note that {@link ExternalFilesHelper#maybeHandleExternalFile} is only used for
+ * {@link FileStateValue} and {@link DirectoryStateValue}, and also note that output files do
+ * not normally have corresponding {@link FileValue} instances (and thus also
+ * {@link FileStateValue} instances) in the Skyframe graph ({@link ArtifactFunction} only uses
+ * {@link FileValue}s for source files). But {@link FileStateValue}s for output files can still
+ * make their way into the Skyframe graph if e.g. a source file is a symlink to an output file.
+ */
+ // TODO(nharmata): Consider an alternative design where we have an OutputFileDiffAwareness. This
+ // could work but would first require that we clean up all RootedPath usage.
+ OUTPUT,
+
+ /**
+ * A file in the part of Bazel's output tree that contains (/ symlinks to) to external
+ * repositories.
+ */
+ EXTERNAL_REPO,
}
- static boolean isInternal(RootedPath rootedPath, PathPackageLocator packageLocator) {
- // TODO(bazel-team): This is inefficient when there are a lot of package roots or there are a
- // lot of external directories. Consider either explicitly preventing this case or using a more
- // efficient approach here (e.g. use a trie for determining if a file is under an external
- // directory).
- return packageLocator.getPathEntries().contains(rootedPath.getRoot());
+ static class ExternalFilesKnowledge {
+ final boolean anyOutputFilesSeen;
+ final boolean anyNonOutputExternalFilesSeen;
+
+ private ExternalFilesKnowledge(boolean anyOutputFilesSeen,
+ boolean anyNonOutputExternalFilesSeen) {
+ this.anyOutputFilesSeen = anyOutputFilesSeen;
+ this.anyNonOutputExternalFilesSeen = anyNonOutputExternalFilesSeen;
+ }
+ }
+
+ @ThreadCompatible
+ ExternalFilesKnowledge getExternalFilesKnowledge() {
+ return new ExternalFilesKnowledge(anyOutputFilesSeen, anyNonOutputExternalFilesSeen);
+ }
+
+ @ThreadCompatible
+ void setExternalFilesKnowledge(ExternalFilesKnowledge externalFilesKnowledge) {
+ anyOutputFilesSeen = externalFilesKnowledge.anyOutputFilesSeen;
+ anyNonOutputExternalFilesSeen = externalFilesKnowledge.anyNonOutputExternalFilesSeen;
+ }
+
+ ExternalFilesHelper cloneWithFreshExternalFilesKnowledge() {
+ return new ExternalFilesHelper(pkgLocator, externalFileAction);
+ }
+
+ FileType getAndNoteFileType(RootedPath rootedPath) {
+ PathPackageLocator packageLocator = pkgLocator.get();
+ if (packageLocator.getPathEntries().contains(rootedPath.getRoot())) {
+ return FileType.INTERNAL;
+ }
+ // The outputBase may be null if we're not actually running a build.
+ Path outputBase = packageLocator.getOutputBase();
+ if (outputBase == null) {
+ anyNonOutputExternalFilesSeen = true;
+ return FileType.EXTERNAL_MUTABLE;
+ }
+ if (rootedPath.asPath().startsWith(outputBase)) {
+ Path externalRepoDir = outputBase.getRelative(Label.EXTERNAL_PATH_PREFIX);
+ if (rootedPath.asPath().startsWith(externalRepoDir)) {
+ anyNonOutputExternalFilesSeen = true;
+ return FileType.EXTERNAL_REPO;
+ } else {
+ anyOutputFilesSeen = true;
+ return FileType.OUTPUT;
+ }
+ }
+ anyNonOutputExternalFilesSeen = true;
+ return FileType.EXTERNAL_MUTABLE;
}
/**
@@ -72,26 +150,22 @@
* under a package root then this adds a dependency on the //external package. If the action is
* ERROR_OUT, it will throw an error instead.
*/
+ @ThreadSafe
public void maybeHandleExternalFile(RootedPath rootedPath, SkyFunction.Environment env)
throws FileOutsidePackageRootsException {
- if (isInternal(rootedPath, pkgLocator.get())) {
+ FileType fileType = getAndNoteFileType(rootedPath);
+ if (fileType == FileType.INTERNAL) {
return;
}
-
- externalFileSeen = true;
- if (externalFileAction == ExternalFileAction.ERROR_OUT) {
- throw new FileOutsidePackageRootsException(rootedPath);
- }
-
- // The outputBase may be null if we're not actually running a build.
- Path outputBase = pkgLocator.get().getOutputBase();
- if (outputBase == null) {
+ if (fileType == FileType.OUTPUT || fileType == FileType.EXTERNAL_MUTABLE) {
+ if (externalFileAction == ExternalFileAction.ERROR_OUT) {
+ throw new FileOutsidePackageRootsException(rootedPath);
+ }
return;
}
- Path relativeExternal = outputBase.getRelative(Label.EXTERNAL_PATH_PREFIX);
- if (!rootedPath.asPath().startsWith(relativeExternal)) {
- return;
- }
+ Preconditions.checkState(
+ externalFileAction == ExternalFileAction.DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_FILES,
+ externalFileAction);
// For files that are under $OUTPUT_BASE/external, add a dependency on the corresponding rule
// so that if the WORKSPACE file changes, the File/DirectoryStateValue will be re-evaluated.
@@ -105,7 +179,8 @@
// neither want to encourage nor optimize for since it is not common. So the set of external
// files is small.
- PathFragment repositoryPath = rootedPath.asPath().relativeTo(relativeExternal);
+ Path externalRepoDir = pkgLocator.get().getOutputBase().getRelative(Label.EXTERNAL_PATH_PREFIX);
+ PathFragment repositoryPath = rootedPath.asPath().relativeTo(externalRepoDir);
if (repositoryPath.segmentCount() == 0) {
// We are the top of the repository path (<outputBase>/external), not in an actual external
// repository path.
