Return platform with all toolchains (including non-mandatory) when it…

… exists

With C++ toolchain optional, toolchain resolution prefers to select first execution platform which might be missing cc toolchain. Then the build fails.

PiperOrigin-RevId: 468626692
Change-Id: Iabfeef4035a3522358bd013f688a43bedd7bdee7

src/main/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunction.java

@@ -19,6 +19,7 @@
 
 import com.google.auto.value.AutoValue;
 import com.google.common.base.Preconditions;
+import com.google.common.base.Predicates;
 import com.google.common.collect.HashBasedTable;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -526,21 +527,32 @@ private static Optional<ConfiguredTargetKey> findExecutionPlatformForToolchains(
 
     if (forcedExecutionPlatform.isPresent()) {
       // Is the forced platform suitable?
-      if (isPlatformSuitable(forcedExecutionPlatform.get(), toolchainTypes, resolvedToolchains)) {
+      if (isPlatformSuitable(
+          forcedExecutionPlatform.get(), toolchainTypes, resolvedToolchains, true)) {
         return forcedExecutionPlatform;
       }
     }
 
+    // If there is one, choose the first execution platform that has all toolchains.
+    Optional<ConfiguredTargetKey> bestPlatform =
+        availableExecutionPlatformKeys.stream()
+            .filter(epk -> isPlatformSuitable(epk, toolchainTypes, resolvedToolchains, false))
+            .findFirst();
+    if (bestPlatform.isPresent()) {
+      return bestPlatform;
+    }
+
     // Choose the first execution platform that has all mandatory toolchains.
     return availableExecutionPlatformKeys.stream()
-        .filter(epk -> isPlatformSuitable(epk, toolchainTypes, resolvedToolchains))
+        .filter(epk -> isPlatformSuitable(epk, toolchainTypes, resolvedToolchains, true))
         .findFirst();
   }
 
   private static boolean isPlatformSuitable(
       ConfiguredTargetKey executionPlatformKey,
       ImmutableSet<ToolchainType> toolchainTypes,
-      Table<ConfiguredTargetKey, ToolchainTypeInfo, Label> resolvedToolchains) {
+      Table<ConfiguredTargetKey, ToolchainTypeInfo, Label> resolvedToolchains,
+      boolean onlyMandatory) {
     if (toolchainTypes.isEmpty()) {
       // Since there aren't any toolchains, we should be able to use any execution platform that
       // has made it this far.
@@ -553,7 +565,7 @@ private static boolean isPlatformSuitable(
         .keySet()
         .containsAll(
             toolchainTypes.stream()
-                .filter(ToolchainType::mandatory)
+                .filter(onlyMandatory ? ToolchainType::mandatory : Predicates.alwaysTrue())
                 .map(ToolchainType::toolchainTypeInfo)
                 .collect(toImmutableSet()));
   }

src/test/java/com/google/devtools/build/lib/analysis/testing/ToolchainContextSubject.java

@@ -107,6 +107,6 @@ public void hasResolvedToolchain(Label resolvedToolchain) {
   }
 
   public IterableSubject resolvedToolchainLabels() {
-    return check("resolevdToolchainLabels()").that(actual.resolvedToolchainLabels());
+    return check("resolvedToolchainLabels()").that(actual.resolvedToolchainLabels());
   }
 }

src/test/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunctionTest.java

@@ -93,13 +93,13 @@ public void resolve() throws Exception {
   public void resolve_optional() throws Exception {
     // This should select platform mac, toolchain extra_toolchain_mac, because platform
     // mac is listed first.
-    addToolchain(
+    addOptionalToolchain(
         "extra",
         "extra_toolchain_linux",
         ImmutableList.of("//constraints:linux"),
         ImmutableList.of("//constraints:linux"),
         "baz");
-    addToolchain(
+    addOptionalToolchain(
         "extra",
         "extra_toolchain_mac",
         ImmutableList.of("//constraints:mac"),
@@ -113,7 +113,7 @@ public void resolve_optional() throws Exception {
     ToolchainContextKey key =
         ToolchainContextKey.key()
             .configurationKey(targetConfigKey)
-            .toolchainTypes(testToolchainType)
+            .toolchainTypes(optionalToolchainType)
             .build();
 
     EvaluationResult<UnloadedToolchainContext> result = invokeToolchainResolution(key);
@@ -122,7 +122,71 @@ public void resolve_optional() throws Exception {
     UnloadedToolchainContext unloadedToolchainContext = result.get(key);
     assertThat(unloadedToolchainContext).isNotNull();
 
-    assertThat(unloadedToolchainContext).hasToolchainType(testToolchainTypeLabel);
+    assertThat(unloadedToolchainContext).hasToolchainType(optionalToolchainTypeLabel);
+    assertThat(unloadedToolchainContext).hasResolvedToolchain("//extra:extra_toolchain_mac_impl");
+    assertThat(unloadedToolchainContext).hasExecutionPlatform("//platforms:mac");
+    assertThat(unloadedToolchainContext).hasTargetPlatform("//platforms:linux");
+  }
+
+  @Test
+  public void resolve_optional_on_first_platform() throws Exception {
+    // This should select platform mac, toolchain extra_toolchain_mac, independent of platform order
+    addOptionalToolchain(
+        "extra",
+        "extra_toolchain_mac",
+        ImmutableList.of("//constraints:mac"),
+        ImmutableList.of("//constraints:linux"),
+        "baz");
+    rewriteWorkspace(
+        "register_toolchains('//extra:extra_toolchain_mac')",
+        "register_execution_platforms('//platforms:mac', '//platforms:linux')");
+
+    useConfiguration("--platforms=//platforms:linux");
+    ToolchainContextKey key =
+        ToolchainContextKey.key()
+            .configurationKey(targetConfigKey)
+            .toolchainTypes(optionalToolchainType)
+            .build();
+
+    EvaluationResult<UnloadedToolchainContext> result = invokeToolchainResolution(key);
+
+    assertThatEvaluationResult(result).hasNoError();
+    UnloadedToolchainContext unloadedToolchainContext = result.get(key);
+    assertThat(unloadedToolchainContext).isNotNull();
+
+    assertThat(unloadedToolchainContext).hasToolchainType(optionalToolchainTypeLabel);
+    assertThat(unloadedToolchainContext).hasResolvedToolchain("//extra:extra_toolchain_mac_impl");
+    assertThat(unloadedToolchainContext).hasExecutionPlatform("//platforms:mac");
+    assertThat(unloadedToolchainContext).hasTargetPlatform("//platforms:linux");
+  }
+
+  @Test
+  public void resolve_optional_on_second_platform() throws Exception {
+    // This should select platform mac, toolchain extra_toolchain_mac, independent of platform order
+    addOptionalToolchain(
+        "extra",
+        "extra_toolchain_mac",
+        ImmutableList.of("//constraints:mac"),
+        ImmutableList.of("//constraints:linux"),
+        "baz");
+    rewriteWorkspace(
+        "register_toolchains('//extra:extra_toolchain_mac')",
+        "register_execution_platforms('//platforms:linux', '//platforms:mac')");
+
+    useConfiguration("--platforms=//platforms:linux");
+    ToolchainContextKey key =
+        ToolchainContextKey.key()
+            .configurationKey(targetConfigKey)
+            .toolchainTypes(optionalToolchainType)
+            .build();
+
+    EvaluationResult<UnloadedToolchainContext> result = invokeToolchainResolution(key);
+
+    assertThatEvaluationResult(result).hasNoError();
+    UnloadedToolchainContext unloadedToolchainContext = result.get(key);
+    assertThat(unloadedToolchainContext).isNotNull();
+
+    assertThat(unloadedToolchainContext).hasToolchainType(optionalToolchainTypeLabel);
     assertThat(unloadedToolchainContext).hasResolvedToolchain("//extra:extra_toolchain_mac_impl");
     assertThat(unloadedToolchainContext).hasExecutionPlatform("//platforms:mac");
     assertThat(unloadedToolchainContext).hasTargetPlatform("//platforms:linux");

src/test/shell/integration/toolchain_test.sh

@@ -2594,10 +2594,9 @@ EOF
   bazel build \
     --toolchain_resolution_debug=.* \
     "//${pkg}/demo:use" &> $TEST_log || fail "Build failed"
-  # Verify that no toolchain was provided.
-  expect_log 'Using toolchain: rule message: "this is the rule", toolchain is none: True'
-  # Verify that the exec platform is platform1.
-  expect_log "Selected execution platform //${pkg}/platforms:platform1"
+
+  # Verify that the exec platform is platform2.
+  expect_log "Selected execution platform //${pkg}/platforms:platform2"
 }
 
 # TODO(katre): Test using toolchain-provided make variables from a genrule.