Check for additions to the directory in new_local_repository

Fixes #806.

RELNOTES: External repository correctness fix: adding a new file/directory as a
child of a new_local_repository is now noticed.

--
MOS_MIGRATED_REVID=120557511

6 files changed, 132 insertions, 51 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryFunction.java
index be7826cd28..66942f390d 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryFunction.java
@@ -17,10 +17,15 @@ package com.google.devtools.build.lib.rules.repository;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.RuleDefinition;
 import com.google.devtools.build.lib.packages.Rule;
+import com.google.devtools.build.lib.skyframe.DirectoryListingValue;
+import com.google.devtools.build.lib.skyframe.InconsistentFilesystemException;
+import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.RootedPath;
 import com.google.devtools.build.skyframe.SkyFunction.Environment;
 import com.google.devtools.build.skyframe.SkyFunctionException;
+import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 
 import java.io.IOException;
@@ -38,7 +43,7 @@ public class NewLocalRepositoryFunction extends RepositoryFunction {
   @Override
   public SkyValue fetch(
       Rule rule, Path outputDirectory, BlazeDirectories directories, Environment env)
-          throws SkyFunctionException {
+      throws SkyFunctionException {
 
     NewRepositoryBuildFileHandler buildFileHandler =
         new NewRepositoryBuildFileHandler(directories.getWorkspace());
@@ -48,9 +53,29 @@ public class NewLocalRepositoryFunction extends RepositoryFunction {
 
     PathFragment pathFragment = getTargetPath(rule, directories.getWorkspace());
 
+    FileSystem fs = directories.getOutputBase().getFileSystem();
+    Path path = fs.getPath(pathFragment);
+
+    // fetch() creates symlinks to each child under 'path' and DiffAwareness handles checking all
+    // of these files and directories for changes. However, if a new file/directory is added
+    // directly under 'path', Bazel doesn't know that this has to be symlinked in. Thus, this
+    // creates a dependency on the contents of the 'path' directory.
+    SkyKey dirKey = DirectoryListingValue.key(
+        RootedPath.toRootedPath(fs.getRootDirectory(), path));
+    DirectoryListingValue directoryValue;
+    try {
+      directoryValue = (DirectoryListingValue) env.getValueOrThrow(
+          dirKey, InconsistentFilesystemException.class);
+    } catch (InconsistentFilesystemException e) {
+      throw new RepositoryFunctionException(
+          new IOException(e), SkyFunctionException.Transience.PERSISTENT);
+    }
+    if (directoryValue == null) {
+      return null;
+    }
+
     // Link x/y/z to /some/path/to/y/z.
-    if (!symlinkLocalRepositoryContents(
-        outputDirectory, directories.getOutputBase().getFileSystem().getPath(pathFragment))) {
+    if (!symlinkLocalRepositoryContents(outputDirectory, fs.getPath(pathFragment))) {
       return null;
     }
 
@@ -68,7 +93,7 @@ public class NewLocalRepositoryFunction extends RepositoryFunction {
     }
     createWorkspaceFile(outputDirectory, rule);
 
-    return RepositoryDirectoryValue.create(outputDirectory);
+    return RepositoryDirectoryValue.createWithSourceDirectory(outputDirectory, directoryValue);
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDirectoryValue.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDirectoryValue.java
index 5756d51753..9ada38de87 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDirectoryValue.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDirectoryValue.java
@@ -16,28 +16,43 @@ package com.google.devtools.build.lib.rules.repository;
 
 import com.google.common.base.Objects;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
+import com.google.devtools.build.lib.skyframe.DirectoryListingValue;
 import com.google.devtools.build.lib.skyframe.SkyFunctions;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 
+import javax.annotation.Nullable;
+
 /**
  * A local view of an external repository.
  */
 public class RepositoryDirectoryValue implements SkyValue {
   private final Path path;
   private final boolean fetchingDelayed;
+  @Nullable
+  private final DirectoryListingValue sourceDir;
 
-  private RepositoryDirectoryValue(Path path, boolean fetchingDelayed) {
+  private RepositoryDirectoryValue(
+      Path path, boolean fetchingDelayed, DirectoryListingValue sourceDir) {
     this.path = path;
     this.fetchingDelayed = fetchingDelayed;
+    this.sourceDir = sourceDir;
   }
 
   /**
    * Creates an immutable external repository.
    */
   public static RepositoryDirectoryValue create(Path repositoryDirectory) {
-    return new RepositoryDirectoryValue(repositoryDirectory, false);
+    return new RepositoryDirectoryValue(repositoryDirectory, false, null);
+  }
+
+  /**
+   * new_local_repositories
+   */
+  public static RepositoryDirectoryValue createWithSourceDirectory(
+      Path repositoryDirectory, DirectoryListingValue sourceDir) {
+    return new RepositoryDirectoryValue(repositoryDirectory, false, sourceDir);
   }
 
   /**
@@ -45,7 +60,7 @@ public class RepositoryDirectoryValue implements SkyValue {
    * {@code --nofetch} command line option.
    */
   public static RepositoryDirectoryValue fetchingDelayed(Path repositoryDirectory) {
-    return new RepositoryDirectoryValue(repositoryDirectory, true);
+    return new RepositoryDirectoryValue(repositoryDirectory, true, null);
   }
 
   /**
@@ -70,14 +85,15 @@ public class RepositoryDirectoryValue implements SkyValue {
 
     if (other instanceof RepositoryDirectoryValue) {
       RepositoryDirectoryValue otherValue = (RepositoryDirectoryValue) other;
-      return path.equals(otherValue.path);
+      return Objects.equal(path, otherValue.path)
+          && Objects.equal(sourceDir, otherValue.sourceDir);
     }
     return false;
   }
 
   @Override
   public int hashCode() {
-    return Objects.hashCode(path);
+    return Objects.hashCode(path, sourceDir);
   }
 
   @Override
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 1fd1b5fccc..d9c0b0b697 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
@@ -27,6 +27,7 @@ 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,6 +36,7 @@ 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;
@@ -80,6 +82,7 @@ import javax.annotation.Nullable;
  * {@link RepositoryDirectoryValue} is invalidated using the usual Skyframe route.
  */
 public abstract class RepositoryFunction {
+
   /**
    * Exception thrown when something goes wrong accessing a remote repository.
    *
@@ -350,6 +353,69 @@ public abstract class RepositoryFunction {
   }
 
   /**
+   * 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.
+   *
+   * Note that:
+   * - We don't add a dependency on the parent directory at the package root boundary, so
+   * the only transitive dependencies from files inside the package roots to external files
+   * are through symlinks. So the upwards transitive closure of external files is small.
+   * - The only way other than external repositories for external source files to get into the
+   * skyframe graph in the first place is through symlinks outside the package roots, which we
+   * neither want to encourage nor optimize for since it is not common. So the set of external
+   * files is small.
+   */
+  public static void addExternalFilesDependencies(
+      RootedPath rootedPath, BlazeDirectories directories, Environment env)
+      throws IOException {
+    Path externalRepoDir = getExternalRepositoryDirectory(directories);
+    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.
+      return;
+    }
+    String repositoryName = repositoryPath.getSegment(0);
+
+    Rule repositoryRule;
+    try {
+      repositoryRule = RepositoryFunction.getRule(repositoryName, env);
+    } 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) {
+      // 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.
+    if (repositoryRule.getRuleClass().equals(NewLocalRepositoryRule.NAME)
+        && repositoryPath.segmentCount() == 1) {
+      PathFragment pathDir = RepositoryFunction.getTargetPath(
+          repositoryRule, directories.getWorkspace());
+      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());
+      }
+    }
+  }
+
+  /**
    * Returns the RuleDefinition class for this type of repository.
    */
   public abstract Class<? extends RuleDefinition> getRuleDefinition();
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingValue.java
index 7731c1dd4e..3f61fe660e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingValue.java
@@ -54,7 +54,7 @@ public abstract class DirectoryListingValue implements SkyValue {
    * from a directory listing on its parent directory).
    */
   @ThreadSafe
-  static SkyKey key(RootedPath directoryUnderRoot) {
+  public static SkyKey key(RootedPath directoryUnderRoot) {
     return SkyKey.create(SkyFunctions.DIRECTORY_LISTING, directoryUnderRoot);
   }
 
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 d55a0297f4..792cc5fa13 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
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
+import com.google.devtools.build.lib.analysis.BlazeDirectories;
 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;
@@ -20,16 +21,17 @@ 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;
 import com.google.devtools.build.skyframe.SkyFunction;
 
+import java.io.IOException;
 import java.util.concurrent.atomic.AtomicReference;
 
 /** Common utilities for dealing with files outside the package roots. */
 public class ExternalFilesHelper {
   private final AtomicReference<PathPackageLocator> pkgLocator;
   private final ExternalFileAction externalFileAction;
+  private final BlazeDirectories directories;
 
   // These variables are set to true from multiple threads, but only read in the main thread.
   // So volatility or an AtomicBoolean is not needed.
@@ -42,18 +44,22 @@ public class ExternalFilesHelper {
    * @param errorOnExternalFiles If files outside of package paths should be allowed.
    */
   public ExternalFilesHelper(
-      AtomicReference<PathPackageLocator> pkgLocator, boolean errorOnExternalFiles) {
+      AtomicReference<PathPackageLocator> pkgLocator, boolean errorOnExternalFiles,
+      BlazeDirectories directories) {
     this(
         pkgLocator,
         errorOnExternalFiles
             ? ExternalFileAction.ERROR_OUT
-            : ExternalFileAction.DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_FILES);
+            : ExternalFileAction.DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_FILES,
+        directories);
   }
 
   private ExternalFilesHelper(AtomicReference<PathPackageLocator> pkgLocator,
-      ExternalFileAction externalFileAction) {
+      ExternalFileAction externalFileAction,
+      BlazeDirectories directories) {
     this.pkgLocator = pkgLocator;
     this.externalFileAction = externalFileAction;
+    this.directories = directories;
   }
 
   private enum ExternalFileAction {
@@ -117,7 +123,7 @@ public class ExternalFilesHelper {
   }
 
   ExternalFilesHelper cloneWithFreshExternalFilesKnowledge() {
-    return new ExternalFilesHelper(pkgLocator, externalFileAction);
+    return new ExternalFilesHelper(pkgLocator, externalFileAction, directories);
   }
 
   FileType getAndNoteFileType(RootedPath rootedPath) {
@@ -152,7 +158,7 @@ public class ExternalFilesHelper {
    */
   @ThreadSafe
   public void maybeHandleExternalFile(RootedPath rootedPath, SkyFunction.Environment env)
-      throws FileOutsidePackageRootsException {
+      throws IOException {
     FileType fileType = getAndNoteFileType(rootedPath);
     if (fileType == FileType.INTERNAL) {
       return;
@@ -166,39 +172,6 @@ public class ExternalFilesHelper {
     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.
-    //
-    // Note that:
-    // - We don't add a dependency on the parent directory at the package root boundary, so
-    // the only transitive dependencies from files inside the package roots to external files
-    // are through symlinks. So the upwards transitive closure of external files is small.
-    // - The only way other than external repositories for external source files to get into the
-    // skyframe graph in the first place is through symlinks outside the package roots, which we
-    // neither want to encourage nor optimize for since it is not common. So the set of external
-    // files is small.
-
-    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.
-      return;
-    }
-    String repositoryName = repositoryPath.getSegment(0);
-
-    try {
-      RepositoryFunction.getRule(repositoryName, env);
-    } 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.
-    } catch (RepositoryFunction.RepositoryFunctionException ex) {
-      // This should never happen.
-      throw new IllegalStateException(
-          "Repository " + repositoryName + " cannot be resolved for path " + rootedPath, ex);
-    }
+    RepositoryFunction.addExternalFilesDependencies(rootedPath, directories, env);
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index ca8dfadd2d..25bfc92688 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -310,7 +310,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
         binTools,
         (ConfiguredRuleClassProvider) pkgFactory.getRuleClassProvider());
     this.artifactFactory.set(skyframeBuildView.getArtifactFactory());
-    this.externalFilesHelper = new ExternalFilesHelper(pkgLocator, this.errorOnExternalFiles);
+    this.externalFilesHelper = new ExternalFilesHelper(
+        pkgLocator, this.errorOnExternalFiles, directories);
   }
 
   private ImmutableMap<SkyFunctionName, SkyFunction> skyFunctions(