Make files and directories under managed directories be correctly pro…

…cessed 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

src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java

@@ -51,6 +51,7 @@
 import com.google.devtools.build.lib.rules.repository.LocalRepositoryFunction;
 import com.google.devtools.build.lib.rules.repository.LocalRepositoryRule;
 import com.google.devtools.build.lib.rules.repository.ManagedDirectoriesKnowledgeImpl;
+import com.google.devtools.build.lib.rules.repository.ManagedDirectoriesKnowledgeImpl.ManagedDirectoriesListener;
 import com.google.devtools.build.lib.rules.repository.NewLocalRepositoryFunction;
 import com.google.devtools.build.lib.rules.repository.NewLocalRepositoryRule;
 import com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction;
@@ -70,6 +71,7 @@
 import com.google.devtools.build.lib.skyframe.SkyFunctions;
 import com.google.devtools.build.lib.skylarkbuildapi.repository.RepositoryBootstrap;
 import com.google.devtools.build.lib.util.AbruptExitException;
+import com.google.devtools.build.lib.util.ExitCode;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
@@ -87,7 +89,6 @@
 
 /** Adds support for fetching external code. */
 public class BazelRepositoryModule extends BlazeModule {
-
   // Default location (relative to output user root) of the repository cache.
   public static final String DEFAULT_CACHE_LOCATION = "cache/repos/v1";
 
@@ -107,12 +108,28 @@ public class BazelRepositoryModule extends BlazeModule {
   private FileSystem filesystem;
   // We hold the precomputed value of the managed directories here, so that the dependency
   // on WorkspaceFileValue is not registered for each FileStateValue.
-  private final ManagedDirectoriesKnowledgeImpl managedDirectoriesKnowledge =
-      new ManagedDirectoriesKnowledgeImpl();
+  private final ManagedDirectoriesKnowledgeImpl managedDirectoriesKnowledge;
 
   public BazelRepositoryModule() {
     this.skylarkRepositoryFunction = new SkylarkRepositoryFunction(httpDownloader);
     this.repositoryHandlers = repositoryRules(httpDownloader, mavenDownloader);
+    ManagedDirectoriesListener listener =
+        repositoryNamesWithManagedDirs -> {
+          Set<String> conflicting =
+              overrides.keySet().stream()
+                  .filter(repositoryNamesWithManagedDirs::contains)
+                  .map(RepositoryName::getName)
+                  .collect(Collectors.toSet());
+          if (!conflicting.isEmpty()) {
+            String message =
+                "Overriding repositories is not allowed"
+                    + " for the repositories with managed directories.\n"
+                    + "The following overridden external repositories have managed directories: "
+                    + String.join(", ", conflicting.toArray(new String[0]));
+            throw new AbruptExitException(message, ExitCode.COMMAND_LINE_ERROR);
+          }
+        };
+    managedDirectoriesKnowledge = new ManagedDirectoriesKnowledgeImpl(listener);
   }
 
   public static ImmutableMap<String, RepositoryFunction> repositoryRules(
@@ -153,10 +170,7 @@ public void serverInit(OptionsParsingResult startupOptions, ServerBuilder builde
   @Override
   public void workspaceInit(
       BlazeRuntime runtime, BlazeDirectories directories, WorkspaceBuilder builder) {
-    builder.setWorkspaceFileHeaderListener(
-        value ->
-            managedDirectoriesKnowledge.setManagedDirectories(
-                value != null ? value.getManagedDirectories() : ImmutableMap.of()));
+    builder.setManagedDirectoriesKnowledge(managedDirectoriesKnowledge);
 
     RepositoryDirectoryDirtinessChecker customDirtinessChecker =
         new RepositoryDirectoryDirtinessChecker(managedDirectoriesKnowledge);

src/main/java/com/google/devtools/build/lib/rules/repository/ManagedDirectoriesKnowledge.java

@@ -16,8 +16,9 @@
 
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
+import com.google.devtools.build.lib.packages.WorkspaceFileValue;
+import com.google.devtools.build.lib.skyframe.SequencedSkyframeExecutor.WorkspaceFileHeaderListener;
 import com.google.devtools.build.lib.vfs.PathFragment;
-import com.google.devtools.build.lib.vfs.RootedPath;
 import javax.annotation.Nullable;
 
 /**
@@ -29,12 +30,18 @@
  * <p>Having managed directories as a separate component (and not SkyValue) allows to skip recording
  * the dependency in Skyframe for each FileStateValue and DirectoryListingStateValue.
  */
-public interface ManagedDirectoriesKnowledge {
+public interface ManagedDirectoriesKnowledge extends WorkspaceFileHeaderListener {
   ManagedDirectoriesKnowledge NO_MANAGED_DIRECTORIES =
       new ManagedDirectoriesKnowledge() {
+        @Override
+        public boolean workspaceHeaderReloaded(
+            @Nullable WorkspaceFileValue oldValue, @Nullable WorkspaceFileValue newValue) {
+          return false;
+        }
+
         @Nullable
         @Override
-        public RepositoryName getOwnerRepository(RootedPath rootedPath, boolean old) {
+        public RepositoryName getOwnerRepository(PathFragment relativePathFragment) {
           return null;
         }
 
@@ -44,8 +51,15 @@ public ImmutableSet<PathFragment> getManagedDirectories(RepositoryName repositor
         }
       };
 
+  /**
+   * Returns the owning repository for the incrementally updated path, or null.
+   *
+   * @param relativePathFragment path to check, relative to workspace root
+   * @return RepositoryName or null if there is no owning repository
+   */
   @Nullable
-  RepositoryName getOwnerRepository(RootedPath rootedPath, boolean old);
+  RepositoryName getOwnerRepository(PathFragment relativePathFragment);
 
+  /** Returns managed directories for the passed repository. */
   ImmutableSet<PathFragment> getManagedDirectories(RepositoryName repositoryName);
 }

src/main/java/com/google/devtools/build/lib/rules/repository/ManagedDirectoriesKnowledgeImpl.java

@@ -14,88 +14,91 @@
 
 package com.google.devtools.build.lib.rules.repository;
 
-import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedMap;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
+import com.google.devtools.build.lib.packages.WorkspaceFileValue;
+import com.google.devtools.build.lib.util.AbruptExitException;
 import com.google.devtools.build.lib.vfs.PathFragment;
-import com.google.devtools.build.lib.vfs.RootedPath;
 import java.util.Comparator;
 import java.util.Map;
-import java.util.NavigableMap;
+import java.util.Objects;
 import java.util.Set;
-import java.util.concurrent.atomic.AtomicReference;
 import javax.annotation.Nullable;
 
 /** Managed directories component. {@link ManagedDirectoriesKnowledge} */
 public class ManagedDirectoriesKnowledgeImpl implements ManagedDirectoriesKnowledge {
-  private final AtomicReference<ImmutableSortedMap<PathFragment, RepositoryName>>
-      managedDirectoriesRef = new AtomicReference<>(ImmutableSortedMap.of());
-  private final AtomicReference<Map<RepositoryName, ImmutableSet<PathFragment>>> repoToDirMapRef =
-      new AtomicReference<>(ImmutableMap.of());
+  private final ManagedDirectoriesListener listener;
 
-  /**
-   * During build commands execution, Skyframe caches the states of files (FileStateValue) and
-   * directory listings (DirectoryListingStateValue). In the case when the files/directories are
-   * under a managed directory or inside an external repository, evaluation of file/directory
-   * listing states requires that the RepositoryDirectoryValue of the owning external repository is
-   * evaluated beforehand. (So that the repository rule generates the files.) So there is a
-   * dependency on RepositoryDirectoryValue for files under managed directories and external
-   * repositories. This dependency is recorded by Skyframe,
-   *
-   * <p>From the other side, by default Skyframe injects the new values of changed files already at
-   * the stage of checking what files have changed. Only the values without any dependencies can be
-   * injected into Skyframe. Skyframe can be specifically instructed to not inject new values and
-   * only register them as changed.
-   *
-   * <p>When the values of managed directories change, some files appear to become files under
-   * managed directories, or they are no longer files under managed directories. This implies that
-   * corresponding file/directory listing states gain the dependency (RepositoryDirectoryValue) or
-   * they lose this dependency. In both cases, we should prevent Skyframe from injecting those new
-   * values of file/directory listing states at the stage of checking changed files.
-   *
-   * <p>That is why we need to keep track of the previous value of the managed directories.
-   */
-  private final AtomicReference<ImmutableSortedMap<PathFragment, RepositoryName>>
-      oldManagedDirectoriesRef = new AtomicReference<>(ImmutableSortedMap.of());
+  private ImmutableSortedMap<PathFragment, RepositoryName> dirToRepoMap = ImmutableSortedMap.of();
+  private ImmutableSortedMap<RepositoryName, ImmutableSet<PathFragment>> repoToDirMap =
+      ImmutableSortedMap.of();
+
+  public ManagedDirectoriesKnowledgeImpl(ManagedDirectoriesListener listener) {
+    this.listener = listener;
+  }
 
   @Override
   @Nullable
-  public RepositoryName getOwnerRepository(RootedPath rootedPath, boolean old) {
-    PathFragment relativePath = rootedPath.getRootRelativePath();
-    NavigableMap<PathFragment, RepositoryName> map =
-        old ? oldManagedDirectoriesRef.get() : managedDirectoriesRef.get();
-    Map.Entry<PathFragment, RepositoryName> entry = map.floorEntry(relativePath);
-    if (entry != null && relativePath.startsWith(entry.getKey())) {
+  public RepositoryName getOwnerRepository(PathFragment relativePathFragment) {
+    Map.Entry<PathFragment, RepositoryName> entry = dirToRepoMap.floorEntry(relativePathFragment);
+    if (entry != null && relativePathFragment.startsWith(entry.getKey())) {
       return entry.getValue();
     }
     return null;
   }
 
   @Override
   public ImmutableSet<PathFragment> getManagedDirectories(RepositoryName repositoryName) {
-    ImmutableSet<PathFragment> pathFragments = repoToDirMapRef.get().get(repositoryName);
+    ImmutableSet<PathFragment> pathFragments = repoToDirMap.get(repositoryName);
     return pathFragments != null ? pathFragments : ImmutableSet.of();
   }
 
-  public void setManagedDirectories(ImmutableMap<PathFragment, RepositoryName> map) {
-    oldManagedDirectoriesRef.set(managedDirectoriesRef.get());
-    ImmutableSortedMap<PathFragment, RepositoryName> pathsMap = ImmutableSortedMap.copyOf(map);
-    managedDirectoriesRef.set(pathsMap);
+  @Override
+  public boolean workspaceHeaderReloaded(
+      @Nullable WorkspaceFileValue oldValue, @Nullable WorkspaceFileValue newValue)
+      throws AbruptExitException {
+    if (Objects.equals(oldValue, newValue)) {
+      listener.onManagedDirectoriesRefreshed(repoToDirMap.keySet());
+      return false;
+    }
+    Map<PathFragment, RepositoryName> oldDirToRepoMap = dirToRepoMap;
+    refreshMappings(newValue);
+    if (!Objects.equals(oldDirToRepoMap, dirToRepoMap)) {
+      listener.onManagedDirectoriesRefreshed(repoToDirMap.keySet());
+      return true;
+    }
+    return false;
+  }
+
+  private void refreshMappings(@Nullable WorkspaceFileValue newValue) {
+    if (newValue == null) {
+      dirToRepoMap = ImmutableSortedMap.of();
+      repoToDirMap = ImmutableSortedMap.of();
+      return;
+    }
+
+    dirToRepoMap = ImmutableSortedMap.copyOf(newValue.getManagedDirectories());
 
     Map<RepositoryName, Set<PathFragment>> reposMap = Maps.newHashMap();
-    for (Map.Entry<PathFragment, RepositoryName> entry : pathsMap.entrySet()) {
+    for (Map.Entry<PathFragment, RepositoryName> entry : dirToRepoMap.entrySet()) {
       RepositoryName repositoryName = entry.getValue();
       reposMap.computeIfAbsent(repositoryName, name -> Sets.newTreeSet()).add(entry.getKey());
     }
 
-    ImmutableSortedMap.Builder<RepositoryName, ImmutableSet<PathFragment>> builder =
+    ImmutableSortedMap.Builder<RepositoryName, ImmutableSet<PathFragment>> reposMapBuilder =
         new ImmutableSortedMap.Builder<>(Comparator.comparing(RepositoryName::getName));
     for (Map.Entry<RepositoryName, Set<PathFragment>> entry : reposMap.entrySet()) {
-      builder.put(entry.getKey(), ImmutableSet.copyOf(entry.getValue()));
+      reposMapBuilder.put(entry.getKey(), ImmutableSet.copyOf(entry.getValue()));
     }
-    repoToDirMapRef.set(builder.build());
+    repoToDirMap = reposMapBuilder.build();
+  }
+
+  /** Interface allows {@link BazelRepositoryModule} to react to managed directories refreshes. */
+  public interface ManagedDirectoriesListener {
+    void onManagedDirectoriesRefreshed(Set<RepositoryName> repositoryNames)
+        throws AbruptExitException;
   }
 }

src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java

@@ -99,10 +99,10 @@ public abstract class RepositoryFunction {
    * {@link RepositoryDelegatorFunction} has to know how to catch.</p>
    */
   public static class RepositoryFunctionException extends SkyFunctionException {
+
     public RepositoryFunctionException(NoSuchPackageException cause, Transience transience) {
       super(cause, transience);
     }
-
     /**
      * Error reading or writing to the filesystem.
      */
@@ -117,11 +117,11 @@ public RepositoryFunctionException(EvalException cause, Transience transience) {
       super(cause, transience);
     }
   }
-
   /**
    * Exception thrown when something a repository rule cannot be found.
    */
   public static final class RepositoryNotFoundException extends RepositoryFunctionException {
+
     public RepositoryNotFoundException(String repositoryName) {
       super(
           new BuildFileContainsErrorsException(
@@ -130,7 +130,6 @@ public RepositoryNotFoundException(String repositoryName) {
           Transience.PERSISTENT);
     }
   }
-
   /**
    * An exception thrown when a dependency is missing to notify the SkyFunction from an evaluation.
    */
@@ -140,7 +139,6 @@ protected static class RepositoryMissingDependencyException extends EvalExceptio
       super(Location.BUILTIN, "Internal exception");
     }
   }
-
   /**
    * repository functions can throw the result of this function to notify the RepositoryFunction
    * that a dependency was missing and the evaluation of the function must be restarted.
@@ -528,7 +526,7 @@ protected static Path getExternalRepositoryDirectory(BlazeDirectories directorie
    */
   public static void addExternalFilesDependencies(
       RootedPath rootedPath, boolean isDirectory, BlazeDirectories directories, Environment env)
-      throws IOException, InterruptedException {
+      throws InterruptedException {
     Path externalRepoDir = getExternalRepositoryDirectory(directories);
     PathFragment repositoryPath = rootedPath.asPath().relativeTo(externalRepoDir);
     if (repositoryPath.isEmpty()) {
@@ -579,14 +577,29 @@ public static void addExternalFilesDependencies(
       // Alternatively, the repository might still be provided by an override. Therefore, in
       // any case, register the dependency on the repository overrides.
       RepositoryDelegatorFunction.REPOSITORY_OVERRIDES.get(env);
-      return;
     } catch (ExternalPackageException ex) {
       // This should never happen.
       throw new IllegalStateException(
           "Repository " + repositoryName + " cannot be resolved for path " + rootedPath, ex);
     }
   }
 
+  /**
+   * For paths that are under managed directories, we require that the corresponding FileStateValue
+   * or DirectoryListingStateValue is evaluated only after RepositoryDirectoryValue is evaluated.
+   * This way we guarantee that the repository rule is given a chance to update the managed
+   * directory before the files under the managed directory are accessed.
+   *
+   * <p>We do not need to require anything else (comparing to dependencies required for external
+   * repositories files), as overriding external repositories with managed directories is currently
+   * forbidden; also, we do not have do perform special checks for local_repository targets, since
+   * such targets cannot have managed directories by definition.
+   */
+  public static void addManagedDirectoryDependencies(RepositoryName repositoryName, Environment env)
+      throws InterruptedException {
+    env.getValue(RepositoryDirectoryValue.key(repositoryName));
+  }
+
   /**
    * Sets up a mapping of environment variables to use.
    */

src/main/java/com/google/devtools/build/lib/runtime/WorkspaceBuilder.java

@@ -23,8 +23,8 @@
 import com.google.devtools.build.lib.exec.BinTools;
 import com.google.devtools.build.lib.packages.PackageFactory;
 import com.google.devtools.build.lib.profiler.memory.AllocationTracker;
+import com.google.devtools.build.lib.rules.repository.ManagedDirectoriesKnowledge;
 import com.google.devtools.build.lib.skyframe.DiffAwareness;
-import com.google.devtools.build.lib.skyframe.SequencedSkyframeExecutor.WorkspaceFileHeaderListener;
 import com.google.devtools.build.lib.skyframe.SequencedSkyframeExecutorFactory;
 import com.google.devtools.build.lib.skyframe.SkyValueDirtinessChecker;
 import com.google.devtools.build.lib.skyframe.SkyframeExecutor;
@@ -53,7 +53,7 @@ public final class WorkspaceBuilder {
   private final ImmutableList.Builder<SkyValueDirtinessChecker> customDirtinessCheckers =
       ImmutableList.builder();
   private AllocationTracker allocationTracker;
-  private WorkspaceFileHeaderListener workspaceFileHeaderListener;
+  private ManagedDirectoriesKnowledge managedDirectoriesKnowledge;
 
   WorkspaceBuilder(BlazeDirectories directories, BinTools binTools) {
     this.directories = directories;
@@ -82,7 +82,7 @@ BlazeWorkspace build(
             diffAwarenessFactories.build(),
             skyFunctions.build(),
             customDirtinessCheckers.build(),
-            workspaceFileHeaderListener);
+            managedDirectoriesKnowledge);
     return new BlazeWorkspace(
         runtime,
         directories,
@@ -157,9 +157,9 @@ public WorkspaceBuilder addCustomDirtinessChecker(
     return this;
   }
 
-  public WorkspaceBuilder setWorkspaceFileHeaderListener(
-      WorkspaceFileHeaderListener workspaceFileHeaderListener) {
-    this.workspaceFileHeaderListener = workspaceFileHeaderListener;
+  public WorkspaceBuilder setManagedDirectoriesKnowledge(
+      ManagedDirectoriesKnowledge managedDirectoriesKnowledge) {
+    this.managedDirectoriesKnowledge = managedDirectoriesKnowledge;
     return this;
   }
 }