Fix non-deterministic Blaze crash with "--aspects -k".
The crash is:
java.lang.ClassCastException: class com.google.devtools.build.lib.skyframe.AspectValue$AspectKey cannot be cast to class com.google.devtools.build.lib.skyframe.ConfiguredTargetKey (com.google.devtools.build.lib.skyframe.AspectValue$AspectKey and com.google.devtools.build.lib.skyframe.ConfiguredTargetKey are in unnamed module of loader 'app')
at com.google.devtools.build.lib.skyframe.ConfiguredTargetCycleReporter.asTransitiveTargetKey(ConfiguredTargetCycleReporter.java:94)
...
This only works with the following magical combination of features:
1) Building with a top-level aspect (--aspects).
2) Building -keep_going (-k).
3) The rule you're building or one of its transitive deps is a genquery.
4) The genquery hits a loading phase cycle.
5) Actually building whatever the genquery visits doesn't hit a cycle because a select() makes the cycle go away in the actual build.
While this sounds incomprehensible, the basic story is this crash only happens in a pure loading phase cycle based from TransitiveTargetValue. genquery is the only rule that uses TransitiveTargetValue. Its "scope = " attribute normally guarantees whatever rule has the cycle will first be analyzed and hit the cycle during configured target evaluation, which skips this code path (you'll see the nice non-crashy message "This cycle occurred because of a configuration option" in those cases). But select()able deps short-circuit this: The select() means the cycle *doesn't* appear in analysis because the build's flags happen not to trigger whatever dependency path would trigger the cycle. So "scope = " finishes just fine. Then genquery gets into its rule-specific analysis and calls TransitiveTargetValue, which doesn't know which build flags are set so chooses *all* deps, including the bad ones. And we get a "loading-phase only" error, which propagates up to the ConfiguredTargetCycleReporter.
Even *this* is normally fine: ConfiguredTargetCycleReporter already has logic to handle loading phase errors. But that logic assumes the top-level analyzing unit was a configured target, not an aspect. Bringing in --aspects breaks that assumption.
Code details:
This causes both a ConfiguredTarget and Aspect to be requested as top-level nodes at:
https://github.com/bazelbuild/bazel/blob/b2e1fe367269103e127718131163cd702cefcdff/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java#L403-L404
-keep_going guarantees both the aspect and configured target are evaluated in Skyframe. Both will return null on a cycle
error. Both have their error recorded at
https://github.com/bazelbuild/bazel/blob/f998f4e8b5c172286074f4de58eb3fb966ed8243/src/main/java/com/google/devtools/build/skyframe/EvaluationResult.java#L171
which stores them in a HashMap<SkyKey, ErrorInfo>:
https://github.com/bazelbuild/bazel/blob/f998f4e8b5c172286074f4de58eb3fb966ed8243/src/main/java/com/google/devtools/build/skyframe/EvaluationResult.java#L157
which is then non-deterministically iterated over for cycle detection in:
https://github.com/bazelbuild/bazel/blob/3740768a2f625e7b3310f69bcec47b5dd0898cf1/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java#L527.
If the top-level build request includes a configured target and aspect, that means we're randomly going to get one of [ConfiguredTargetKey, AspectKey] first.
If AspectKey comes first, then https://github.com/bazelbuild/bazel/blob/209847415ac251d0b73e98e705bcb1a411490a2e/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetCycleReporter.java#L94 crashes.
If it comes second, that code path is skipped because of
https://github.com/bazelbuild/bazel/blob/6505f7c5db239a823e8503dcc6798702b2d54519/src/main/java/com/google/devtools/build/lib/skyframe/AbstractLabelCycleReporter.java#L85.
I have no idea how to test this, given how deeply this depends on Skyframe's evaluation model.
========================================================================
Minimal repro:
$ mkdir testapp
$ cat >testapp/defs.bzl <<EOF
def _rule_impl(ctx):
pass
custom_rule = rule(
implementation = _rule_impl,
attrs = {
"deps": attr.label_list(),
},
)
AspectProvider = provider()
def _aspect_impl(target, ctx):
pass
my_aspect = aspect(
implementation = _aspect_impl,
attr_aspects = ["deps"],
)
EOF
$ cat >testapp/BUILD <<EOF
load("//testapp:defs.bzl", "custom_rule")
config_setting(
name = "not_invoked",
define_values = {"dont_set_me": "1"},
)
custom_rule(
name = "my_rule",
deps = [":maybe_cycle_dep"],
)
custom_rule(
name = "maybe_cycle_dep",
deps = select({
"not_invoked": [":my_rule"],
"//conditions:default": [],
}),
)
genquery(
name = "my_query",
expression = "(//testapp:my_rule)",
scope = ["//testapp:my_rule"],
)
EOF
$ blaze build //testapp:my_query --aspects=//testapp:defs.bzl%my_aspect -k
Because of the non-determinism, this may or may not crash Blaze.
PiperOrigin-RevId: 277807980
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 fe73b63..24b01e8 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
@@ -20,7 +20,7 @@
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.actions.ActionLookupValue.ActionLookupKey;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.pkgcache.PackageProvider;
@@ -89,9 +89,13 @@
}
private SkyKey asTransitiveTargetKey(SkyKey key) {
- return IS_TRANSITIVE_TARGET_SKY_KEY.apply(key)
- ? key
- : TransitiveTargetKey.of(((ConfiguredTargetKey) key.argument()).getLabel());
+ if (IS_TRANSITIVE_TARGET_SKY_KEY.apply(key)) {
+ return key;
+ } else if (key.argument() instanceof ActionLookupKey) {
+ return TransitiveTargetKey.of(((ActionLookupKey) key.argument()).getLabel());
+ } else {
+ throw new IllegalArgumentException("Unknown type: " + key);
+ }
}
@Override
@@ -109,9 +113,8 @@
@Override
public Label getLabel(SkyKey key) {
- if (key instanceof ActionLookupValue.ActionLookupKey) {
- return Preconditions.checkNotNull(
- ((ActionLookupValue.ActionLookupKey) key.argument()).getLabel(), key);
+ if (key instanceof ActionLookupKey) {
+ return Preconditions.checkNotNull(((ActionLookupKey) key.argument()).getLabel(), key);
} else if (SkyFunctions.isSkyFunction(TRANSITIVE_TARGET).apply(key)) {
return ((TransitiveTargetKey) key).getLabel();
} else {
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetCycleReporterTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetCycleReporterTest.java
new file mode 100644
index 0000000..e455350
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetCycleReporterTest.java
@@ -0,0 +1,86 @@
+// 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.
+package com.google.devtools.build.lib.skyframe;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
+import com.google.devtools.build.lib.skyframe.AspectValue.AspectKey;
+import com.google.devtools.build.skyframe.CycleInfo;
+import com.google.devtools.build.skyframe.SkyKey;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Tests for {@link ConfiguredTargetCycleReporter}. */
+@RunWith(JUnit4.class)
+public class ConfiguredTargetCycleReporterTest extends BuildViewTestCase {
+
+ /**
+ * Regression test for b/142966884 : Blaze crashes when building with --aspects and --keep_going
+ * on a target where the transitive deps have a genquery which finds a cycle over //foo:c that
+ * doesn't happen when actually building //foo:c because of a select() on its deps that skips the
+ * path that happens to make the cycle.
+ *
+ * <p>That results in top-level keys that aren't {@link ConfiguredTargetKey} in {@link
+ * ConfiguredTargetCycleReporter#getAdditionalMessageAboutCycle}.
+ */
+ @Test
+ public void loadingPhaseCycleWithDifferentTopLevelKeyTypes() throws Exception {
+ scratch.file(
+ "foo/BUILD",
+ "genrule(name = 'a', srcs = [], outs = ['a.o'], cmd = 'echo uh > $@')",
+ "genrule(name = 'b', srcs = [], outs = ['b.o'], cmd = 'echo hi > $@', visibility = [':c'])",
+ "genrule(name = 'c', srcs = [], outs = ['c.o'], cmd = 'echo hi > $@')");
+ ConfiguredTargetCycleReporter cycleReporter =
+ new ConfiguredTargetCycleReporter(getPackageManager());
+ CycleInfo cycle =
+ new CycleInfo(
+ ImmutableList.of(
+ TransitiveTargetKey.of(makeLabel("//foo:b")),
+ TransitiveTargetKey.of(makeLabel("//foo:c"))));
+
+ ConfiguredTargetKey ctKey = ConfiguredTargetKey.of(makeLabel("//foo:a"), targetConfig);
+ assertThat(cycleReporter.getAdditionalMessageAboutCycle(reporter, ctKey, cycle))
+ .contains(
+ "The cycle is caused by a visibility edge from //foo:b to the non-package-group "
+ + "target //foo:c");
+
+ SkyKey aspectKey =
+ AspectKey.createAspectKey(
+ makeLabel("//foo:a"),
+ ctKey,
+ ImmutableList.of(),
+ null,
+ BuildConfigurationValue.key(targetConfig),
+ /*aspectConfigurationIsHost=*/ false);
+ assertThat(cycleReporter.getAdditionalMessageAboutCycle(reporter, aspectKey, cycle))
+ .contains(
+ "The cycle is caused by a visibility edge from //foo:b to the non-package-group "
+ + "target //foo:c");
+
+ SkyKey starlarkAspectKey =
+ AspectValue.createSkylarkAspectKey(
+ makeLabel("//foo:a"),
+ targetConfig,
+ targetConfig,
+ makeLabel("//foo:b"),
+ "my Starlark key");
+ assertThat(cycleReporter.getAdditionalMessageAboutCycle(reporter, starlarkAspectKey, cycle))
+ .contains(
+ "The cycle is caused by a visibility edge from //foo:b to the non-package-group "
+ + "target //foo:c");
+ }
+}
