)]}'
{
  "commit": "68e4319600d8c68c4142c99e730a5732d85fd2d3",
  "tree": "6eac9f295332c48fa613256dcdc30efa1e8f9dce",
  "parents": [
    "9c5f8508d4bf378cb17069262918ca6775095223"
  ],
  "author": {
    "name": "gregce",
    "email": "gregce@google.com",
    "time": "Thu Oct 31 15:00:10 2019 -0700"
  },
  "committer": {
    "name": "Copybara-Service",
    "email": "copybara-worker@google.com",
    "time": "Thu Oct 31 15:01:10 2019 -0700"
  },
  "message": "Fix non-deterministic Blaze crash with \"--aspects -k\".\n\nThe crash is:\n\njava.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 \u0027app\u0027)\n\tat com.google.devtools.build.lib.skyframe.ConfiguredTargetCycleReporter.asTransitiveTargetKey(ConfiguredTargetCycleReporter.java:94)\n...\n\nThis only works with the following magical combination of features:\n\n 1) Building with a top-level aspect (--aspects).\n 2) Building -keep_going (-k).\n 3) The rule you\u0027re building or one of its transitive deps is a genquery.\n 4) The genquery hits a loading phase cycle.\n 5) Actually building whatever the genquery visits doesn\u0027t hit a cycle because a select() makes the cycle go away in the actual build.\n\nWhile 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 \u003d \" 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\u0027ll 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\u0027t* appear in analysis because the build\u0027s flags happen not to trigger whatever dependency path would trigger the cycle. So \"scope \u003d \" finishes just fine. Then genquery gets into its rule-specific analysis and calls TransitiveTargetValue, which doesn\u0027t 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.\n\nEven *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.\n\nCode details:\n\nThis causes both a ConfiguredTarget and Aspect to be requested as top-level nodes at:\n\nhttps://github.com/bazelbuild/bazel/blob/b2e1fe367269103e127718131163cd702cefcdff/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java#L403-L404\n\n-keep_going guarantees both the aspect and configured target are evaluated in Skyframe. Both will return null on a cycle\nerror. Both have their error recorded at\n\nhttps://github.com/bazelbuild/bazel/blob/f998f4e8b5c172286074f4de58eb3fb966ed8243/src/main/java/com/google/devtools/build/skyframe/EvaluationResult.java#L171\n\nwhich stores them in a HashMap\u003cSkyKey, ErrorInfo\u003e:\n\nhttps://github.com/bazelbuild/bazel/blob/f998f4e8b5c172286074f4de58eb3fb966ed8243/src/main/java/com/google/devtools/build/skyframe/EvaluationResult.java#L157\n\nwhich is then non-deterministically iterated over for cycle detection in:\n\nhttps://github.com/bazelbuild/bazel/blob/3740768a2f625e7b3310f69bcec47b5dd0898cf1/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java#L527.\n\nIf the top-level build request includes a configured target and aspect, that means we\u0027re randomly going to get one of [ConfiguredTargetKey, AspectKey] first.\n\nIf AspectKey comes first, then https://github.com/bazelbuild/bazel/blob/209847415ac251d0b73e98e705bcb1a411490a2e/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetCycleReporter.java#L94 crashes.\n\nIf it comes second, that code path is skipped because of\nhttps://github.com/bazelbuild/bazel/blob/6505f7c5db239a823e8503dcc6798702b2d54519/src/main/java/com/google/devtools/build/lib/skyframe/AbstractLabelCycleReporter.java#L85.\n\nI have no idea how to test this, given how deeply this depends on Skyframe\u0027s evaluation model.\n\n\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\n\nMinimal repro:\n\n$ mkdir testapp\n$ cat \u003etestapp/defs.bzl \u003c\u003cEOF\ndef _rule_impl(ctx):\n    pass\n\ncustom_rule \u003d rule(\n    implementation \u003d _rule_impl,\n    attrs \u003d {\n        \"deps\": attr.label_list(),\n    },\n)\n\nAspectProvider \u003d provider()\n\ndef _aspect_impl(target, ctx):\n    pass\n\nmy_aspect \u003d aspect(\n    implementation \u003d _aspect_impl,\n    attr_aspects \u003d [\"deps\"],\n)\nEOF\n$ cat \u003etestapp/BUILD \u003c\u003cEOF\nload(\"//testapp:defs.bzl\", \"custom_rule\")\n\nconfig_setting(\n    name \u003d \"not_invoked\",\n    define_values \u003d {\"dont_set_me\": \"1\"},\n)\n\ncustom_rule(\n    name \u003d \"my_rule\",\n    deps \u003d [\":maybe_cycle_dep\"],\n)\n\ncustom_rule(\n    name \u003d \"maybe_cycle_dep\",\n    deps \u003d select({\n        \"not_invoked\": [\":my_rule\"],\n        \"//conditions:default\": [],\n    }),\n)\n\ngenquery(\n    name \u003d \"my_query\",\n    expression \u003d \"(//testapp:my_rule)\",\n    scope \u003d [\"//testapp:my_rule\"],\n)\n\nEOF\n\n$ blaze build //testapp:my_query --aspects\u003d//testapp:defs.bzl%my_aspect -k\n\nBecause of the non-determinism, this may or may not crash Blaze.\n\nPiperOrigin-RevId: 277807980\n",
  "tree_diff": [
    {
      "type": "modify",
      "old_id": "fe73b63c33a8970cd076392ac450b155f22994cf",
      "old_mode": 33188,
      "old_path": "src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetCycleReporter.java",
      "new_id": "24b01e86e911db6e167435fdc3f6c743c6c7b574",
      "new_mode": 33188,
      "new_path": "src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetCycleReporter.java"
    },
    {
      "type": "add",
      "old_id": "0000000000000000000000000000000000000000",
      "old_mode": 0,
      "old_path": "/dev/null",
      "new_id": "e455350eab769bec160bc0cc0fd669de495efd9b",
      "new_mode": 33188,
      "new_path": "src/test/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetCycleReporterTest.java"
    }
  ]
}