Always run toolchain resolution, even when no toolchain types are requested, in order to properly choose the execution platform from the available execution platforms.

Change-Id: I05dc84403e0db765865e9b91c4222894fa867cd9
PiperOrigin-RevId: 200211635

2 files changed, 245 insertions, 39 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainUtil.java b/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainUtil.java
index 39149f54e4..b5f363158a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ToolchainUtil.java
@@ -49,6 +49,7 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.stream.Collectors;
 import javax.annotation.Nullable;
 
 /**
@@ -59,8 +60,19 @@ import javax.annotation.Nullable;
 public class ToolchainUtil {
 
   /**
-   * Returns a new {@link ToolchainContext}, with the correct toolchain labels based on the results
-   * of the {@link ToolchainResolutionFunction}.
+   * Returns a new {@link ToolchainContext}, containing:
+   *
+   * <ul>
+   *   <li>If {@code requiredToolchains} was non-empty, the resolved toolchains and execution
+   *       platform (as labels), based on the results of the {@link ToolchainResolutionFunction}
+   *   <li>If {@code requiredToolchains} was empty:
+   *       <ul>
+   *         <li>The resolved toolchains will be empty.
+   *         <li>The execution platform will be the host platform, if the host platform was in the
+   *             set of available execution platforms.
+   *         <li>Otherwise, the execution platform will be the first available execution platform.
+   *       </ul>
+   * </ul>
    *
    * @param env the Skyframe environment to use to acquire dependencies
    * @param targetDescription a description of the target use, for error and debug message context
@@ -126,42 +138,33 @@ public class ToolchainUtil {
             .build();
 
     // Filter out execution platforms that don't satisfy the extra constraints.
+    boolean debug = configuration.getOptions().get(PlatformOptions.class).toolchainResolutionDebug;
     availableExecutionPlatformKeys =
-        filterPlatforms(availableExecutionPlatformKeys, execConstraintKeys, env);
+        filterPlatforms(availableExecutionPlatformKeys, execConstraintKeys, env, debug);
     if (availableExecutionPlatformKeys == null) {
       return null;
     }
 
-    Optional<ResolvedToolchains> resolvedToolchains =
+    ResolvedToolchains resolvedToolchains =
         resolveToolchainLabels(
             env,
             requiredToolchains,
-            configuration,
             configurationKey,
+            hostPlatformKey,
             availableExecutionPlatformKeys,
-            targetPlatformKey);
+            targetPlatformKey,
+            debug);
     if (resolvedToolchains == null) {
       return null;
     }
 
-    if (resolvedToolchains.isPresent()) {
-      return createContext(
-          env,
-          targetDescription,
-          resolvedToolchains.get().executionPlatformKey(),
-          resolvedToolchains.get().targetPlatformKey(),
-          requiredToolchains,
-          resolvedToolchains.get().toolchains());
-    } else {
-      // No toolchain could be resolved, but no error happened, so fall back to host platform.
-      return createContext(
-          env,
-          targetDescription,
-          hostPlatformKey,
-          targetPlatformKey,
-          requiredToolchains,
-          ImmutableBiMap.of());
-    }
+    return createContext(
+        env,
+        targetDescription,
+        resolvedToolchains.executionPlatformKey(),
+        resolvedToolchains.targetPlatformKey(),
+        requiredToolchains,
+        resolvedToolchains.toolchains());
   }
 
   private static RegisteredExecutionPlatformsValue loadRegisteredExecutionPlatforms(
@@ -253,20 +256,16 @@ public class ToolchainUtil {
   }
 
   @Nullable
-  private static Optional<ResolvedToolchains> resolveToolchainLabels(
+  private static ResolvedToolchains resolveToolchainLabels(
       Environment env,
       Set<Label> requiredToolchains,
-      BuildConfiguration configuration,
       BuildConfigurationValue.Key configurationKey,
+      ConfiguredTargetKey hostPlatformKey,
       ImmutableList<ConfiguredTargetKey> availableExecutionPlatformKeys,
-      ConfiguredTargetKey targetPlatformKey)
+      ConfiguredTargetKey targetPlatformKey,
+      boolean debug)
       throws InterruptedException, ToolchainContextException {
 
-    // If there are no required toolchains, bail out early.
-    if (requiredToolchains.isEmpty()) {
-      return Optional.absent();
-    }
-
     // Find the toolchains for the required toolchain types.
     List<ToolchainResolutionValue.Key> registeredToolchainKeys = new ArrayList<>();
     for (Label toolchainType : requiredToolchains) {
@@ -333,9 +332,37 @@ public class ToolchainUtil {
       return null;
     }
 
-    boolean debug = configuration.getOptions().get(PlatformOptions.class).toolchainResolutionDebug;
-
     // Find and return the first execution platform which has all required toolchains.
+    Optional<ConfiguredTargetKey> selectedExecutionPlatformKey;
+    if (requiredToolchains.isEmpty() && availableExecutionPlatformKeys.contains(hostPlatformKey)) {
+      // Fall back to the legacy behavior: use the host platform if it's available, otherwise the
+      // first execution platform.
+      selectedExecutionPlatformKey = Optional.of(hostPlatformKey);
+    } else {
+      // If there are no toolchains, this will return the first execution platform.
+      selectedExecutionPlatformKey =
+          findExecutionPlatformForToolchains(
+              env, requiredToolchains, availableExecutionPlatformKeys, resolvedToolchains, debug);
+    }
+
+    if (!selectedExecutionPlatformKey.isPresent()) {
+      throw new ToolchainContextException(
+          new NoMatchingPlatformException(
+              requiredToolchains, availableExecutionPlatformKeys, targetPlatformKey));
+    }
+
+    return ResolvedToolchains.create(
+        selectedExecutionPlatformKey.get(),
+        targetPlatformKey,
+        resolvedToolchains.row(selectedExecutionPlatformKey.get()));
+  }
+
+  private static Optional<ConfiguredTargetKey> findExecutionPlatformForToolchains(
+      Environment env,
+      Set<Label> requiredToolchains,
+      ImmutableList<ConfiguredTargetKey> availableExecutionPlatformKeys,
+      Table<ConfiguredTargetKey, Label, Label> resolvedToolchains,
+      boolean debug) {
     for (ConfiguredTargetKey executionPlatformKey : availableExecutionPlatformKeys) {
       // PlatformInfo executionPlatform = platforms.get(executionPlatformKey);
       Map<Label, Label> toolchains = resolvedToolchains.row(executionPlatformKey);
@@ -360,8 +387,7 @@ public class ToolchainUtil {
                                         "type %s -> toolchain %s", e.getKey(), e.getValue()))
                             .collect(joining(", ")))));
       }
-      return Optional.of(
-          ResolvedToolchains.create(executionPlatformKey, targetPlatformKey, toolchains));
+      return Optional.of(executionPlatformKey);
     }
 
     return Optional.absent();
@@ -449,7 +475,8 @@ public class ToolchainUtil {
   private static ImmutableList<ConfiguredTargetKey> filterPlatforms(
       ImmutableList<ConfiguredTargetKey> platformKeys,
       ImmutableList<ConfiguredTargetKey> constraintKeys,
-      Environment env)
+      Environment env,
+      boolean debug)
       throws ToolchainContextException, InterruptedException {
 
     // Short circuit if not needed.
@@ -468,7 +495,7 @@ public class ToolchainUtil {
 
     return platformKeys
         .stream()
-        .filter(key -> filterPlatform(platformInfoMap.get(key), constraints))
+        .filter(key -> filterPlatform(platformInfoMap.get(key), constraints, env, debug))
         .collect(toImmutableList());
   }
 
@@ -519,13 +546,25 @@ public class ToolchainUtil {
   }
 
   private static boolean filterPlatform(
-      PlatformInfo platformInfo, List<ConstraintValueInfo> constraints) {
+      PlatformInfo platformInfo,
+      List<ConstraintValueInfo> constraints,
+      Environment env,
+      boolean debug) {
     for (ConstraintValueInfo filterConstraint : constraints) {
       ConstraintValueInfo platformInfoConstraint =
           platformInfo.getConstraint(filterConstraint.constraint());
       if (platformInfoConstraint == null || !platformInfoConstraint.equals(filterConstraint)) {
         // The value for this setting is not present in the platform, or doesn't match the expected
         // value.
+        if (debug) {
+          env.getListener()
+              .handle(
+                  Event.info(
+                      String.format(
+                          "ToolchainUtil: Removed execution platform %s from"
+                              + " available execution platforms, it is missing constraint %s",
+                          platformInfo.label(), filterConstraint.label())));
+        }
         return false;
       }
     }
@@ -533,6 +572,45 @@ public class ToolchainUtil {
     return true;
   }
 
+  /** Exception used when no execution platform can be found. */
+  static final class NoMatchingPlatformException extends Exception {
+    NoMatchingPlatformException() {
+      super("No available execution platform satisfies all requested toolchain types");
+    }
+
+    public NoMatchingPlatformException(
+        Set<Label> requiredToolchains,
+        ImmutableList<ConfiguredTargetKey> availableExecutionPlatformKeys,
+        ConfiguredTargetKey targetPlatformKey) {
+      super(formatError(requiredToolchains, availableExecutionPlatformKeys, targetPlatformKey));
+    }
+
+    private static String formatError(
+        Set<Label> requiredToolchains,
+        ImmutableList<ConfiguredTargetKey> availableExecutionPlatformKeys,
+        ConfiguredTargetKey targetPlatformKey) {
+      if (requiredToolchains.isEmpty()) {
+        return String.format(
+            "Unable to find an execution platform for target platform %s"
+                + " from available execution platforms [%s]",
+            targetPlatformKey.getLabel(),
+            availableExecutionPlatformKeys
+                .stream()
+                .map(key -> key.getLabel().toString())
+                .collect(Collectors.joining(", ")));
+      }
+      return String.format(
+          "Unable to find an execution platform for toolchains [%s] and target platform %s"
+              + " from available execution platforms [%s]",
+          Joiner.on(", ").join(requiredToolchains),
+          targetPlatformKey.getLabel(),
+          availableExecutionPlatformKeys
+              .stream()
+              .map(key -> key.getLabel().toString())
+              .collect(Collectors.joining(", ")));
+    }
+  }
+
   /**
    * Exception used when an error occurs in {@link #expandTargetPatterns(Environment, List,
    * FilteringPolicy)}.
@@ -609,6 +687,10 @@ public class ToolchainUtil {
 
   /** Exception used to wrap exceptions during toolchain resolution. */
   public static class ToolchainContextException extends Exception {
+    public ToolchainContextException(NoMatchingPlatformException e) {
+      super(e);
+    }
+
     public ToolchainContextException(InvalidPlatformException e) {
       super(e);
     }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainUtilTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainUtilTest.java
index 9c3d251cc1..b2175c640a 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainUtilTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainUtilTest.java
@@ -28,6 +28,7 @@ import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.rules.platform.ToolchainTestCase;
 import com.google.devtools.build.lib.skyframe.ToolchainUtil.InvalidConstraintValueException;
 import com.google.devtools.build.lib.skyframe.ToolchainUtil.InvalidPlatformException;
+import com.google.devtools.build.lib.skyframe.ToolchainUtil.NoMatchingPlatformException;
 import com.google.devtools.build.lib.skyframe.ToolchainUtil.ToolchainContextException;
 import com.google.devtools.build.lib.skyframe.ToolchainUtil.UnresolvedToolchainsException;
 import com.google.devtools.build.lib.skyframe.util.SkyframeExecutorTestUtils;
@@ -117,6 +118,77 @@ public class ToolchainUtilTest extends ToolchainTestCase {
   }
 
   @Test
+  public void createToolchainContext_noToolchainType() throws Exception {
+    scratch.file("host/BUILD", "platform(name = 'host')");
+    rewriteWorkspace("register_execution_platforms('//platforms:mac', '//platforms:linux')");
+
+    useConfiguration("--host_platform=//host:host", "--platforms=//platforms:linux");
+    CreateToolchainContextKey key =
+        CreateToolchainContextKey.create("test", ImmutableSet.of(), targetConfigKey);
+
+    EvaluationResult<CreateToolchainContextValue> result = createToolchainContext(key);
+
+    assertThatEvaluationResult(result).hasNoError();
+    ToolchainContext toolchainContext = result.get(key).toolchainContext();
+    assertThat(toolchainContext).isNotNull();
+
+    assertThat(toolchainContext.getRequiredToolchains()).isEmpty();
+
+    // With no toolchains requested, should fall back to the host platform.
+    assertThat(toolchainContext.getExecutionPlatform()).isNotNull();
+    assertThat(toolchainContext.getExecutionPlatform().label())
+        .isEqualTo(Label.parseAbsoluteUnchecked("//host:host"));
+
+    assertThat(toolchainContext.getTargetPlatform()).isNotNull();
+    assertThat(toolchainContext.getTargetPlatform().label())
+        .isEqualTo(Label.parseAbsoluteUnchecked("//platforms:linux"));
+  }
+
+  @Test
+  public void createToolchainContext_noToolchainType_hostNotAvailable() throws Exception {
+    scratch.file("host/BUILD", "platform(name = 'host')");
+    scratch.file(
+        "sample/BUILD",
+        "constraint_setting(name='demo')",
+        "constraint_value(name = 'demo_a', constraint_setting=':demo')",
+        "constraint_value(name = 'demo_b', constraint_setting=':demo')",
+        "platform(name = 'sample_a',",
+        "  constraint_values = [':demo_a'],",
+        ")",
+        "platform(name = 'sample_b',",
+        "  constraint_values = [':demo_b'],",
+        ")");
+    rewriteWorkspace(
+        "register_execution_platforms('//platforms:mac', '//platforms:linux',",
+        "    '//sample:sample_a', '//sample:sample_b')");
+
+    useConfiguration("--host_platform=//host:host", "--platforms=//platforms:linux");
+    CreateToolchainContextKey key =
+        CreateToolchainContextKey.create(
+            "test",
+            ImmutableSet.of(),
+            ImmutableSet.of(Label.parseAbsoluteUnchecked("//sample:demo_b")),
+            targetConfigKey);
+
+    EvaluationResult<CreateToolchainContextValue> result = createToolchainContext(key);
+
+    assertThatEvaluationResult(result).hasNoError();
+    ToolchainContext toolchainContext = result.get(key).toolchainContext();
+    assertThat(toolchainContext).isNotNull();
+
+    assertThat(toolchainContext.getRequiredToolchains()).isEmpty();
+
+    // With no toolchains requested, should fall back to the host platform.
+    assertThat(toolchainContext.getExecutionPlatform()).isNotNull();
+    assertThat(toolchainContext.getExecutionPlatform().label())
+        .isEqualTo(Label.parseAbsoluteUnchecked("//sample:sample_b"));
+
+    assertThat(toolchainContext.getTargetPlatform()).isNotNull();
+    assertThat(toolchainContext.getTargetPlatform().label())
+        .isEqualTo(Label.parseAbsoluteUnchecked("//platforms:linux"));
+  }
+
+  @Test
   public void createToolchainContext_unavailableToolchainType_single() throws Exception {
     useConfiguration(
         "--host_platform=//platforms:linux",
@@ -335,6 +407,58 @@ public class ToolchainUtilTest extends ToolchainTestCase {
         .contains("//platforms:linux");
   }
 
+  @Test
+  public void createToolchainContext_noMatchingPlatform() throws Exception {
+    // Write toolchain A, and a toolchain implementing it.
+    scratch.appendFile(
+        "a/BUILD",
+        "toolchain_type(name = 'toolchain_type_A')",
+        "toolchain(",
+        "    name = 'toolchain',",
+        "    toolchain_type = ':toolchain_type_A',",
+        "    exec_compatible_with = ['//constraints:mac'],",
+        "    target_compatible_with = [],",
+        "    toolchain = ':toolchain_impl')",
+        "filegroup(name='toolchain_impl')");
+    // Write toolchain B, and a toolchain implementing it.
+    scratch.appendFile(
+        "b/BUILD",
+        "load('//toolchain:toolchain_def.bzl', 'test_toolchain')",
+        "toolchain_type(name = 'toolchain_type_B')",
+        "toolchain(",
+        "    name = 'toolchain',",
+        "    toolchain_type = ':toolchain_type_B',",
+        "    exec_compatible_with = ['//constraints:linux'],",
+        "    target_compatible_with = [],",
+        "    toolchain = ':toolchain_impl')",
+        "filegroup(name='toolchain_impl')");
+
+    rewriteWorkspace(
+        "register_toolchains('//a:toolchain', '//b:toolchain')",
+        "register_execution_platforms('//platforms:mac', '//platforms:linux')");
+
+    useConfiguration("--platforms=//platforms:linux");
+    CreateToolchainContextKey key =
+        CreateToolchainContextKey.create(
+            "test",
+            ImmutableSet.of(
+                Label.parseAbsoluteUnchecked("//a:toolchain_type_A"),
+                Label.parseAbsoluteUnchecked("//b:toolchain_type_B")),
+            targetConfigKey);
+
+    EvaluationResult<CreateToolchainContextValue> result = createToolchainContext(key);
+    assertThatEvaluationResult(result).hasError();
+    assertThatEvaluationResult(result)
+        .hasErrorEntryForKeyThat(key)
+        .hasExceptionThat()
+        .isInstanceOf(ToolchainContextException.class);
+    assertThatEvaluationResult(result)
+        .hasErrorEntryForKeyThat(key)
+        .hasExceptionThat()
+        .hasCauseThat()
+        .isInstanceOf(NoMatchingPlatformException.class);
+  }
+
   // Calls ToolchainUtil.createToolchainContext.
   private static final SkyFunctionName CREATE_TOOLCHAIN_CONTEXT_FUNCTION =
       SkyFunctionName.create("CREATE_TOOLCHAIN_CONTEXT_FUNCTION");