Report the full chain of repositories for WORKSPACE cycles

When a cycle through the workspace file is found, this should indicate that
a repository was used prior to being defined (either because it was defined
too late, or because it wasn't defined at all). In this case do not only
report the load statement that eventually lead to the missing definition,
but the full list of repositories involved; in particular, indicate that
the last one in this chain is the one where the definition is missing.

Fixes bazelbuild#7871.
Improves on bazelbuild#7784.

Change-Id: Ic3eb499460c87ad5c78f21373d34e2ccbba2f2fe
PiperOrigin-RevId: 243804742

src/main/java/com/google/devtools/build/lib/skyframe/SkylarkModuleCycleReporter.java

@@ -61,6 +61,17 @@ public class SkylarkModuleCycleReporter implements CyclesReporter.SingleCycleRep
   private static final Predicate<SkyKey> IS_LOCAL_REPOSITORY_LOOKUP =
       SkyFunctions.isSkyFunction(SkyFunctions.LOCAL_REPOSITORY_LOOKUP);
 
+  private static void requestRepoDefinitions(
+      ExtendedEventHandler eventHandler, Iterable<SkyKey> repos) {
+    for (SkyKey repo : repos) {
+      if (repo instanceof RepositoryValue.Key) {
+        eventHandler.post(
+            new RequestRepositoryInformationEvent(
+                ((RepositoryValue.Key) repo).argument().strippedName()));
+      }
+    }
+  }
+
   @Override
   public boolean maybeReportCycle(
       SkyKey topLevelKey,
@@ -142,50 +153,64 @@ public String apply(SkyKey input) {
       eventHandler.handle(Event.error(null, cycleMessage.toString()));
       // To help debugging, request that the information be printed about where the respective
       // repositories were defined.
+      requestRepoDefinitions(eventHandler, repos);
+      return true;
+    } else if (Iterables.any(cycle, IS_REPOSITORY) && Iterables.any(cycle, IS_WORKSPACE_FILE)) {
+      Iterable<SkyKey> repos =
+          Iterables.filter(Iterables.concat(pathToCycle, cycle), IS_REPOSITORY);
+
+      StringBuilder message = new StringBuilder();
+
+      if (Iterables.any(cycle, IS_SKYLARK_IMPORTS_LOOKUP)) {
+        Label fileLabel =
+            ((SkylarkImportLookupValue.SkylarkImportLookupKey)
+                    Iterables.getLast(Iterables.filter(cycle, IS_SKYLARK_IMPORTS_LOOKUP)))
+                .getImportLabel();
+        message.append("Failed to load Starlark extension '").append(fileLabel).append("'.\n");
+      }
+
+      message
+          .append("Cycle in the workspace file detected. ")
+          .append("This indicates that a repository is used prior to being defined.\n")
+          .append(
+              "The following chain of repository dependencies lead to the missing definition.\n");
       for (SkyKey repo : repos) {
         if (repo instanceof RepositoryValue.Key) {
-          eventHandler.post(
-              new RequestRepositoryInformationEvent(
-                  ((RepositoryValue.Key) repo).argument().strippedName()));
+          message
+              .append(" - ")
+              .append(((RepositoryValue.Key) repo).argument().getName())
+              .append("\n");
         }
       }
+      SkyKey missingRepo = Iterables.getLast(repos);
+      if (missingRepo instanceof RepositoryValue.Key) {
+        message
+            .append("This could either mean you have to add the '")
+            .append(((RepositoryValue.Key) missingRepo).argument().getName())
+            .append("' repository with a statement like `http_archive` in your WORKSPACE file")
+            .append(" (note that transitive dependencies are not added automatically), or move")
+            .append(" an existing definition earlier in your WORKSPACE file.");
+      }
+      eventHandler.handle(Event.error(message.toString()));
+      // To help debugging, request that the information be printed about where the respective
+      // repositories were defined.
+      requestRepoDefinitions(eventHandler, repos);
       return true;
-    } else if (Iterables.any(cycle, IS_WORKSPACE_FILE)
-        || IS_REPOSITORY_DIRECTORY.apply(lastPathElement)
-        || IS_PACKAGE_SKY_KEY.apply(lastPathElement)
-        || IS_EXTERNAL_PACKAGE.apply(lastPathElement)
-        || IS_LOCAL_REPOSITORY_LOOKUP.apply(lastPathElement)) {
-      // We have a cycle in the workspace file, report as such.
-      if (Iterables.any(cycle, IS_SKYLARK_IMPORTS_LOOKUP)) {
+    } else if (Iterables.any(cycle, IS_SKYLARK_IMPORTS_LOOKUP)) {
         Label fileLabel =
             ((SkylarkImportLookupValue.SkylarkImportLookupKey)
                     Iterables.getLast(Iterables.filter(cycle, IS_SKYLARK_IMPORTS_LOOKUP)))
                 .getImportLabel();
-        String repositoryName = fileLabel.getPackageIdentifier().getRepository().strippedName();
-        eventHandler.handle(
-            Event.error(
-                null,
-                "Failed to load Starlark extension '"
-                    + fileLabel
-                    + "'.\n"
-                    + "It usually happens when the repository is not defined prior to being used.\n"
-                    + "This could either mean you have to add the '"
-                    + fileLabel.getWorkspaceName()
-                    + "' repository with a statement like `http_archive` in your WORKSPACE file"
-                    + " (note that transitive dependencies are not added automatically), or"
-                    + " the repository '"
-                    + repositoryName
-                    + "' was defined too late in your WORKSPACE file."));
-        return true;
-      } else if (Iterables.any(cycle, IS_PACKAGE_LOOKUP)) {
-        PackageIdentifier pkg =
-            (PackageIdentifier)
-                Iterables.getLast(Iterables.filter(cycle, IS_PACKAGE_LOOKUP)).argument();
-        eventHandler.handle(Event.error(null, "cannot load package '" + pkg + "'"));
+      eventHandler.handle(
+          Event.error(null, "Failed to load Starlark extension '" + fileLabel + "'.\n"));
         return true;
-      }
+    } else if (Iterables.any(cycle, IS_PACKAGE_LOOKUP)) {
+      PackageIdentifier pkg =
+          (PackageIdentifier)
+              Iterables.getLast(Iterables.filter(cycle, IS_PACKAGE_LOOKUP)).argument();
+      eventHandler.handle(Event.error(null, "cannot load package '" + pkg + "'"));
+      return true;
     }
-
     return false;
   }
 }

src/test/shell/bazel/external_integration_test.sh

@@ -2151,4 +2151,43 @@ EOF
   expect_log "bar.bzl:4:3"
 }
 
+function test_missing_repo_reported() {
+  # Verify that, if a WORKSPACE cycle is reported due to
+  # a missing repository definition, the name of the actually
+  # missing repository is reported.
+  WRKDIR=$(mktemp -d "${TEST_TMPDIR}/testXXXXXX")
+  cd "${WRKDIR}"
+
+  mkdir main
+  cd main
+
+  cat > withimplicit.bzl <<'EOF'
+def _impl(ctx):
+  ctx.file("data.txt", ctx.attr.value)
+  ctx.file("data.bzl", "value = %s" % (ctx.attr.value,))
+  ctx.symlink(ctx.attr._generic_build_file, "BUILD")
+
+data_repo = repository_rule(
+  implementation = _impl,
+  attrs = { "value" : attr.string(),
+            "_generic_build_file" : attr.label(
+                default = Label("@this_repo_is_missing//:generic.BUILD")) },
+)
+EOF
+  touch BUILD
+  cat > WORKSPACE <<'EOF'
+load("//:withimplicit.bzl", "data_repo")
+
+data_repo(
+  name = "data",
+  value = "42")
+
+load("@data//:value.bzl", "value")
+EOF
+
+  bazel build //... > "${TEST_log}" 2>&1 && fail "expected failure" || :
+
+  expect_log "add.*this_repo_is_missing.*WORKSPACE"
+}
+
 run_suite "external tests"