Update package lookup to check for files named BUILD.bazel before files named

BUILD.

Fixes #552.

RELNOTES[NEW]: Packages are defined in BUILD.bazel as well as BUILD files.

--
MOS_MIGRATED_REVID=138828981

5 files changed, 133 insertions, 62 deletions

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 331a081b4e..f96ca84d57 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
@@ -34,6 +34,7 @@ import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 import java.io.IOException;
+import java.util.List;
 import java.util.concurrent.atomic.AtomicReference;
 import javax.annotation.Nullable;
 
@@ -51,12 +52,15 @@ public class PackageLookupFunction implements SkyFunction {
 
   private final AtomicReference<ImmutableSet<PackageIdentifier>> deletedPackages;
   private final CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy;
+  private final List<BuildFileName> buildFilesByPriority;
 
   public PackageLookupFunction(
       AtomicReference<ImmutableSet<PackageIdentifier>> deletedPackages,
-      CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy) {
+      CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy,
+      List<BuildFileName> buildFilesByPriority) {
     this.deletedPackages = deletedPackages;
     this.crossRepositoryLabelViolationStrategy = crossRepositoryLabelViolationStrategy;
+    this.buildFilesByPriority = buildFilesByPriority;
   }
 
   @Override
@@ -99,7 +103,7 @@ public class PackageLookupFunction implements SkyFunction {
       }
     }
 
-    return getPackageLookupValue(env, pkgLocator.getPathEntries(), packageKey, BuildFileName.BUILD);
+    return findPackageByBuildFile(env, pkgLocator, packageKey);
   }
 
   @Nullable
@@ -109,6 +113,32 @@ public class PackageLookupFunction implements SkyFunction {
   }
 
   @Nullable
+  private PackageLookupValue findPackageByBuildFile(
+      Environment env, PathPackageLocator pkgLocator, PackageIdentifier packageKey)
+      throws PackageLookupFunctionException, InterruptedException {
+    // TODO(bazel-team): The following is O(n^2) on the number of elements on the package path due
+    // to having restart the SkyFunction after every new dependency. However, if we try to batch
+    // the missing value keys, more dependencies than necessary will be declared. This wart can be
+    // fixed once we have nicer continuation support [skyframe-loading]
+    for (Path packagePathEntry : pkgLocator.getPathEntries()) {
+
+      // This checks for the build file names in the correct precedence order.
+      for (BuildFileName buildFileName : buildFilesByPriority) {
+        PackageLookupValue result =
+            getPackageLookupValue(env, packagePathEntry, packageKey, buildFileName);
+        if (result == null) {
+          return null;
+        }
+        if (result != PackageLookupValue.NO_BUILD_FILE_VALUE) {
+          return result;
+        }
+      }
+    }
+
+    return PackageLookupValue.NO_BUILD_FILE_VALUE;
+  }
+
+  @Nullable
   private static FileValue getFileValue(
       RootedPath fileRootedPath, Environment env, PackageIdentifier packageIdentifier)
       throws PackageLookupFunctionException, InterruptedException {
@@ -143,67 +173,84 @@ public class PackageLookupFunction implements SkyFunction {
       PackageIdentifier packageIdentifier,
       BuildFileName buildFileName)
       throws PackageLookupFunctionException, InterruptedException {
+
     // TODO(bazel-team): The following is O(n^2) on the number of elements on the package path due
     // to having restart the SkyFunction after every new dependency. However, if we try to batch
     // the missing value keys, more dependencies than necessary will be declared. This wart can be
     // fixed once we have nicer continuation support [skyframe-loading]
     for (Path packagePathEntry : packagePathEntries) {
-      PathFragment buildFileFragment = buildFileName.getBuildFileFragment(packageIdentifier);
-      RootedPath buildFileRootedPath = RootedPath.toRootedPath(packagePathEntry,
-          buildFileFragment);
-
-      if (crossRepositoryLabelViolationStrategy == CrossRepositoryLabelViolationStrategy.ERROR) {
-        // Is this path part of a local repository?
-        RootedPath currentPath =
-            RootedPath.toRootedPath(packagePathEntry, buildFileFragment.getParentDirectory());
-        SkyKey repositoryLookupKey = LocalRepositoryLookupValue.key(currentPath);
-
-        // TODO(jcater): Consider parallelizing these lookups.
-        LocalRepositoryLookupValue localRepository;
-        try {
-          localRepository =
-              (LocalRepositoryLookupValue)
-                  env.getValueOrThrow(
-                      repositoryLookupKey, ErrorDeterminingRepositoryException.class);
-          if (localRepository == null) {
-            return null;
-          }
-        } catch (ErrorDeterminingRepositoryException e) {
-          // If the directory selected isn't part of a repository, that's an error.
-          // TODO(katre): Improve the error message given here.
-          throw new PackageLookupFunctionException(
-              new BuildFileNotFoundException(
-                  packageIdentifier,
-                  "Unable to determine the local repository for directory "
-                      + currentPath.asPath().getPathString()),
-              Transience.PERSISTENT);
-        }
+      PackageLookupValue result =
+          getPackageLookupValue(env, packagePathEntry, packageIdentifier, buildFileName);
+      if (result == null) {
+        return null;
+      }
+      if (result != PackageLookupValue.NO_BUILD_FILE_VALUE) {
+        return result;
+      }
+    }
+    return PackageLookupValue.NO_BUILD_FILE_VALUE;
+  }
 
-        if (localRepository.exists()
-            && !localRepository.getRepository().equals(packageIdentifier.getRepository())) {
-          // There is a repository mismatch, this is an error.
-          // TODO(jcater): Work out the correct package name for this error message.
-          return PackageLookupValue.invalidPackageName(
-              "Package crosses into repository " + localRepository.getRepository().getName());
-        }
+  private PackageLookupValue getPackageLookupValue(
+      Environment env,
+      Path packagePathEntry,
+      PackageIdentifier packageIdentifier,
+      BuildFileName buildFileName)
+      throws InterruptedException, PackageLookupFunctionException {
+    PathFragment buildFileFragment = buildFileName.getBuildFileFragment(packageIdentifier);
+    RootedPath buildFileRootedPath = RootedPath.toRootedPath(packagePathEntry, buildFileFragment);
 
-        // There's no local repository, keep going.
-      } else {
-        // Future-proof against adding future values to CrossRepositoryLabelViolationStrategy.
-        Preconditions.checkState(
-            crossRepositoryLabelViolationStrategy == CrossRepositoryLabelViolationStrategy.IGNORE,
-            crossRepositoryLabelViolationStrategy);
-      }
+    if (crossRepositoryLabelViolationStrategy == CrossRepositoryLabelViolationStrategy.ERROR) {
+      // Is this path part of a local repository?
+      RootedPath currentPath =
+          RootedPath.toRootedPath(packagePathEntry, buildFileFragment.getParentDirectory());
+      SkyKey repositoryLookupKey = LocalRepositoryLookupValue.key(currentPath);
 
-      // Check for the existence of the build file.
-      FileValue fileValue = getFileValue(buildFileRootedPath, env, packageIdentifier);
-      if (fileValue == null) {
-        return null;
+      // TODO(jcater): Consider parallelizing these lookups.
+      LocalRepositoryLookupValue localRepository;
+      try {
+        localRepository =
+            (LocalRepositoryLookupValue)
+                env.getValueOrThrow(repositoryLookupKey, ErrorDeterminingRepositoryException.class);
+        if (localRepository == null) {
+          return null;
+        }
+      } catch (ErrorDeterminingRepositoryException e) {
+        // If the directory selected isn't part of a repository, that's an error.
+        // TODO(katre): Improve the error message given here.
+        throw new PackageLookupFunctionException(
+            new BuildFileNotFoundException(
+                packageIdentifier,
+                "Unable to determine the local repository for directory "
+                    + currentPath.asPath().getPathString()),
+            Transience.PERSISTENT);
       }
-      if (fileValue.isFile()) {
-        return PackageLookupValue.success(buildFileRootedPath.getRoot(), buildFileName);
+
+      if (localRepository.exists()
+          && !localRepository.getRepository().equals(packageIdentifier.getRepository())) {
+        // There is a repository mismatch, this is an error.
+        // TODO(jcater): Work out the correct package name for this error message.
+        return PackageLookupValue.invalidPackageName(
+            "Package crosses into repository " + localRepository.getRepository().getName());
       }
+
+      // There's no local repository, keep going.
+    } else {
+      // Future-proof against adding future values to CrossRepositoryLabelViolationStrategy.
+      Preconditions.checkState(
+          crossRepositoryLabelViolationStrategy == CrossRepositoryLabelViolationStrategy.IGNORE,
+          crossRepositoryLabelViolationStrategy);
+    }
+
+    // Check for the existence of the build file.
+    FileValue fileValue = getFileValue(buildFileRootedPath, env, packageIdentifier);
+    if (fileValue == null) {
+      return null;
     }
+    if (fileValue.isFile()) {
+      return PackageLookupValue.success(buildFileRootedPath.getRoot(), buildFileName);
+    }
+
     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 8ad5ae0c06..32d1e9007f 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
@@ -39,6 +39,7 @@ public abstract class PackageLookupValue implements SkyValue {
    * The file (BUILD, WORKSPACE, etc.) that defines this package, referred to as the "build file".
    */
   public enum BuildFileName {
+
     WORKSPACE("WORKSPACE") {
       @Override
       public PathFragment getBuildFileFragment(PackageIdentifier packageIdentifier) {
@@ -50,6 +51,12 @@ public abstract class PackageLookupValue implements SkyValue {
       public PathFragment getBuildFileFragment(PackageIdentifier packageIdentifier) {
         return packageIdentifier.getPackageFragment().getChild(getFilename());
       }
+    },
+    BUILD_DOT_BAZEL("BUILD.bazel") {
+      @Override
+      public PathFragment getBuildFileFragment(PackageIdentifier packageIdentifier) {
+        return packageIdentifier.getPackageFragment().getChild(getFilename());
+      }
     };
 
     private static final BuildFileName[] VALUES = BuildFileName.values();
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
index 7a45eeded1..40e8a5d14f 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
@@ -48,6 +48,7 @@ import com.google.devtools.build.lib.skyframe.ExternalFilesHelper.ExternalFileAc
 import com.google.devtools.build.lib.skyframe.ExternalFilesHelper.ExternalFilesKnowledge;
 import com.google.devtools.build.lib.skyframe.ExternalFilesHelper.FileType;
 import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy;
+import com.google.devtools.build.lib.skyframe.PackageLookupValue.BuildFileName;
 import com.google.devtools.build.lib.util.AbruptExitException;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.Preconditions;
@@ -73,6 +74,7 @@ import java.util.ArrayList;
 import java.util.Collection;
 import java.util.EnumSet;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.UUID;
@@ -112,7 +114,8 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor {
       Iterable<SkyValueDirtinessChecker> customDirtinessCheckers,
       PathFragment blacklistedPackagePrefixesFile,
       String productName,
-      CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy) {
+      CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy,
+      List<BuildFileName> buildFilesByPriority) {
     super(
         evaluatorSupplier,
         pkgFactory,
@@ -127,7 +130,8 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor {
         ExternalFileAction.DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_PATHS,
         blacklistedPackagePrefixesFile,
         productName,
-        crossRepositoryLabelViolationStrategy);
+        crossRepositoryLabelViolationStrategy,
+        buildFilesByPriority);
     this.diffAwarenessManager = new DiffAwarenessManager(diffAwarenessFactories);
     this.customDirtinessCheckers = customDirtinessCheckers;
   }
@@ -145,7 +149,8 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor {
       ImmutableList<PrecomputedValue.Injected> extraPrecomputedValues,
       Iterable<SkyValueDirtinessChecker> customDirtinessCheckers,
       String productName,
-      CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy) {
+      CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy,
+      List<BuildFileName> buildFilesByPriority) {
     return create(
         pkgFactory,
         directories,
@@ -160,7 +165,8 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor {
         customDirtinessCheckers,
         /*blacklistedPackagePrefixesFile=*/ PathFragment.EMPTY_FRAGMENT,
         productName,
-        crossRepositoryLabelViolationStrategy);
+        crossRepositoryLabelViolationStrategy,
+        buildFilesByPriority);
   }
 
   private static SequencedSkyframeExecutor create(
@@ -177,7 +183,8 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor {
       Iterable<SkyValueDirtinessChecker> customDirtinessCheckers,
       PathFragment blacklistedPackagePrefixesFile,
       String productName,
-      CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy) {
+      CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy,
+      List<BuildFileName> buildFilesByPriority) {
     SequencedSkyframeExecutor skyframeExecutor =
         new SequencedSkyframeExecutor(
             InMemoryMemoizingEvaluator.SUPPLIER,
@@ -194,7 +201,8 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor {
             customDirtinessCheckers,
             blacklistedPackagePrefixesFile,
             productName,
-            crossRepositoryLabelViolationStrategy);
+            crossRepositoryLabelViolationStrategy,
+            buildFilesByPriority);
     skyframeExecutor.init();
     return skyframeExecutor;
   }
@@ -221,7 +229,8 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor {
         ImmutableList.<SkyValueDirtinessChecker>of(),
         blacklistedPackagePrefixesFile,
         productName,
-        CrossRepositoryLabelViolationStrategy.ERROR);
+        CrossRepositoryLabelViolationStrategy.ERROR,
+        ImmutableList.of(BuildFileName.BUILD_DOT_BAZEL, BuildFileName.BUILD));
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java
index 1e3b046738..4c92f23612 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java
@@ -23,6 +23,7 @@ import com.google.devtools.build.lib.analysis.config.BinTools;
 import com.google.devtools.build.lib.packages.PackageFactory;
 import com.google.devtools.build.lib.packages.Preprocessor;
 import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy;
+import com.google.devtools.build.lib.skyframe.PackageLookupValue.BuildFileName;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyFunctionName;
@@ -59,6 +60,7 @@ public class SequencedSkyframeExecutorFactory implements SkyframeExecutorFactory
         extraPrecomputedValues,
         customDirtinessCheckers,
         productName,
-        CrossRepositoryLabelViolationStrategy.ERROR);
+        CrossRepositoryLabelViolationStrategy.ERROR,
+        ImmutableList.of(BuildFileName.BUILD_DOT_BAZEL, BuildFileName.BUILD));
   }
 }
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 39cf8c8e06..ec330247dc 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
@@ -108,6 +108,7 @@ import com.google.devtools.build.lib.skyframe.DirtinessCheckerUtils.FileDirtines
 import com.google.devtools.build.lib.skyframe.ExternalFilesHelper.ExternalFileAction;
 import com.google.devtools.build.lib.skyframe.PackageFunction.CacheEntryWithGlobDeps;
 import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy;
+import com.google.devtools.build.lib.skyframe.PackageLookupValue.BuildFileName;
 import com.google.devtools.build.lib.skyframe.SkyframeActionExecutor.ActionCompletedReceiver;
 import com.google.devtools.build.lib.skyframe.SkyframeActionExecutor.ProgressSupplier;
 import com.google.devtools.build.lib.util.AbruptExitException;
@@ -274,6 +275,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
   private final RuleClassProvider ruleClassProvider;
 
   private final CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy;
+  
+  private final List<BuildFileName> buildFilesByPriority;
 
   private static final Logger LOG = Logger.getLogger(SkyframeExecutor.class.getName());
 
@@ -291,7 +294,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       ExternalFileAction externalFileAction,
       PathFragment blacklistedPackagePrefixesFile,
       String productName,
-      CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy) {
+      CrossRepositoryLabelViolationStrategy crossRepositoryLabelViolationStrategy,
+      List<BuildFileName> buildFilesByPriority) {
     // Strictly speaking, these arguments are not required for initialization, but all current
     // callsites have them at hand, so we might as well set them during construction.
     this.evaluatorSupplier = evaluatorSupplier;
@@ -325,6 +329,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
         pkgLocator, this.externalFileAction, directories);
     this.productName = productName;
     this.crossRepositoryLabelViolationStrategy = crossRepositoryLabelViolationStrategy;
+    this.buildFilesByPriority = buildFilesByPriority;
   }
 
   private ImmutableMap<SkyFunctionName, SkyFunction> skyFunctions(
@@ -348,7 +353,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     map.put(SkyFunctions.DIRECTORY_LISTING, new DirectoryListingFunction());
     map.put(
         SkyFunctions.PACKAGE_LOOKUP,
-        new PackageLookupFunction(deletedPackages, crossRepositoryLabelViolationStrategy));
+        new PackageLookupFunction(
+            deletedPackages, crossRepositoryLabelViolationStrategy, buildFilesByPriority));
     map.put(SkyFunctions.CONTAINING_PACKAGE_LOOKUP, new ContainingPackageLookupFunction());
     map.put(SkyFunctions.AST_FILE_LOOKUP, new ASTFileLookupFunction(ruleClassProvider));
     map.put(