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 #7871.
Improves on #7784.
Change-Id: Ic3eb499460c87ad5c78f21373d34e2ccbba2f2fe
PiperOrigin-RevId: 243804742
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkModuleCycleReporter.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkModuleCycleReporter.java
index 37c8958..a3d0a4d 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkModuleCycleReporter.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkModuleCycleReporter.java
@@ -61,6 +61,17 @@
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 @@
eventHandler.handle(Event.error(null, cycleMessage.toString()));
// To help debugging, request that the information be printed about where the respective
// repositories were defined.
- for (SkyKey repo : repos) {
- if (repo instanceof RepositoryValue.Key) {
- eventHandler.post(
- new RequestRepositoryInformationEvent(
- ((RepositoryValue.Key) repo).argument().strippedName()));
- }
- }
+ 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.
+ } 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();
- 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 + "'"));
- return true;
+ 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) {
+ 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_SKYLARK_IMPORTS_LOOKUP)) {
+ Label fileLabel =
+ ((SkylarkImportLookupValue.SkylarkImportLookupKey)
+ Iterables.getLast(Iterables.filter(cycle, IS_SKYLARK_IMPORTS_LOOKUP)))
+ .getImportLabel();
+ 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;
}
}
diff --git a/src/test/shell/bazel/external_integration_test.sh b/src/test/shell/bazel/external_integration_test.sh
index dceeda6..c050f7b 100755
--- a/src/test/shell/bazel/external_integration_test.sh
+++ b/src/test/shell/bazel/external_integration_test.sh
@@ -2151,4 +2151,43 @@
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"
