From 4dca905533c30badfaf25388a5fa226ed276edb8 Mon Sep 17 00:00:00 2001
From: Chenchu Kolli <ckolli@google.com>
Date: Mon, 9 May 2022 19:42:58 -0500
Subject: [PATCH] Improve error message for unresolved toolchains (#15451)

Closes #15135.

PiperOrigin-RevId: 447028054

Co-authored-by: Christopher Peterson Sauer <christophersauer@pacbell.net>
---
 .../skyframe/ToolchainResolutionFunction.java |  21 +-
 .../ToolchainResolutionFunctionTest.java      | 190 ++++++++++++++++++
 src/test/shell/bazel/toolchain_test.sh        |  16 +-
 3 files changed, 205 insertions(+), 22 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunction.java
index f579293b6f484a..2122cad3af7fe9 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunction.java
@@ -24,7 +24,6 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSetMultimap;
-import com.google.common.collect.Iterables;
 import com.google.common.collect.Table;
 import com.google.devtools.build.lib.analysis.PlatformConfiguration;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
@@ -578,20 +577,14 @@ protected Code getDetailedCode() {
     }
 
     private static String getMessage(List<Label> missingToolchainTypes) {
-      if (missingToolchainTypes.size() == 1
-          && Iterables.getOnlyElement(missingToolchainTypes)
-              .toString()
-              .equals("@bazel_tools//tools/cpp:toolchain_type")) {
-        return "No matching toolchains found for types @bazel_tools//tools/cpp:toolchain_type. "
-            + "Maybe --incompatible_use_cc_configure_from_rules_cc has been flipped and there "
-            + "is no default C++ toolchain added in the WORKSPACE file? See "
-            + "https://github.com/bazelbuild/bazel/issues/10134 for details and migration "
-            + "instructions.";
-      }
-
+      ImmutableList<String> labelStrings =
+          missingToolchainTypes.stream().map(Label::toString).collect(toImmutableList());
       return String.format(
-          "no matching toolchains found for types %s",
-          missingToolchainTypes.stream().map(Label::toString).collect(joining(", ")));
+          "No matching toolchains found for types %s."
+              + "\nTo debug, rerun with --toolchain_resolution_debug='%s'"
+              + "\nIf platforms or toolchains are a new concept for you, we'd encourage reading "
+              + "https://bazel.build/concepts/platforms-intro.",
+          String.join(", ", labelStrings), String.join("|", labelStrings));
     }
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunctionTest.java
index 3b9dae64139bf7..ad5f79f870cee9 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunctionTest.java
@@ -85,6 +85,196 @@ public void resolve() throws Exception {
     assertThat(unloadedToolchainContext).hasTargetPlatform("//platforms:linux");
   }
 
+  // TODO(katre): Add further tests for optional/mandatory/mixed toolchains.
+
+  @Test
+  public void resolve_optional() throws Exception {
+    // This should select platform mac, toolchain extra_toolchain_mac, because platform
+    // mac is listed first.
+    addToolchain(
+        "extra",
+        "extra_toolchain_linux",
+        ImmutableList.of("//constraints:linux"),
+        ImmutableList.of("//constraints:linux"),
+        "baz");
+    addToolchain(
+        "extra",
+        "extra_toolchain_mac",
+        ImmutableList.of("//constraints:mac"),
+        ImmutableList.of("//constraints:linux"),
+        "baz");
+    rewriteWorkspace(
+        "register_toolchains('//extra:extra_toolchain_linux', '//extra:extra_toolchain_mac')",
+        "register_execution_platforms('//platforms:mac', '//platforms:linux')");
+
+    useConfiguration("--platforms=//platforms:linux");
+    ToolchainContextKey key =
+        ToolchainContextKey.key()
+            .configurationKey(targetConfigKey)
+            .toolchainTypes(testToolchainType)
+            .build();
+
+    EvaluationResult<UnloadedToolchainContext> result = invokeToolchainResolution(key);
+
+    assertThatEvaluationResult(result).hasNoError();
+    UnloadedToolchainContext unloadedToolchainContext = result.get(key);
+    assertThat(unloadedToolchainContext).isNotNull();
+
+    assertThat(unloadedToolchainContext).hasToolchainType(testToolchainTypeLabel);
+    assertThat(unloadedToolchainContext).hasResolvedToolchain("//extra:extra_toolchain_mac_impl");
+    assertThat(unloadedToolchainContext).hasExecutionPlatform("//platforms:mac");
+    assertThat(unloadedToolchainContext).hasTargetPlatform("//platforms:linux");
+  }
+
+  @Test
+  public void resolve_multiple() throws Exception {
+    Label secondToolchainTypeLabel = Label.parseAbsoluteUnchecked("//second:toolchain_type");
+    ToolchainTypeRequirement secondToolchainTypeRequirement =
+        ToolchainTypeRequirement.create(secondToolchainTypeLabel);
+    ToolchainTypeInfo secondToolchainTypeInfo = ToolchainTypeInfo.create(secondToolchainTypeLabel);
+    scratch.file("second/BUILD", "toolchain_type(name = 'toolchain_type')");
+
+    addToolchain(
+        "main",
+        "main_toolchain_linux",
+        ImmutableList.of("//constraints:linux"),
+        ImmutableList.of("//constraints:linux"),
+        "baz");
+    addToolchain(
+        "main",
+        "second_toolchain_linux",
+        secondToolchainTypeLabel,
+        ImmutableList.of("//constraints:linux"),
+        ImmutableList.of("//constraints:linux"),
+        "baz");
+    rewriteWorkspace(
+        "register_toolchains('//main:all',)", "register_execution_platforms('//platforms:linux')");
+
+    useConfiguration("--platforms=//platforms:linux");
+    ToolchainContextKey key =
+        ToolchainContextKey.key()
+            .configurationKey(targetConfigKey)
+            .toolchainTypes(testToolchainType, secondToolchainTypeRequirement)
+            .build();
+
+    EvaluationResult<UnloadedToolchainContext> result = invokeToolchainResolution(key);
+
+    assertThatEvaluationResult(result).hasNoError();
+    UnloadedToolchainContext unloadedToolchainContext = result.get(key);
+    assertThat(unloadedToolchainContext).isNotNull();
+
+    assertThat(unloadedToolchainContext).hasToolchainType(testToolchainTypeLabel);
+    assertThat(unloadedToolchainContext).hasResolvedToolchain("//main:main_toolchain_linux_impl");
+    assertThat(unloadedToolchainContext).hasToolchainType(secondToolchainTypeLabel);
+    assertThat(unloadedToolchainContext).hasResolvedToolchain("//main:second_toolchain_linux_impl");
+    assertThat(unloadedToolchainContext).hasExecutionPlatform("//platforms:linux");
+    assertThat(unloadedToolchainContext).hasTargetPlatform("//platforms:linux");
+  }
+
+  @Test
+  public void resolve_mandatory_missing() throws Exception {
+    // There is no toolchain for the requested type.
+    useConfiguration("--platforms=//platforms:linux");
+    ToolchainContextKey key =
+        ToolchainContextKey.key()
+            .configurationKey(targetConfigKey)
+            .toolchainTypes(testToolchainType)
+            .build();
+
+    EvaluationResult<UnloadedToolchainContext> result = invokeToolchainResolution(key);
+
+    assertThatEvaluationResult(result)
+        .hasErrorEntryForKeyThat(key)
+        .hasExceptionThat()
+        .hasMessageThat()
+        .contains("No matching toolchains found for types //toolchain:test_toolchain");
+  }
+
+  @Test
+  public void resolve_multiple_optional() throws Exception {
+    Label secondToolchainTypeLabel = Label.parseAbsoluteUnchecked("//second:toolchain_type");
+    ToolchainTypeRequirement secondToolchainTypeRequirement =
+        ToolchainTypeRequirement.builder(secondToolchainTypeLabel).mandatory(false).build();
+    ToolchainTypeInfo secondToolchainTypeInfo = ToolchainTypeInfo.create(secondToolchainTypeLabel);
+    scratch.file("second/BUILD", "toolchain_type(name = 'toolchain_type')");
+
+    addToolchain(
+        "main",
+        "main_toolchain_linux",
+        ImmutableList.of("//constraints:linux"),
+        ImmutableList.of("//constraints:linux"),
+        "baz");
+    addToolchain(
+        "main",
+        "second_toolchain_linux",
+        secondToolchainTypeLabel,
+        ImmutableList.of("//constraints:linux"),
+        ImmutableList.of("//constraints:linux"),
+        "baz");
+    rewriteWorkspace(
+        "register_toolchains('//main:all',)", "register_execution_platforms('//platforms:linux')");
+
+    useConfiguration("--platforms=//platforms:linux");
+    ToolchainContextKey key =
+        ToolchainContextKey.key()
+            .configurationKey(targetConfigKey)
+            .toolchainTypes(testToolchainType, secondToolchainTypeRequirement)
+            .build();
+
+    EvaluationResult<UnloadedToolchainContext> result = invokeToolchainResolution(key);
+
+    assertThatEvaluationResult(result).hasNoError();
+    UnloadedToolchainContext unloadedToolchainContext = result.get(key);
+    assertThat(unloadedToolchainContext).isNotNull();
+
+    assertThat(unloadedToolchainContext).hasToolchainType(testToolchainTypeLabel);
+    assertThat(unloadedToolchainContext).hasResolvedToolchain("//main:main_toolchain_linux_impl");
+    assertThat(unloadedToolchainContext).hasToolchainType(secondToolchainTypeLabel);
+    assertThat(unloadedToolchainContext).hasResolvedToolchain("//main:second_toolchain_linux_impl");
+    assertThat(unloadedToolchainContext).hasExecutionPlatform("//platforms:linux");
+    assertThat(unloadedToolchainContext).hasTargetPlatform("//platforms:linux");
+  }
+
+  @Test
+  public void resolve_multiple_optional_missing() throws Exception {
+    Label secondToolchainTypeLabel = Label.parseAbsoluteUnchecked("//second:toolchain_type");
+    ToolchainTypeRequirement secondToolchainTypeRequirement =
+        ToolchainTypeRequirement.builder(secondToolchainTypeLabel).mandatory(false).build();
+    ToolchainTypeInfo secondToolchainTypeInfo = ToolchainTypeInfo.create(secondToolchainTypeLabel);
+    scratch.file("second/BUILD", "toolchain_type(name = 'toolchain_type')");
+
+    addToolchain(
+        "main",
+        "main_toolchain_linux",
+        ImmutableList.of("//constraints:linux"),
+        ImmutableList.of("//constraints:linux"),
+        "baz");
+    rewriteWorkspace(
+        "register_toolchains('//main:all',)", "register_execution_platforms('//platforms:linux')");
+
+    useConfiguration("--platforms=//platforms:linux");
+    ToolchainContextKey key =
+        ToolchainContextKey.key()
+            .configurationKey(targetConfigKey)
+            .toolchainTypes(testToolchainType, secondToolchainTypeRequirement)
+            .build();
+
+    EvaluationResult<UnloadedToolchainContext> result = invokeToolchainResolution(key);
+
+    assertThatEvaluationResult(result).hasNoError();
+    UnloadedToolchainContext unloadedToolchainContext = result.get(key);
+    assertThat(unloadedToolchainContext).isNotNull();
+
+    assertThat(unloadedToolchainContext).hasToolchainType(testToolchainTypeLabel);
+    assertThat(unloadedToolchainContext).hasResolvedToolchain("//main:main_toolchain_linux_impl");
+    assertThat(unloadedToolchainContext).hasToolchainType(secondToolchainTypeLabel);
+    assertThat(unloadedToolchainContext)
+        .resolvedToolchainLabels()
+        .doesNotContain(Label.parseAbsoluteUnchecked("//main:second_toolchain_linux_impl"));
+    assertThat(unloadedToolchainContext).hasExecutionPlatform("//platforms:linux");
+    assertThat(unloadedToolchainContext).hasTargetPlatform("//platforms:linux");
+  }
+
   @Test
   public void resolve_toolchainTypeAlias() throws Exception {
     addToolchain(
diff --git a/src/test/shell/bazel/toolchain_test.sh b/src/test/shell/bazel/toolchain_test.sh
index 3fce2118140d65..0c0be82ed3a9e3 100755
--- a/src/test/shell/bazel/toolchain_test.sh
+++ b/src/test/shell/bazel/toolchain_test.sh
@@ -386,8 +386,8 @@ use_toolchain(
     message = 'this is the rule')
 EOF
 
-  bazel build //demo:use &> $TEST_log && fail "Build failure expected"
-  expect_log 'While resolving toolchains for target //demo:use: no matching toolchains found for types //toolchain:test_toolchain'
+  bazel build "//${pkg}/demo:use" &> $TEST_log && fail "Build failure expected"
+  expect_log "While resolving toolchains for target //${pkg}/demo:use: No matching toolchains found for types //${pkg}/toolchain:test_toolchain."
 }
 
 function test_multiple_toolchain_use_in_rule {
@@ -474,8 +474,8 @@ use_toolchains(
     message = 'this is the rule')
 EOF
 
-  bazel build //demo:use &> $TEST_log && fail "Build failure expected"
-  expect_log 'While resolving toolchains for target //demo:use: no matching toolchains found for types //toolchain:test_toolchain_2'
+  bazel build "//${pkg}/demo:use" &> $TEST_log && fail "Build failure expected"
+  expect_log "While resolving toolchains for target //${pkg}/demo:use: No matching toolchains found for types //${pkg}/toolchain:test_toolchain_2."
 }
 
 function test_toolchain_use_in_rule_non_required_toolchain {
@@ -721,10 +721,10 @@ EOF
 
   # This should not match any toolchains.
   bazel build \
-    --host_platform=//:platform1 \
-    --platforms=//:platform1 \
-    //demo:use &> $TEST_log && fail "Build failure expected"
-  expect_log 'While resolving toolchains for target //demo:use: no matching toolchains found for types //toolchain:test_toolchain'
+    --host_platform="//${pkg}:platform1" \
+    --platforms="//${pkg}:platform1" \
+    "//${pkg}/demo:use" &> $TEST_log && fail "Build failure expected"
+  expect_log "While resolving toolchains for target //${pkg}/demo:use: No matching toolchains found for types //${pkg}/toolchain:test_toolchain."
   expect_not_log 'Using toolchain: rule message:'
 }