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 320a8a2e48d95a..eb525be77cf71d 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 final class AspectValue extends BasicActionLookupValue { */ 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 @@ public SkyFunctionName functionName() { return SkyFunctions.LOAD_SKYLARK_ASPECT; } - public String getSkylarkValueName() { + String getSkylarkValueName() { return skylarkValueName; } - public SkylarkImport getSkylarkImport() { + SkylarkImport getSkylarkImport() { return skylarkImport; } @@ -325,6 +328,11 @@ protected boolean isAspectConfigurationHost() { return false; } + @Override + public Label getLabel() { + return targetLabel; + } + @Override public String getDescription() { // Skylark aspects are referred to on command line with % 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 2baeacaff33263..fe73b63c33a897 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 @@ class ConfiguredTargetCycleReporter extends AbstractLabelCycleReporter { private static final Predicate 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 IS_TRANSITIVE_TARGET_SKY_KEY = SkyFunctions.isSkyFunction(TRANSITIVE_TARGET); @@ -97,6 +100,8 @@ public String prettyPrint(SkyKey key) { 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 @@ public String prettyPrint(SkyKey key) { @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 d89593ca07f6a5..38c361b7ca4d32 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 @@ public Package getPackage(ExtendedEventHandler eventHandler, PackageIdentifier p @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 39c5fec71d6b96..301774d13ae642 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 CyclesReporter(SingleCycleReporter... cycleReporters) { */ public void reportCycles( Iterable 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; - } + maybeReportCycle(cycleInfo, topLevelKey, eventHandler); + } + } + + 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; } - Preconditions.checkState(successfullyReported, - printArbitraryCycle(topLevelKey, cycleInfo, alreadyReported)); } + throw new IllegalStateException( + printArbitraryCycle(topLevelKey, cycleInfo, alreadyReported) + "\n" + cycleReporters); } - 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 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 b45e81106f45f8..b7cf8edb2ee7f0 100644 --- a/src/test/shell/integration/BUILD +++ b/src/test/shell/integration/BUILD @@ -68,6 +68,16 @@ sh_test( ], ) +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", diff --git a/src/test/shell/integration/aspect_test.sh b/src/test/shell/integration/aspect_test.sh new file mode 100755 index 00000000000000..ff8fd06a382feb --- /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"