Now that external files are not always treated as immutable, eliminate the "overlaid BUILD files" hack in RepositoryValue.

--
MOS_MIGRATED_REVID=109877252

7 files changed, 17 insertions, 80 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/LocalRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/LocalRepositoryFunction.java
index bbf22e9b5c..8bb7cc96d7 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/repository/LocalRepositoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/repository/LocalRepositoryFunction.java
@@ -58,7 +58,7 @@ public class LocalRepositoryFunction extends RepositoryFunction {
     try {
       FileSystemUtils.createDirectoryAndParents(repositoryPath.getParentDirectory());
       if (repositoryPath.exists(Symlinks.NOFOLLOW)) {
-        repositoryPath.delete();
+        FileSystemUtils.deleteTree(repositoryPath);
       }
       repositoryPath.createSymbolicLink(pathFragment);
     } catch (IOException e) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java
index a79b1d848b..4200d92f4c 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java
@@ -136,10 +136,6 @@ public class RepositoryDelegatorFunction implements SkyFunction {
         + "run the build without the '--nofetch' command line option.",
         rule.getName())));
 
-    // NB: This returns the wrong repository value for non-local new_* repository functions because
-    // overlaidBuildFile won't be set.
-    // TODO(lberki): overlaidBuildFile can now probably be removed. Try to excise it write a test
-    // that makes sure this works as expected.
     return RepositoryValue.fetchingDelayed(repoRootValue.realRootedPath().asPath());
   }
 
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 75af99931d..d13ca101a2 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
@@ -252,6 +252,12 @@ public abstract class RepositoryFunction {
     SkyKey buildFileKey = FileValue.key(rootedBuild);
     FileValue buildFileValue;
     try {
+      // Note that this dependency is, strictly speaking, not necessary: the symlink could simply
+      // point to this FileValue and the symlink chasing could be done while loading the package
+      // but this results in a nicer error message and it's correct as long as RepositoryFunctions
+      // don't write to things in the file system this FileValue depends on. In theory, the latter
+      // is possible if the file referenced by build_file is a symlink to somewhere under the
+      // external/ directory, but if you do that, you are really asking for trouble.
       buildFileValue = (FileValue) env.getValueOrThrow(buildFileKey, IOException.class,
           FileSymlinkException.class, InconsistentFilesystemException.class);
       if (buildFileValue == null) {
@@ -285,7 +291,7 @@ public abstract class RepositoryFunction {
     if (createSymbolicLink(buildFilePath, buildFileValue.realRootedPath().asPath(), env) == null) {
       return null;
     }
-    return RepositoryValue.createNew(outputDirectory, buildFileValue);
+    return RepositoryValue.create(outputDirectory);
   }
 
   protected static PathFragment getTargetPath(Rule rule) throws RepositoryFunctionException {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java
index 28557d1b9c..36350f5c31 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java
@@ -13,7 +13,6 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
-import com.google.common.base.Optional;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.cmdline.LabelValidator;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
@@ -178,14 +177,11 @@ public class PackageLookupFunction implements SkyFunction {
     if (fileValue == null) {
       return null;
     }
-    Optional<FileValue> overlaidBuildFile = repositoryValue.getOverlaidBuildFile();
+
     if (fileValue.isFile()) {
-      if (overlaidBuildFile.isPresent()) {
-        return PackageLookupValue.overlaidBuildFile(repositoryValue.getPath(), overlaidBuildFile);
-      } else {
         return PackageLookupValue.success(repositoryValue.getPath());
-      }
     }
+
     return PackageLookupValue.NO_BUILD_FILE_VALUE;
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupValue.java
index 7eb743f651..be8f883780 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupValue.java
@@ -13,7 +13,6 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
-import com.google.common.base.Optional;
 import com.google.common.base.Preconditions;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.vfs.Path;
@@ -21,8 +20,6 @@ import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 
-import java.util.Objects;
-
 /**
  * A value that represents a package lookup result.
  *
@@ -59,11 +56,6 @@ public abstract class PackageLookupValue implements SkyValue {
     return new SuccessfulPackageLookupValue(root);
   }
 
-  public static PackageLookupValue overlaidBuildFile(
-      Path root, Optional<FileValue> overlaidBuildFile) {
-    return new OverlaidPackageLookupValue(root, overlaidBuildFile);
-  }
-
   public static PackageLookupValue workspace(Path root) {
     return new WorkspacePackageLookupValue(root);
   }
@@ -151,35 +143,6 @@ public abstract class PackageLookupValue implements SkyValue {
     }
   }
 
-  /**
-   * A package under external/ that has a BUILD file that is not under external/.
-   *
-   * <p>This is kind of a hack to get around our assumption that external/ is immutable.</p>
-   */
-  private static class OverlaidPackageLookupValue extends SuccessfulPackageLookupValue {
-
-    private final Optional<FileValue> overlaidBuildFile;
-
-    public OverlaidPackageLookupValue(Path root, Optional<FileValue> overlaidBuildFile) {
-      super(root);
-      this.overlaidBuildFile = overlaidBuildFile;
-    }
-
-    @Override
-    public boolean equals(Object obj) {
-      if (!(obj instanceof OverlaidPackageLookupValue)) {
-        return false;
-      }
-      OverlaidPackageLookupValue other = (OverlaidPackageLookupValue) obj;
-      return super.equals(other) && overlaidBuildFile.equals(other.overlaidBuildFile);
-    }
-
-    @Override
-    public int hashCode() {
-      return Objects.hash(super.hashCode(), overlaidBuildFile);
-    }
-  }
-
   // TODO(kchodorow): fix these semantics.  This class should not exist, WORKSPACE lookup should
   // just return success/failure like a "normal" package.
   private static class WorkspacePackageLookupValue extends SuccessfulPackageLookupValue {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryValue.java
index ef9087144c..4aea6ea92b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryValue.java
@@ -15,7 +15,6 @@
 package com.google.devtools.build.lib.skyframe;
 
 import com.google.common.base.Objects;
-import com.google.common.base.Optional;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier.RepositoryName;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.skyframe.SkyKey;
@@ -26,20 +25,10 @@ import com.google.devtools.build.skyframe.SkyValue;
  */
 public class RepositoryValue implements SkyValue {
   private final Path path;
-
-  /**
-   * If this repository is using a user-created BUILD file (any of the new_* functions) then that
-   * FileValue needs to be propagated up to the PackageLookup so it doesn't get pruned. The BUILD
-   * file symlink will be under external/, thus assumed to be immutable, thus Skyframe will prune
-   * it. Then user changes will be ignored (in favor of the cached version).
-   */
-  private final Optional<FileValue> overlaidBuildFile;
   private final boolean fetchingDelayed;
 
-  private RepositoryValue(
-      Path path, Optional<FileValue> overlaidBuildFile, boolean fetchingDelayed) {
+  private RepositoryValue(Path path, boolean fetchingDelayed) {
     this.path = path;
-    this.overlaidBuildFile = overlaidBuildFile;
     this.fetchingDelayed = fetchingDelayed;
   }
 
@@ -47,7 +36,7 @@ public class RepositoryValue implements SkyValue {
    * Creates an immutable external repository.
    */
   public static RepositoryValue create(Path repositoryDirectory) {
-    return new RepositoryValue(repositoryDirectory, Optional.<FileValue>absent(), false);
+    return new RepositoryValue(repositoryDirectory, false);
   }
 
   /**
@@ -55,14 +44,7 @@ public class RepositoryValue implements SkyValue {
    * {@code --nofetch} command line option.
    */
   public static RepositoryValue fetchingDelayed(Path repositoryDirectory) {
-    return new RepositoryValue(repositoryDirectory, Optional.<FileValue>absent(), true);
-  }
-
-  /**
-   * Creates an immutable external repository with a mutable BUILD file.
-   */
-  public static RepositoryValue createNew(Path repositoryDirectory, FileValue overlaidBuildFile) {
-    return new RepositoryValue(repositoryDirectory, Optional.of(overlaidBuildFile), false);
+    return new RepositoryValue(repositoryDirectory, true);
   }
 
   /**
@@ -75,10 +57,6 @@ public class RepositoryValue implements SkyValue {
     return path;
   }
 
-  public Optional<FileValue> getOverlaidBuildFile() {
-    return overlaidBuildFile;
-  }
-
   public boolean isFetchingDelayed() {
     return fetchingDelayed;
   }
@@ -91,20 +69,19 @@ public class RepositoryValue implements SkyValue {
 
     if (other instanceof RepositoryValue) {
       RepositoryValue otherValue = (RepositoryValue) other;
-      return overlaidBuildFile.equals(otherValue.overlaidBuildFile);
+      return path.equals(otherValue.path);
     }
     return false;
   }
 
   @Override
   public int hashCode() {
-    return Objects.hashCode(overlaidBuildFile);
+    return Objects.hashCode(path);
   }
 
   @Override
   public String toString() {
-    return path.getPathString() + (overlaidBuildFile.isPresent()
-        ? " (BUILD file: " + overlaidBuildFile.get() + ")" : "");
+    return path.getPathString();
   }
 
   /**
diff --git a/src/test/shell/bazel/local_repository_test.sh b/src/test/shell/bazel/local_repository_test.sh
index 127fff74f2..7e6d8a447c 100755
--- a/src/test/shell/bazel/local_repository_test.sh
+++ b/src/test/shell/bazel/local_repository_test.sh
@@ -967,8 +967,7 @@ EOF
   expect_log "42"
 }
 
-# Currently disabled due to a bug in Bazel. Stay tuned.
-function DISABLED_test_change_new_repository_build_file() {
+function test_change_new_repository_build_file() {
   local r=$TEST_TMPDIR/r
   rm -fr $r
   mkdir -p $r