Make files and directories under managed directories be correctly processed by SequencedSkyframeExecutor.
TL;DR - two main changes: how to invalidate changed managed directories files, and how to force owning external repository to be evaluated before managed directories files.
- In ExternalFilesHelper, introduce one more file type - EXTERNAL_REPO_IN_USER_DIRECTORY, for the files under managed directories.
- For files under managed directories, require owning RepositoryDirectoryValue to be evaluated first.
- For correct dirtying of files under managed directories, both with watchfs flag and without, the following should be taken into account: not only that new values for external repositories and managed directories files can not be injected at the stage of dirtying, but also files that used to be under managed directories on the previous evaluator invocation.
The latter are still cached in evaluator, and the fact that they used to depend on their RepositoryDirectory values would prevent injection of the new values for them.
To meet those conditions, in SequencedSkyframeExecutor.handleChangedFiles() filtering of the going-to-be-injected files is added.
(The change can not be done inside ExternalDirtinessChecker only, as then it does not affect watchfs=true case).
- ManagedDirectoriesBlackBoxTest added to demonstrate and validate the functionality of managed directories.
PiperOrigin-RevId: 246496823
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 abc7577..1fcbc4e 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,12 +14,17 @@
package com.google.devtools.build.lib.skyframe;
import com.google.common.base.Preconditions;
+import com.google.devtools.build.lib.actions.FileStateValue;
+import com.google.devtools.build.lib.actions.FileValue;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.cmdline.LabelConstants;
+import com.google.devtools.build.lib.cmdline.RepositoryName;
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.ManagedDirectoriesKnowledge;
import com.google.devtools.build.lib.rules.repository.RepositoryFunction;
+import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.build.skyframe.SkyFunction;
@@ -45,40 +50,59 @@
private boolean anyOutputFilesSeen = false;
private boolean anyNonOutputExternalFilesSeen = false;
+ private final ManagedDirectoriesKnowledge managedDirectoriesKnowledge;
+
private ExternalFilesHelper(
AtomicReference<PathPackageLocator> pkgLocator,
ExternalFileAction externalFileAction,
BlazeDirectories directories,
- int maxNumExternalFilesToLog) {
+ int maxNumExternalFilesToLog,
+ ManagedDirectoriesKnowledge managedDirectoriesKnowledge) {
this.pkgLocator = pkgLocator;
this.externalFileAction = externalFileAction;
this.directories = directories;
this.maxNumExternalFilesToLog = maxNumExternalFilesToLog;
+ this.managedDirectoriesKnowledge = managedDirectoriesKnowledge;
}
public static ExternalFilesHelper create(
AtomicReference<PathPackageLocator> pkgLocator,
ExternalFileAction externalFileAction,
- BlazeDirectories directories) {
+ BlazeDirectories directories,
+ ManagedDirectoriesKnowledge managedDirectoriesKnowledge) {
return IN_TEST
- ? createForTesting(pkgLocator, externalFileAction, directories)
+ ? createForTesting(pkgLocator, externalFileAction, directories, managedDirectoriesKnowledge)
: new ExternalFilesHelper(
pkgLocator,
externalFileAction,
directories,
- /*maxNumExternalFilesToLog=*/ 100);
+ /*maxNumExternalFilesToLog=*/ 100,
+ managedDirectoriesKnowledge);
}
public static ExternalFilesHelper createForTesting(
AtomicReference<PathPackageLocator> pkgLocator,
ExternalFileAction externalFileAction,
BlazeDirectories directories) {
+ return createForTesting(
+ pkgLocator,
+ externalFileAction,
+ directories,
+ ManagedDirectoriesKnowledge.NO_MANAGED_DIRECTORIES);
+ }
+
+ private static ExternalFilesHelper createForTesting(
+ AtomicReference<PathPackageLocator> pkgLocator,
+ ExternalFileAction externalFileAction,
+ BlazeDirectories directories,
+ ManagedDirectoriesKnowledge managedDirectoriesKnowledge) {
return new ExternalFilesHelper(
pkgLocator,
externalFileAction,
directories,
// These log lines are mostly spam during unit and integration tests.
- /*maxNumExternalFilesToLog=*/ 0);
+ /*maxNumExternalFilesToLog=*/ 0,
+ managedDirectoriesKnowledge);
}
@@ -105,12 +129,12 @@
/** Classification of a path encountered by Bazel. */
public enum FileType {
- /** A path inside the package roots or in an external repository. */
+ /** 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.
+ * A non {@link #EXTERNAL_REPO} or {@link #EXTERNAL_IN_MANAGED_DIRECTORY} path outside the
+ * package roots about which we may make no other assumptions.
*/
EXTERNAL,
@@ -132,9 +156,22 @@
/**
* A path in the part of Bazel's output tree that contains (/ symlinks to) to external
- * repositories.
+ * repositories. Every such path under the external repository is generated by the execution of
+ * the corresponding repository rule, so these paths should not be cached by Skyframe before the
+ * RepositoryDirectoryValue is computed.
*/
EXTERNAL_REPO,
+
+ /**
+ * A path is under one of the managed directories. Managed directories are user-owned
+ * directories, which can be incrementally updated by repository rules, so that the updated
+ * files are visible for the actions in the same build.
+ *
+ * <p>Every such path under the managed directory is generated or updated by the execution of
+ * the corresponding repository rule, so these paths should not be cached by Skyframe before the
+ * RepositoryDirectoryValue is computed. {@link ManagedDirectoriesKnowledge}
+ */
+ EXTERNAL_IN_MANAGED_DIRECTORY,
}
/**
@@ -168,10 +205,35 @@
ExternalFilesHelper cloneWithFreshExternalFilesKnowledge() {
return new ExternalFilesHelper(
- pkgLocator, externalFileAction, directories, maxNumExternalFilesToLog);
+ pkgLocator,
+ externalFileAction,
+ directories,
+ maxNumExternalFilesToLog,
+ managedDirectoriesKnowledge);
}
- FileType getAndNoteFileType(RootedPath rootedPath) {
+ public FileType getAndNoteFileType(RootedPath rootedPath) {
+ return getFileTypeAndRepository(rootedPath).getFirst();
+ }
+
+ private Pair<FileType, RepositoryName> getFileTypeAndRepository(RootedPath rootedPath) {
+ RepositoryName repositoryName =
+ managedDirectoriesKnowledge.getOwnerRepository(rootedPath.getRootRelativePath());
+ if (repositoryName != null) {
+ anyNonOutputExternalFilesSeen = true;
+ return Pair.of(FileType.EXTERNAL_IN_MANAGED_DIRECTORY, repositoryName);
+ }
+ FileType fileType = detectFileType(rootedPath);
+ if (FileType.EXTERNAL == fileType || FileType.EXTERNAL_REPO == fileType) {
+ anyNonOutputExternalFilesSeen = true;
+ }
+ if (FileType.OUTPUT == fileType) {
+ anyOutputFilesSeen = true;
+ }
+ return Pair.of(fileType, null);
+ }
+
+ private FileType detectFileType(RootedPath rootedPath) {
PathPackageLocator packageLocator = pkgLocator.get();
if (packageLocator.getPathEntries().contains(rootedPath.getRoot())) {
return FileType.INTERNAL;
@@ -204,27 +266,39 @@
* a {@link NonexistentImmutableExternalFileException} instead.
*/
@ThreadSafe
- void maybeHandleExternalFile(
+ FileType maybeHandleExternalFile(
RootedPath rootedPath, boolean isDirectory, SkyFunction.Environment env)
throws NonexistentImmutableExternalFileException, IOException, InterruptedException {
- FileType fileType = getAndNoteFileType(rootedPath);
- if (fileType == FileType.INTERNAL) {
- return;
+ Pair<FileType, RepositoryName> pair = getFileTypeAndRepository(rootedPath);
+
+ FileType fileType = Preconditions.checkNotNull(pair.getFirst());
+ switch (fileType) {
+ case EXTERNAL_IN_MANAGED_DIRECTORY:
+ Preconditions.checkState(
+ externalFileAction == ExternalFileAction.DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_PATHS,
+ externalFileAction);
+ RepositoryFunction.addManagedDirectoryDependencies(pair.getSecond(), env);
+ break;
+ case INTERNAL:
+ break;
+ case EXTERNAL:
+ if (numExternalFilesLogged.incrementAndGet() < maxNumExternalFilesToLog) {
+ logger.info("Encountered an external path " + rootedPath);
+ }
+ // fall through
+ case OUTPUT:
+ if (externalFileAction
+ == ExternalFileAction.ASSUME_NON_EXISTENT_AND_IMMUTABLE_FOR_EXTERNAL_PATHS) {
+ throw new NonexistentImmutableExternalFileException();
+ }
+ break;
+ case EXTERNAL_REPO:
+ Preconditions.checkState(
+ externalFileAction == ExternalFileAction.DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_PATHS,
+ externalFileAction);
+ RepositoryFunction.addExternalFilesDependencies(rootedPath, isDirectory, directories, env);
+ break;
}
- if (fileType == FileType.OUTPUT || fileType == FileType.EXTERNAL) {
- if (externalFileAction
- == ExternalFileAction.ASSUME_NON_EXISTENT_AND_IMMUTABLE_FOR_EXTERNAL_PATHS) {
- throw new NonexistentImmutableExternalFileException();
- }
- if (fileType == FileType.EXTERNAL
- && numExternalFilesLogged.incrementAndGet() < maxNumExternalFilesToLog) {
- logger.info("Encountered an external path " + rootedPath);
- }
- return;
- }
- Preconditions.checkState(
- externalFileAction == ExternalFileAction.DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_PATHS,
- externalFileAction);
- RepositoryFunction.addExternalFilesDependencies(rootedPath, isDirectory, directories, env);
+ return fileType;
}
}
