Fix crash when a command-line aspect depends on a cycle.
PiperOrigin-RevId: 231246040
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
index 320a8a2..eb525be 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
@@ -45,6 +45,9 @@
*/
public abstract static class AspectValueKey extends ActionLookupKey {
public abstract String getDescription();
+
+ @Override
+ public abstract Label getLabel();
}
/** A base class for a key representing an aspect applied to a particular target. */
@@ -313,11 +316,11 @@
return SkyFunctions.LOAD_SKYLARK_ASPECT;
}
- public String getSkylarkValueName() {
+ String getSkylarkValueName() {
return skylarkValueName;
}
- public SkylarkImport getSkylarkImport() {
+ SkylarkImport getSkylarkImport() {
return skylarkImport;
}
@@ -326,6 +329,11 @@
}
@Override
+ public Label getLabel() {
+ return targetLabel;
+ }
+
+ @Override
public String getDescription() {
// Skylark aspects are referred to on command line with <file>%<value ame>
return String.format("%s%%%s of %s", skylarkImport.getImportString(),
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetCycleReporter.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetCycleReporter.java
index 2baeaca..fe73b63 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetCycleReporter.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetCycleReporter.java
@@ -16,9 +16,11 @@
import static com.google.devtools.build.lib.skyframe.SkyFunctions.TRANSITIVE_TARGET;
import com.google.common.base.Function;
+import com.google.common.base.Preconditions;
import com.google.common.base.Predicate;
import com.google.common.base.Predicates;
import com.google.common.collect.Iterables;
+import com.google.devtools.build.lib.actions.ActionLookupValue;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.pkgcache.PackageProvider;
@@ -38,8 +40,9 @@
private static final Predicate<SkyKey> IS_CONFIGURED_TARGET_SKY_KEY =
Predicates.or(
- SkyFunctions.isSkyFunction(SkyFunctions.CONFIGURED_TARGET),
- SkyFunctions.isSkyFunction(SkyFunctions.ASPECT));
+ SkyFunctions.isSkyFunction(SkyFunctions.CONFIGURED_TARGET),
+ SkyFunctions.isSkyFunction(SkyFunctions.ASPECT),
+ SkyFunctions.isSkyFunction(SkyFunctions.LOAD_SKYLARK_ASPECT));
private static final Predicate<SkyKey> IS_TRANSITIVE_TARGET_SKY_KEY =
SkyFunctions.isSkyFunction(TRANSITIVE_TARGET);
@@ -97,6 +100,8 @@
return ((ConfiguredTargetKey) key.argument()).prettyPrint();
} else if (SkyFunctions.isSkyFunction(SkyFunctions.ASPECT).apply(key)) {
return ((AspectKey) key.argument()).prettyPrint();
+ } else if (key instanceof AspectValue.AspectValueKey) {
+ return ((AspectValue.AspectValueKey) key).getDescription();
} else {
return getLabel(key).toString();
}
@@ -104,14 +109,13 @@
@Override
public Label getLabel(SkyKey key) {
- if (SkyFunctions.isSkyFunction(SkyFunctions.CONFIGURED_TARGET).apply(key)) {
- return ((ConfiguredTargetKey) key.argument()).getLabel();
- } else if (SkyFunctions.isSkyFunction(SkyFunctions.ASPECT).apply(key)) {
- return ((AspectKey) key.argument()).getLabel();
+ if (key instanceof ActionLookupValue.ActionLookupKey) {
+ return Preconditions.checkNotNull(
+ ((ActionLookupValue.ActionLookupKey) key.argument()).getLabel(), key);
} else if (SkyFunctions.isSkyFunction(TRANSITIVE_TARGET).apply(key)) {
return ((TransitiveTargetKey) key).getLabel();
} else {
- throw new UnsupportedOperationException();
+ throw new UnsupportedOperationException(key.toString());
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java
index d89593c..38c361b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java
@@ -13,6 +13,7 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;
+import com.google.common.base.Preconditions;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
@@ -76,7 +77,8 @@
@Override
public Target getTarget(ExtendedEventHandler eventHandler, Label label)
throws NoSuchPackageException, NoSuchTargetException, InterruptedException {
- return getPackage(eventHandler, label.getPackageIdentifier()).getTarget(label.getName());
+ return Preconditions.checkNotNull(getPackage(eventHandler, label.getPackageIdentifier()), label)
+ .getTarget(label.getName());
}
@Override
diff --git a/src/main/java/com/google/devtools/build/skyframe/CyclesReporter.java b/src/main/java/com/google/devtools/build/skyframe/CyclesReporter.java
index 39c5fec..301774d 100644
--- a/src/main/java/com/google/devtools/build/skyframe/CyclesReporter.java
+++ b/src/main/java/com/google/devtools/build/skyframe/CyclesReporter.java
@@ -69,36 +69,41 @@
*/
public void reportCycles(
Iterable<CycleInfo> cycles, SkyKey topLevelKey, ExtendedEventHandler eventHandler) {
- Preconditions.checkNotNull(eventHandler);
+ Preconditions.checkNotNull(eventHandler, "topLevelKey: %s, Cycles %s", topLevelKey, cycles);
for (CycleInfo cycleInfo : cycles) {
- boolean alreadyReported = false;
- if (!cycleDeduper.seen(cycleInfo.getCycle())) {
- alreadyReported = true;
- }
- boolean successfullyReported = false;
- for (SingleCycleReporter cycleReporter : cycleReporters) {
- if (cycleReporter.maybeReportCycle(topLevelKey, cycleInfo, alreadyReported, eventHandler)) {
- successfullyReported = true;
- break;
- }
- }
- Preconditions.checkState(successfullyReported,
- printArbitraryCycle(topLevelKey, cycleInfo, alreadyReported));
+ maybeReportCycle(cycleInfo, topLevelKey, eventHandler);
}
}
- private String printArbitraryCycle(SkyKey topLevelKey, CycleInfo cycleInfo,
- boolean alreadyReported) {
- StringBuilder cycleMessage = new StringBuilder()
- .append("topLevelKey: " + topLevelKey + "\n")
- .append("alreadyReported: " + alreadyReported + "\n")
- .append("path to cycle:\n");
+ private void maybeReportCycle(
+ CycleInfo cycleInfo, SkyKey topLevelKey, ExtendedEventHandler eventHandler) {
+ boolean alreadyReported = !cycleDeduper.seen(cycleInfo.getCycle());
+ for (SingleCycleReporter cycleReporter : cycleReporters) {
+ if (cycleReporter.maybeReportCycle(topLevelKey, cycleInfo, alreadyReported, eventHandler)) {
+ return;
+ }
+ }
+ throw new IllegalStateException(
+ printArbitraryCycle(topLevelKey, cycleInfo, alreadyReported) + "\n" + cycleReporters);
+ }
+
+ private static String printArbitraryCycle(
+ SkyKey topLevelKey, CycleInfo cycleInfo, boolean alreadyReported) {
+ StringBuilder cycleMessage =
+ new StringBuilder()
+ .append("topLevelKey: ")
+ .append(topLevelKey)
+ .append("\n")
+ .append("alreadyReported: ")
+ .append(alreadyReported)
+ .append("\n")
+ .append("path to cycle:\n");
for (SkyKey skyKey : cycleInfo.getPathToCycle()) {
- cycleMessage.append(skyKey + "\n");
+ cycleMessage.append(skyKey).append("\n");
}
cycleMessage.append("cycle:\n");
for (SkyKey skyKey : cycleInfo.getCycle()) {
- cycleMessage.append(skyKey + "\n");
+ cycleMessage.append(skyKey).append("\n");
}
return cycleMessage.toString();
}
diff --git a/src/test/shell/integration/BUILD b/src/test/shell/integration/BUILD
index b45e811..b7cf8ed 100644
--- a/src/test/shell/integration/BUILD
+++ b/src/test/shell/integration/BUILD
@@ -69,6 +69,16 @@
)
sh_test(
+ name = "aspect_test",
+ size = "medium",
+ srcs = ["aspect_test.sh"],
+ data = [
+ ":test-deps",
+ "@bazel_tools//tools/bash/runfiles",
+ ],
+)
+
+sh_test(
name = "loading_phase_tests",
size = "large",
srcs = ["loading_phase_test.sh"],
diff --git a/src/test/shell/integration/aspect_test.sh b/src/test/shell/integration/aspect_test.sh
new file mode 100755
index 0000000..ff8fd06
--- /dev/null
+++ b/src/test/shell/integration/aspect_test.sh
@@ -0,0 +1,92 @@
+#!/bin/bash
+#
+# Copyright 2019 The Bazel Authors. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+# --- begin runfiles.bash initialization ---
+set -euo pipefail
+if [[ ! -d "${RUNFILES_DIR:-/dev/null}" && ! -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then
+ if [[ -f "$0.runfiles_manifest" ]]; then
+ export RUNFILES_MANIFEST_FILE="$0.runfiles_manifest"
+ elif [[ -f "$0.runfiles/MANIFEST" ]]; then
+ export RUNFILES_MANIFEST_FILE="$0.runfiles/MANIFEST"
+ elif [[ -f "$0.runfiles/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then
+ export RUNFILES_DIR="$0.runfiles"
+ fi
+fi
+if [[ -f "${RUNFILES_DIR:-/dev/null}/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then
+ source "${RUNFILES_DIR}/bazel_tools/tools/bash/runfiles/runfiles.bash"
+elif [[ -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then
+ source "$(grep -m1 "^bazel_tools/tools/bash/runfiles/runfiles.bash " \
+ "$RUNFILES_MANIFEST_FILE" | cut -d ' ' -f 2-)"
+else
+ echo >&2 "ERROR: cannot find @bazel_tools//tools/bash/runfiles:runfiles.bash"
+ exit 1
+fi
+# --- end runfiles.bash initialization ---
+
+source "$(rlocation "io_bazel/src/test/shell/integration_test_setup.sh")" \
+ || { echo "integration_test_setup.sh not found!" >&2; exit 1; }
+
+case "$(uname -s | tr [:upper:] [:lower:])" in
+msys*|mingw*|cygwin*)
+ declare -r is_windows=true
+ ;;
+*)
+ declare -r is_windows=false
+ ;;
+esac
+
+if "$is_windows"; then
+ export MSYS_NO_PATHCONV=1
+ export MSYS2_ARG_CONV_EXCL="*"
+ declare -r EXE_EXT=".exe"
+else
+ declare -r EXE_EXT=""
+fi
+
+# Tests in this file do not actually start a Python interpreter, but plug in a
+# fake stub executable to serve as the "interpreter".
+
+use_fake_python_runtimes_for_testsuite
+
+#### TESTS #############################################################
+
+# Tests that a cycle reached via a command-line aspect does not crash.
+# Does not crash deterministically, because if the configured target's cycle is
+# reported first, the aspect loading key's cycle is suppressed.
+function test_cycle_under_command_line_aspect() {
+ mkdir -p test
+ cat > test/aspect.bzl << 'EOF' || fail "Couldn't write aspect.bzl"
+def _simple_aspect_impl(target, ctx):
+ return struct()
+
+simple_aspect = aspect(implementation=_simple_aspect_impl)
+EOF
+ echo "sh_library(name = 'cycletarget', deps = [':cycletarget'])" \
+ > test/BUILD || fail "Couldn't write BUILD file"
+
+ # No flag, use the default from the rule.
+ bazel build --nobuild -k //test:cycletarget \
+ --aspects 'test/aspect.bzl%simple_aspect' &> $TEST_log \
+ && fail "Expected failure"
+ local readonly exit_code="$?"
+ [[ "$exit_code" == 1 ]] || fail "Unexpected exit code: $exit_code"
+ expect_log "cycle in dependency graph"
+ expect_log "//test:cycletarget \[self-edge\]"
+ expect_not_log "IllegalStateException"
+}
+
+run_suite "Tests for aspects"
