Add check to LocalRepositoryFunction that the path contains a WORKSPACE

file.

Fixes #2841.

RELNOTES: Every local_repository now requires a WORKSPACE file.

Change-Id: I11d50b852796b8f919b1a61c8c9b59cb78c5b724
PiperOrigin-RevId: 154179215

2 files changed, 65 insertions, 5 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 916569c4cb..490b1f6324 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
@@ -17,13 +17,19 @@ 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.FileSymlinkException;
 import com.google.devtools.build.lib.skyframe.FileValue;
+import com.google.devtools.build.lib.skyframe.InconsistentFilesystemException;
+import com.google.devtools.build.lib.skyframe.PackageLookupValue;
 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.Transience;
+import com.google.devtools.build.skyframe.SkyKey;
 import java.io.IOException;
 import java.util.Map;
+import javax.annotation.Nullable;
 
 /**
  * Access a repository on the local filesystem.
@@ -62,7 +68,20 @@ public class LocalRepositoryFunction extends RepositoryFunction {
 
     if (!repositoryValue.isDirectory()) {
       throw new RepositoryFunctionException(
-          new IOException(source + " must be an existing directory"), Transience.TRANSIENT);
+          new IOException(source + " must be an existing directory"), Transience.PERSISTENT);
+    }
+
+    // Check that the repository contains a WORKSPACE file.
+    // It's important to check the real path, otherwise this looks under the "external/[repo]" path
+    // and cause a Skyframe cycle in the lookup.
+    FileValue workspaceFileValue = getWorkspaceFile(repositoryValue.realRootedPath(), env);
+    if (workspaceFileValue == null) {
+      return null;
+    }
+
+    if (!workspaceFileValue.exists()) {
+      throw new RepositoryFunctionException(
+          new IOException("No WORKSPACE file found in " + source), Transience.PERSISTENT);
     }
 
     return RepositoryDirectoryValue.builder().setPath(source);
@@ -72,4 +91,32 @@ public class LocalRepositoryFunction extends RepositoryFunction {
   public Class<? extends RuleDefinition> getRuleDefinition() {
     return LocalRepositoryRule.class;
   }
+
+  @Nullable
+  protected static FileValue getWorkspaceFile(RootedPath directory, Environment env)
+      throws RepositoryFunctionException, InterruptedException {
+    RootedPath workspaceRootedFile =
+        RootedPath.toRootedPath(
+            directory.getRoot(),
+            directory
+                .getRelativePath()
+                .getChild(PackageLookupValue.BuildFileName.WORKSPACE.getFilename()));
+
+    SkyKey workspaceFileKey = FileValue.key(workspaceRootedFile);
+    FileValue value;
+    try {
+      value =
+          (FileValue)
+              env.getValueOrThrow(
+                  workspaceFileKey,
+                  IOException.class,
+                  FileSymlinkException.class,
+                  InconsistentFilesystemException.class);
+    } catch (IOException | FileSymlinkException | InconsistentFilesystemException e) {
+      throw new RepositoryFunctionException(
+          new IOException("Could not access " + workspaceRootedFile + ": " + e.getMessage()),
+          Transience.PERSISTENT);
+    }
+    return value;
+  }
 }
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 8afb6eeb83..20c26b5942 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
@@ -31,6 +31,7 @@ import com.google.devtools.build.lib.skyframe.FileSymlinkException;
 import com.google.devtools.build.lib.skyframe.FileValue;
 import com.google.devtools.build.lib.skyframe.InconsistentFilesystemException;
 import com.google.devtools.build.lib.skyframe.PackageLookupValue;
+import com.google.devtools.build.lib.skyframe.PackageLookupValue.BuildFileName;
 import com.google.devtools.build.lib.skyframe.WorkspaceFileValue;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.Type;
@@ -462,11 +463,23 @@ public abstract class RepositoryFunction {
     String repositoryName = repositoryPath.getSegment(0);
 
     try {
-      // 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);
+      // Add a dependency to the repository rule. RepositoryDirectoryValue does add this
+      // dependency already but we want to catch RepositoryNotFoundException, so invoke #getRule
+      // first.
+      Rule rule = RepositoryFunction.getRule(repositoryName, env);
+      if (rule == null) {
+        return;
+      }
+
       if (repositoryPath.segmentCount() > 1) {
-        // For all file under the repository directory, depends on the actual RepositoryDirectory
+        if (rule.getRuleClass().equals(LocalRepositoryRule.NAME)
+            && repositoryPath.endsWith(
+                PathFragment.create(BuildFileName.WORKSPACE.getFilename()))) {
+          // Ignore this, there is a dependency from LocalRepositoryFunction->WORKSPACE file already
+          return;
+        }
+
+        // For all files under the repository directory, depend 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).