Add dependency for external files to the corresponding RepositoryDirectoryValue

Previously we were adding a dependency to the rule itself, however the repository
itself can depends on further dependencies (environment, template files, ...) with
Skylark repositories. Not invalidating when those change was causing weird invalidation
issues that were shown with the invalidation on environment (basically a refetch of
the repository on environment invalidation would not cause an invalidation of the
corresponding package, the next call would not have invalidation of the repository
but would see the change in the build files...).

--
Change-Id: I8945d9885e6390734ba02ccc6c3c6ca639fcec35
Reviewed-on: https://cr.bazel.build/8137
PiperOrigin-RevId: 145675258
MOS_MIGRATED_REVID=145675258

1 files changed, 11 insertions, 33 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java
index bf02eb11ff..354d30f4c3 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java
@@ -26,7 +26,6 @@ import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.Rule;
-import com.google.devtools.build.lib.skyframe.DirectoryListingValue;
 import com.google.devtools.build.lib.skyframe.FileSymlinkException;
 import com.google.devtools.build.lib.skyframe.FileValue;
 import com.google.devtools.build.lib.skyframe.InconsistentFilesystemException;
@@ -35,7 +34,6 @@ import com.google.devtools.build.lib.skyframe.WorkspaceFileValue;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.util.Preconditions;
-import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -402,48 +400,28 @@ public abstract class RepositoryFunction {
     }
     String repositoryName = repositoryPath.getSegment(0);
 
-    Rule repositoryRule;
     try {
-      repositoryRule = RepositoryFunction.getRule(repositoryName, env);
+      // Add a dependency to the repository rule. RepositoryDirectoryValue does add this dependency
+      // already but we want to catch RepositoryNotFoundException, so invoke #getRule first.
+      RepositoryFunction.getRule(repositoryName, env);
+      if (repositoryPath.segmentCount() > 1) {
+        // For all file under the repository directory, depends on the actual RepositoryDirectory
+        // function so we get invalidation when the repository is fetched.
+        // For the repository directory itself, we cannot depends on the RepositoryDirectoryValue
+        // (cycle).
+        env.getValue(RepositoryDirectoryValue.key(RepositoryName.create("@" + repositoryName)));
+      }
     } catch (RepositoryFunction.RepositoryNotFoundException ex) {
       // The repository we are looking for does not exist so we should depend on the whole
       // WORKSPACE file. In that case, the call to RepositoryFunction#getRule(String, Environment)
       // already requested all repository functions from the WORKSPACE file from Skyframe as part
       // of the resolution. Therefore we are safe to ignore that Exception.
       return;
-    } catch (RepositoryFunction.RepositoryFunctionException ex) {
+    } catch (RepositoryFunctionException | LabelSyntaxException ex) {
       // This should never happen.
       throw new IllegalStateException(
           "Repository " + repositoryName + " cannot be resolved for path " + rootedPath, ex);
     }
-    if (repositoryRule == null) {
-      return;
-    }
-
-    // new_local_repository needs a dependency on the directory that `path` points to, as the
-    // external/repo-name DirStateValue has a logical dependency on that directory that is not
-    // reflected in the SkyFrame tree, since it's not symlinked to it or anything.
-    // new_local_repository is responsible for verifying that the path exists and is a directory.
-    if (repositoryRule.getRuleClass().equals(NewLocalRepositoryRule.NAME)
-        && repositoryPath.segmentCount() == 1) {
-      PathFragment pathDir;
-      try {
-        pathDir = RepositoryFunction.getTargetPath(
-            repositoryRule, directories.getWorkspace());
-      } catch (RepositoryFunctionException e) {
-        throw new IOException(e.getMessage());
-      }
-      FileSystem fs = directories.getWorkspace().getFileSystem();
-      SkyKey dirKey = DirectoryListingValue.key(
-          RootedPath.toRootedPath(fs.getRootDirectory(), fs.getPath(pathDir)));
-      try {
-        env.getValueOrThrow(
-            dirKey, IOException.class, FileSymlinkException.class,
-            InconsistentFilesystemException.class);
-      } catch (FileSymlinkException | InconsistentFilesystemException e) {
-        throw new IOException(e.getMessage());
-      }
-    }
   }
 
   /**