Include ExecutionPlatformResolutionKey in cfg graph cycle detector.

Summary: ^

Reviewed By: JakobDegen

Differential Revision: D63337541

fbshipit-source-id: 42f53c1c9660cc936fff7111b054c26bd6ecb696

app/buck2_configured/BUCK

@@ -12,6 +12,7 @@ rust_library(
         "fbsource//third-party/rust:async-trait",
         "fbsource//third-party/rust:derive_more",
         "fbsource//third-party/rust:futures",
+        "fbsource//third-party/rust:itertools",
         "//buck2/allocative/allocative:allocative",
         "//buck2/app/buck2_build_api:buck2_build_api",
         "//buck2/app/buck2_common:buck2_common",

app/buck2_configured/Cargo.toml

@@ -10,6 +10,7 @@ anyhow = { workspace = true }
 async-trait = { workspace = true }
 derive_more = { workspace = true }
 futures = { workspace = true }
+itertools = { workspace = true }
 
 allocative = { workspace = true }
 dice = { workspace = true }

app/buck2_configured/src/calculation.rs

@@ -29,6 +29,7 @@ use dupe::Dupe;
 use gazebo::prelude::*;
 
 use crate::configuration::calculation::ConfigurationCalculation;
+use crate::configuration::calculation::ExecutionPlatformResolutionKey;
 use crate::nodes::calculation::get_execution_platform_toolchain_dep;
 use crate::nodes::calculation::ConfiguredTargetNodeKey;
 use crate::target::TargetConfiguredTargetLabel;
@@ -140,6 +141,8 @@ fn display_configured_graph_cycle_error(cycle: &[ConfiguredGraphCycleKeys]) -> S
 pub enum ConfiguredGraphCycleKeys {
     #[display("{}", _0)]
     ConfiguredTargetNode(ConfiguredTargetNodeKey),
+    #[display("{}", _0)]
+    ExecutionPlatformResolution(ExecutionPlatformResolutionKey),
 }
 
 #[derive(Debug)]
@@ -162,6 +165,12 @@ impl CycleAdapterDescriptor for ConfiguredGraphCycleDescriptor {
         if let Some(v) = key.downcast_ref::<ConfiguredTargetNodeKey>() {
             return Some(ConfiguredGraphCycleKeys::ConfiguredTargetNode(v.dupe()));
         }
+        if let Some(v) = key.downcast_ref::<ExecutionPlatformResolutionKey>() {
+            return Some(ConfiguredGraphCycleKeys::ExecutionPlatformResolution(
+                v.dupe(),
+            ));
+        }
+
         None
     }
 }

app/buck2_configured/src/configuration/calculation.rs

@@ -10,6 +10,7 @@
 use std::sync::Arc;
 use anyhow::Context;
 
+use itertools::Itertools;
 use allocative::Allocative;
 use async_trait::async_trait;use buck2_build_api::interpreter::rule_defs::provider::builtin::platform_info::FrozenPlatformInfo;
 use buck2_common::dice::cells::HasCellResolver;
@@ -644,41 +645,6 @@ impl ConfigurationCalculation for DiceComputations<'_> {
         exec_deps: Arc<[TargetLabel]>,
         toolchain_allows: Arc<[ToolchainConstraints]>,
     ) -> buck2_error::Result<ExecutionPlatformResolution> {
-        #[derive(Clone, Dupe, Display, Debug, Eq, Hash, PartialEq, Allocative)]
-        #[display("{:?}", self)]
-        struct ExecutionPlatformResolutionKey {
-            target_node_cell: CellName,
-            exec_compatible_with: Arc<[ConfigurationSettingKey]>,
-            exec_deps: Arc<[TargetLabel]>,
-            toolchain_allows: Arc<[ToolchainConstraints]>,
-        }
-
-        #[async_trait]
-        impl Key for ExecutionPlatformResolutionKey {
-            type Value = buck2_error::Result<ExecutionPlatformResolution>;
-
-            async fn compute(
-                &self,
-                ctx: &mut DiceComputations,
-                _cancellation: &CancellationContext,
-            ) -> Self::Value {
-                resolve_execution_platform_from_constraints(
-                    ctx,
-                    self.target_node_cell,
-                    &self.exec_compatible_with,
-                    &self.exec_deps,
-                    &self.toolchain_allows,
-                )
-                .await
-            }
-
-            fn equality(x: &Self::Value, y: &Self::Value) -> bool {
-                match (x, y) {
-                    (Ok(x), Ok(y)) => x == y,
-                    _ => false,
-                }
-            }
-        }
         self.compute(&ExecutionPlatformResolutionKey {
             target_node_cell,
             exec_compatible_with,
@@ -689,6 +655,83 @@ impl ConfigurationCalculation for DiceComputations<'_> {
     }
 }
 
+#[derive(Clone, Dupe, Debug, Eq, Hash, PartialEq, Allocative)]
+pub struct ExecutionPlatformResolutionKey {
+    target_node_cell: CellName,
+    exec_compatible_with: Arc<[ConfigurationSettingKey]>,
+    exec_deps: Arc<[TargetLabel]>,
+    toolchain_allows: Arc<[ToolchainConstraints]>,
+}
+
+impl Display for ExecutionPlatformResolutionKey {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(
+            f,
+            "Resolving execution platform: cell:{}",
+            self.target_node_cell
+        )?;
+
+        if !self.exec_compatible_with.is_empty() {
+            write!(
+                f,
+                ", exec_compatible_with=[{}]",
+                self.exec_compatible_with.iter().join(", ")
+            )?
+        }
+
+        if !self.exec_deps.is_empty() {
+            write!(f, ", exec_deps=[{}]", self.exec_deps.iter().join(", "))?
+        }
+
+        let mut iter = self
+            .toolchain_allows
+            .iter()
+            .flat_map(|v| v.exec_compatible_with())
+            .peekable();
+        if iter.peek().is_some() {
+            write!(f, ", toolchain_exec_compatible_with=[{}]", iter.join(", "))?
+        }
+
+        let mut iter = self
+            .toolchain_allows
+            .iter()
+            .flat_map(|v| v.exec_deps())
+            .peekable();
+        if iter.peek().is_some() {
+            write!(f, ", toolchain_exec_deps=[{}]", iter.join(", "))?
+        }
+
+        Ok(())
+    }
+}
+
+#[async_trait]
+impl Key for ExecutionPlatformResolutionKey {
+    type Value = buck2_error::Result<ExecutionPlatformResolution>;
+
+    async fn compute(
+        &self,
+        ctx: &mut DiceComputations,
+        _cancellation: &CancellationContext,
+    ) -> Self::Value {
+        resolve_execution_platform_from_constraints(
+            ctx,
+            self.target_node_cell,
+            &self.exec_compatible_with,
+            &self.exec_deps,
+            &self.toolchain_allows,
+        )
+        .await
+    }
+
+    fn equality(x: &Self::Value, y: &Self::Value) -> bool {
+        match (x, y) {
+            (Ok(x), Ok(y)) => x == y,
+            _ => false,
+        }
+    }
+}
+
 struct GetExecutionPlatformsInstance;
 
 #[async_trait]

tests/e2e/cycle_detection/test_cycle_detection.py

@@ -27,6 +27,16 @@ def check_cfg_graph_cycle_stderr(stderr: str) -> None:
     assert r"root//:cycle_top (<unspecified>) ->" in stderr
 
 
+def check_cfg_toolchain_graph_cycle_stderr(stderr: str) -> None:
+    # We're not sure which order these will appear.
+    assert r"root//:toolchain_cycle_top" in stderr
+    # toolchain_cycle_mid is the toolchain rule and it doesn't appear in the error. ideally we'd fix that, but
+    # for performance/memory reasons we aggregate the exec_deps out of toolchain rules.
+    # assert r"root//:toolchain_cycle_mid" in stderr
+    assert r"root//:toolchain_cycle_bot" in stderr
+    assert r"Resolving execution platform" in stderr
+
+
 @buck_test(inplace=False)
 async def test_detect_load_cycle(buck: Buck) -> None:
     failure = await expect_failure(
@@ -66,6 +76,18 @@ async def test_detect_configured_graph_cycles_on_recompute(buck: Buck) -> None:
     check_cfg_graph_cycle_stderr(failure.stderr)
 
 
+@buck_test(inplace=False)
+async def test_detect_configured_graph_cycles_2(buck: Buck) -> None:
+    failure = await expect_failure(
+        buck.cquery(
+            "//:top",
+            "-c",
+            "cycles.cfg_toolchain=yes",
+        ),
+    )
+    check_cfg_toolchain_graph_cycle_stderr(failure.stderr)
+
+
 @buck_test(inplace=False)
 async def test_more_recompute_cases(buck: Buck) -> None:
     await buck.cquery("//:top")

tests/e2e/cycle_detection/test_cycle_detection_data/.buckconfig

@@ -10,6 +10,7 @@ prelude = prelude
 
 [build]
   lazy_cycle_detector = true
+  execution_platforms = //:execution_platforms
 
 [buck2]
   detect_cycles = disabled

tests/e2e/cycle_detection/test_cycle_detection_data/TARGETS.fixture

@@ -1,11 +1,13 @@
-load("//defs.bzl", "suite")
+load("//defs.bzl", "execution_platforms", "suite", "toolchain")
 
 suite(
     name = "top",
     deps = [] + (
         ["//load_cycle:target"] if read_config("cycles", "load") == "yes" else []
     ) + (
         ["//:cycle_top"] if read_config("cycles", "cfg_graph") == "yes" else []
+    ) + (
+        ["//:toolchain_cycle_top"] if read_config("cycles", "cfg_toolchain") == "yes" else []
     ),
 )
 
@@ -29,3 +31,20 @@ suite(
         ":cycle_top",
     ],
 )
+
+suite(
+    name = "toolchain_cycle_top",
+    toolchain = ":toolchain_cycle_mid",
+)
+
+toolchain(
+    name = "toolchain_cycle_mid",
+    exec_deps = [":toolchain_cycle_bot"],
+)
+
+suite(
+    name = "toolchain_cycle_bot",
+    deps = [":toolchain_cycle_top"],
+)
+
+execution_platforms(name = "execution_platforms")

tests/e2e/cycle_detection/test_cycle_detection_data/defs.bzl

@@ -5,12 +5,40 @@
 # License, Version 2.0 found in the LICENSE-APACHE file in the root directory
 # of this source tree.
 
-def _suite_impl(_ctx):
+def _impl(_ctx):
     fail()
 
 suite = rule(
-    impl = _suite_impl,
+    impl = _impl,
     attrs = {
-        "deps": attrs.list(attrs.dep()),
+        "deps": attrs.list(attrs.dep(), default = []),
+        "toolchain": attrs.option(attrs.toolchain_dep(), default = None),
     },
 )
+
+toolchain = rule(
+    impl = _impl,
+    is_toolchain_rule = True,
+    attrs = {
+        "exec_deps": attrs.list(attrs.exec_dep()),
+    },
+)
+
+def exec_platforms_impl(ctx):
+    return [
+        DefaultInfo(),
+        ExecutionPlatformRegistrationInfo(
+            platforms = [
+                ExecutionPlatformInfo(
+                    label = ctx.label.raw_target(),
+                    configuration = ConfigurationInfo(constraints = {}, values = {}),
+                    executor_config = CommandExecutorConfig(local_enabled = True, remote_enabled = False),
+                ),
+            ],
+        ),
+    ]
+
+execution_platforms = rule(
+    impl = exec_platforms_impl,
+    attrs = {},
+)