Fix cycle detection between .bzl files.
The code assumed that the last element before a cycle was a BUILD file. It can
also be a .bzl file.
--
PiperOrigin-RevId: 143673940
MOS_MIGRATED_REVID=143673940
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 d66bd57..849fe1e 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
@@ -19,6 +19,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.skyframe.CycleInfo;
@@ -63,23 +64,32 @@
if (alreadyReported) {
return true;
} else if (Iterables.all(cycle, IS_SKYLARK_MODULE_SKY_KEY)
- // The last element of the path to the cycle has to be a PackageFunction.
- && IS_PACKAGE_SKY_KEY.apply(lastPathElement)) {
- StringBuilder cycleMessage =
- new StringBuilder()
- .append(lastPathElement.argument()).append("/BUILD: ")
- .append("cycle in referenced extension files: ");
+ // The last element before the cycle has to be a PackageFunction or SkylarkModule.
+ && (IS_PACKAGE_SKY_KEY.apply(lastPathElement)
+ || IS_SKYLARK_MODULE_SKY_KEY.apply(lastPathElement))) {
- AbstractLabelCycleReporter.printCycle(
- cycleInfo.getCycle(),
- cycleMessage,
+ Function printer =
new Function<SkyKey, String>() {
@Override
public String apply(SkyKey input) {
- return ((SkylarkImportLookupValue.SkylarkImportLookupKey) input.argument())
- .importLabel.toString();
+ if (input.argument() instanceof SkylarkImportLookupValue.SkylarkImportLookupKey) {
+ return ((SkylarkImportLookupValue.SkylarkImportLookupKey) input.argument())
+ .importLabel.toString();
+ } else if (input.argument() instanceof PackageIdentifier) {
+ return ((PackageIdentifier) input.argument()) + "/BUILD";
+ } else {
+ throw new UnsupportedOperationException();
+ }
}
- });
+ };
+
+ StringBuilder cycleMessage =
+ new StringBuilder()
+ .append("cycle detected in extension files: ")
+ .append("\n ")
+ .append(printer.apply(lastPathElement));
+
+ AbstractLabelCycleReporter.printCycle(cycleInfo.getCycle(), cycleMessage, printer);
// TODO(bazel-team): it would be nice to pass the Location of the load Statement in the
// BUILD file.
eventHandler.handle(Event.error(null, cycleMessage.toString()));
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
index bf6fc78..9ebc6d5 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
@@ -1120,7 +1120,6 @@
getConfiguredTarget("//test/skylark:cr4");
}
-
@Test
public void testRecursiveImport() throws Exception {
scratch.file("test/skylark/ext2.bzl", "load('/test/skylark/ext1', 'symbol2')");
@@ -1140,13 +1139,42 @@
// This is expected
}
assertContainsEvent(
- "test/skylark/BUILD: cycle in referenced extension files: \n"
+ "cycle detected in extension files: \n"
+ + " test/skylark/BUILD\n"
+ ".-> //test/skylark:ext1.bzl\n"
+ "| //test/skylark:ext2.bzl\n"
+ "`-- //test/skylark:ext1.bzl");
}
@Test
+ public void testRecursiveImport2() throws Exception {
+ scratch.file("test/skylark/ext1.bzl", "load('//test/skylark:ext2.bzl', 'symbol2')");
+ scratch.file("test/skylark/ext2.bzl", "load('//test/skylark:ext3.bzl', 'symbol3')");
+ scratch.file("test/skylark/ext3.bzl", "load('//test/skylark:ext4.bzl', 'symbol4')");
+ scratch.file("test/skylark/ext4.bzl", "load('//test/skylark:ext2.bzl', 'symbol2')");
+
+ scratch.file(
+ "test/skylark/BUILD",
+ "load('//test/skylark:ext1.bzl', 'custom_rule')",
+ "genrule(name = 'rule')");
+
+ reporter.removeHandler(failFastHandler);
+ try {
+ getTarget("//test/skylark:rule");
+ fail();
+ } catch (BuildFileContainsErrorsException e) {
+ // This is expected
+ }
+ assertContainsEvent(
+ "cycle detected in extension files: \n"
+ + " //test/skylark:ext1.bzl\n"
+ + ".-> //test/skylark:ext2.bzl\n"
+ + "| //test/skylark:ext3.bzl\n"
+ + "| //test/skylark:ext4.bzl\n"
+ + "`-- //test/skylark:ext2.bzl");
+ }
+
+ @Test
public void testSymbolPropagateThroughImports() throws Exception {
scratch.file("test/skylark/implementation.bzl", "def custom_rule_impl(ctx):", " return None");
@@ -1226,5 +1254,35 @@
assertContainsEvent("Loading of target '//test/skylark:rule' failed; build aborted");
assertThat(eventCollector).hasSize(1);
}
+
+ @Override
+ @Test
+ public void testRecursiveImport2() throws Exception {
+ scratch.file("test/skylark/ext1.bzl", "load('//test/skylark:ext2.bzl', 'symbol2')");
+ scratch.file("test/skylark/ext2.bzl", "load('//test/skylark:ext3.bzl', 'symbol3')");
+ scratch.file("test/skylark/ext3.bzl", "load('//test/skylark:ext4.bzl', 'symbol4')");
+ scratch.file("test/skylark/ext4.bzl", "load('//test/skylark:ext2.bzl', 'symbol2')");
+
+ scratch.file(
+ "test/skylark/BUILD",
+ "load('//test/skylark:ext1.bzl', 'custom_rule')",
+ "genrule(name = 'rule')");
+
+ reporter.removeHandler(failFastHandler);
+ try {
+ ensureTargetsVisited("//test/skylark:rule");
+ getTarget("//test/skylark:rule");
+ fail();
+ } catch (BuildFileContainsErrorsException e) {
+ // This is expected
+ }
+ assertContainsEvent("//test/skylark:ext2.bzl");
+ assertContainsEvent("//test/skylark:ext3.bzl");
+ assertContainsEvent("//test/skylark:ext4.bzl");
+ assertContainsEvent("Skylark import cycle");
+ assertContainsEvent("Loading of target '//test/skylark:rule' failed; build aborted");
+ assertThat(eventCollector).hasSize(1);
+ }
+
}
}
