Skip to content

Commit

Permalink
Fix crash when a command-line aspect depends on a cycle.
Browse files Browse the repository at this point in the history
PiperOrigin-RevId: 231246040
  • Loading branch information
janakdr authored and weixiao-huang committed Jan 31, 2019
1 parent 9efd018 commit 30825c2
Show file tree
Hide file tree
Showing 6 changed files with 152 additions and 31 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down Expand Up @@ -313,18 +316,23 @@ public SkyFunctionName functionName() {
return SkyFunctions.LOAD_SKYLARK_ASPECT;
}

public String getSkylarkValueName() {
String getSkylarkValueName() {
return skylarkValueName;
}

public SkylarkImport getSkylarkImport() {
SkylarkImport getSkylarkImport() {
return skylarkImport;
}

protected boolean isAspectConfigurationHost() {
return false;
}

@Override
public Label getLabel() {
return targetLabel;
}

@Override
public String getDescription() {
// Skylark aspects are referred to on command line with <file>%<value ame>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -38,8 +40,9 @@ class ConfiguredTargetCycleReporter extends AbstractLabelCycleReporter {

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);
Expand Down Expand Up @@ -97,21 +100,22 @@ 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();
}
}

@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());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,36 +69,41 @@ public CyclesReporter(SingleCycleReporter... cycleReporters) {
*/
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;
}
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();
}
Expand Down
10 changes: 10 additions & 0 deletions src/test/shell/integration/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
92 changes: 92 additions & 0 deletions src/test/shell/integration/aspect_test.sh
Original file line number Diff line number Diff line change
@@ -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"

0 comments on commit 30825c2

Please sign in to comment.