Have the RemoteSpawnRunner use the execution platform present in the Spawn to get the remote execution properties.

Fixes #4128.

Change-Id: I7e71caef2465204d2dd8225448d54e52366807e6
PiperOrigin-RevId: 179595126

7 files changed, 76 insertions, 45 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/PlatformOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/PlatformOptions.java
index 445f8e7684..c1d0877254 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/PlatformOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/PlatformOptions.java
@@ -45,6 +45,19 @@ public class PlatformOptions extends FragmentOptions {
   )
   public Label hostPlatform;
 
+  @Option(
+    name = "host_platform_remote_properties_override",
+    oldName = "experimental_remote_platform_override",
+    defaultValue = "null",
+    category = "remote",
+    documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+    effectTags = {OptionEffectTag.UNKNOWN},
+    help =
+        "Manually set the remote_execution_properties for the host platform"
+            + " if it is not already set."
+  )
+  public String hostPlatformRemotePropertiesOverride;
+
   // TODO(katre): Add execution platforms.
 
   @Option(
@@ -115,6 +128,9 @@ public class PlatformOptions extends FragmentOptions {
     host.hostPlatform = this.hostPlatform;
     host.extraToolchains = this.extraToolchains;
     host.enabledToolchainTypes = this.enabledToolchainTypes;
+    host.hostPlatformRemotePropertiesOverride = this.hostPlatformRemotePropertiesOverride;
+    host.toolchainResolutionDebug = this.toolchainResolutionDebug;
+    host.toolchainResolutionOverrides = this.toolchainResolutionOverrides;
     return host;
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformInfo.java b/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformInfo.java
index a5f29fc4f4..bfee814b6c 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformInfo.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformInfo.java
@@ -208,6 +208,11 @@ public class PlatformInfo extends NativeInfo {
       return this;
     }
 
+    /** Returns the data being sent to a potential remote executor. */
+    public String getRemoteExecutionProperties() {
+      return remoteExecutionProperties;
+    }
+
     /**
      * Sets the data being sent to a potential remote executor.
      *
diff --git a/src/main/java/com/google/devtools/build/lib/remote/BUILD b/src/main/java/com/google/devtools/build/lib/remote/BUILD
index 8ac275eea1..51c33841ae 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/remote/BUILD
@@ -22,6 +22,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib:runtime",
         "//src/main/java/com/google/devtools/build/lib:util",
         "//src/main/java/com/google/devtools/build/lib/actions",
+        "//src/main/java/com/google/devtools/build/lib/analysis/platform",
         "//src/main/java/com/google/devtools/build/lib/authandtls",
         "//src/main/java/com/google/devtools/build/lib/buildeventstream",
         "//src/main/java/com/google/devtools/build/lib/concurrent",
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteOptions.java
index 4b257d7ac4..b9b1f55d79 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteOptions.java
@@ -20,9 +20,6 @@ import com.google.devtools.common.options.Option;
 import com.google.devtools.common.options.OptionDocumentationCategory;
 import com.google.devtools.common.options.OptionEffectTag;
 import com.google.devtools.common.options.OptionsBase;
-import com.google.devtools.remoteexecution.v1test.Platform;
-import com.google.protobuf.TextFormat;
-import com.google.protobuf.TextFormat.ParseException;
 
 /** Options for remote execution and distributed caching. */
 public final class RemoteOptions extends OptionsBase {
@@ -113,16 +110,6 @@ public final class RemoteOptions extends OptionsBase {
   public boolean remoteUploadLocalResults;
 
   @Option(
-    name = "experimental_remote_platform_override",
-    defaultValue = "null",
-    category = "remote",
-    documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
-    effectTags = {OptionEffectTag.UNKNOWN},
-    help = "Temporary, for testing only. Manually set a Platform to pass to remote execution."
-  )
-  public String experimentalRemotePlatformOverride;
-
-  @Option(
     name = "remote_instance_name",
     defaultValue = "",
     category = "remote",
@@ -224,19 +211,4 @@ public final class RemoteOptions extends OptionsBase {
     help = "A file path to a local disk cache."
   )
   public PathFragment experimentalLocalDiskCachePath;
-
-  public Platform parseRemotePlatformOverride() {
-    if (experimentalRemotePlatformOverride != null) {
-      Platform.Builder platformBuilder = Platform.newBuilder();
-      try {
-        TextFormat.getParser().merge(experimentalRemotePlatformOverride, platformBuilder);
-      } catch (ParseException e) {
-        throw new IllegalArgumentException(
-            "Failed to parse --experimental_remote_platform_override", e);
-      }
-      return platformBuilder.build();
-    } else {
-      return null;
-    }
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
index 91e27cd49c..b0487a7cc9 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
@@ -32,7 +32,6 @@ import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.remoteexecution.v1test.Action;
 import com.google.devtools.remoteexecution.v1test.ActionResult;
 import com.google.devtools.remoteexecution.v1test.Command;
-import com.google.devtools.remoteexecution.v1test.Platform;
 import io.grpc.Context;
 import java.io.IOException;
 import java.util.Collection;
@@ -52,8 +51,6 @@ import javax.annotation.Nullable;
 final class RemoteSpawnCache implements SpawnCache {
   private final Path execRoot;
   private final RemoteOptions options;
-  // TODO(olaola): This will be set on a per-action basis instead.
-  private final Platform platform;
 
   private final RemoteActionCache remoteCache;
   private final String buildRequestId;
@@ -78,7 +75,6 @@ final class RemoteSpawnCache implements SpawnCache {
       DigestUtil digestUtil) {
     this.execRoot = execRoot;
     this.options = options;
-    this.platform = options.parseRemotePlatformOverride();
     this.remoteCache = remoteCache;
     this.verboseFailures = verboseFailures;
     this.cmdlineReporter = cmdlineReporter;
@@ -102,7 +98,7 @@ final class RemoteSpawnCache implements SpawnCache {
             spawn.getOutputFiles(),
             digestUtil.compute(command),
             repository.getMerkleDigest(inputRoot),
-            platform,
+            spawn.getExecutionPlatform(),
             policy.getTimeout(),
             Spawns.mayBeCached(spawn));
 
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
index c63975738b..695f6ea3bf 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
@@ -26,6 +26,8 @@ import com.google.devtools.build.lib.actions.SpawnResult;
 import com.google.devtools.build.lib.actions.SpawnResult.Status;
 import com.google.devtools.build.lib.actions.Spawns;
 import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
+import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
+import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.Reporter;
@@ -46,6 +48,8 @@ import com.google.devtools.remoteexecution.v1test.Digest;
 import com.google.devtools.remoteexecution.v1test.ExecuteRequest;
 import com.google.devtools.remoteexecution.v1test.ExecuteResponse;
 import com.google.devtools.remoteexecution.v1test.Platform;
+import com.google.protobuf.TextFormat;
+import com.google.protobuf.TextFormat.ParseException;
 import io.grpc.Context;
 import io.grpc.Status.Code;
 import java.io.IOException;
@@ -70,8 +74,6 @@ class RemoteSpawnRunner implements SpawnRunner {
 
   private final Path execRoot;
   private final RemoteOptions options;
-  // TODO(olaola): This will be set on a per-action basis instead.
-  private final Platform platform;
   private final SpawnRunner fallbackRunner;
   private final boolean verboseFailures;
 
@@ -98,7 +100,6 @@ class RemoteSpawnRunner implements SpawnRunner {
       DigestUtil digestUtil) {
     this.execRoot = execRoot;
     this.options = options;
-    this.platform = options.parseRemotePlatformOverride();
     this.fallbackRunner = fallbackRunner;
     this.remoteCache = remoteCache;
     this.remoteExecutor = remoteExecutor;
@@ -129,7 +130,7 @@ class RemoteSpawnRunner implements SpawnRunner {
             spawn.getOutputFiles(),
             digestUtil.compute(command),
             repository.getMerkleDigest(inputRoot),
-            platform,
+            spawn.getExecutionPlatform(),
             policy.getTimeout(),
             Spawns.mayBeCached(spawn));
 
@@ -262,9 +263,10 @@ class RemoteSpawnRunner implements SpawnRunner {
       Collection<? extends ActionInput> outputs,
       Digest command,
       Digest inputRoot,
-      Platform platform,
+      @Nullable PlatformInfo executionPlatform,
       Duration timeout,
       boolean cacheable) {
+
     Action.Builder action = Action.newBuilder();
     action.setCommandDigest(command);
     action.setInputRootDigest(inputRoot);
@@ -275,9 +277,14 @@ class RemoteSpawnRunner implements SpawnRunner {
     Collections.sort(outputPaths);
     // TODO: output directories should be handled here, when they are supported.
     action.addAllOutputFiles(outputPaths);
-    if (platform != null) {
+
+    // Get the remote platform properties.
+    if (executionPlatform != null) {
+      Platform platform =
+          parsePlatform(executionPlatform.label(), executionPlatform.remoteExecutionProperties());
       action.setPlatform(platform);
     }
+
     if (!timeout.isZero()) {
       action.setTimeout(com.google.protobuf.Duration.newBuilder().setSeconds(timeout.getSeconds()));
     }
@@ -287,6 +294,21 @@ class RemoteSpawnRunner implements SpawnRunner {
     return action.build();
   }
 
+  static Platform parsePlatform(Label platformLabel, @Nullable String platformDescription) {
+    Platform.Builder platformBuilder = Platform.newBuilder();
+    try {
+      if (platformDescription != null) {
+        TextFormat.getParser().merge(platformDescription, platformBuilder);
+      }
+    } catch (ParseException e) {
+      throw new IllegalArgumentException(
+          String.format(
+              "Failed to parse remote_execution_properties from platform %s", platformLabel),
+          e);
+    }
+    return platformBuilder.build();
+  }
+
   static Command buildCommand(List<String> arguments, ImmutableMap<String, String> env) {
     Command.Builder command = Command.newBuilder();
     command.addAllArguments(arguments);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/platform/Platform.java b/src/main/java/com/google/devtools/build/lib/rules/platform/Platform.java
index 99cc90b436..07196bb41d 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/platform/Platform.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/platform/Platform.java
@@ -17,6 +17,7 @@ package com.google.devtools.build.lib.rules.platform;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.FileProvider;
 import com.google.devtools.build.lib.analysis.FilesToRunProvider;
+import com.google.devtools.build.lib.analysis.PlatformOptions;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTargetFactory;
 import com.google.devtools.build.lib.analysis.RuleContext;
@@ -39,22 +40,40 @@ public class Platform implements RuleConfiguredTargetFactory {
 
     PlatformInfo.Builder platformBuilder = PlatformInfo.builder().setLabel(ruleContext.getLabel());
 
-    if (ruleContext.attributes().get(PlatformRule.HOST_PLATFORM_ATTR, Type.BOOLEAN)) {
+    Boolean isHostPlatform =
+        ruleContext.attributes().get(PlatformRule.HOST_PLATFORM_ATTR, Type.BOOLEAN);
+    Boolean isTargetPlatform =
+        ruleContext.attributes().get(PlatformRule.TARGET_PLATFORM_ATTR, Type.BOOLEAN);
+    if (isHostPlatform && isTargetPlatform) {
+      ruleContext.attributeError(
+          PlatformRule.HOST_PLATFORM_ATTR,
+          "A single platform cannot have both host_platform and target_platform set.");
+      return null;
+    } else if (isHostPlatform) {
       // Create default constraints based on the current host OS and CPU values.
       String cpuOption = ruleContext.getConfiguration().getHostCpu();
       autodetectConstraints(cpuOption, ruleContext, platformBuilder);
-    } else if (ruleContext.attributes().get(PlatformRule.TARGET_PLATFORM_ATTR, Type.BOOLEAN)) {
+    } else if (isTargetPlatform) {
       // Create default constraints based on the current OS and CPU values.
       String cpuOption = ruleContext.getConfiguration().getCpu();
       autodetectConstraints(cpuOption, ruleContext, platformBuilder);
-    } else {
-      platformBuilder.addConstraints(
-          PlatformProviderUtils.constraintValues(
-              ruleContext.getPrerequisites(PlatformRule.CONSTRAINT_VALUES_ATTR, Mode.DONT_CHECK)));
     }
 
+    // Add the declared constraints. Because setting the host_platform or target_platform attribute
+    // to true on a platform automatically includes the detected CPU and OS constraints, if the
+    // constraint_values attribute tries to add those, this will throw an error.
+    platformBuilder.addConstraints(
+        PlatformProviderUtils.constraintValues(
+            ruleContext.getPrerequisites(PlatformRule.CONSTRAINT_VALUES_ATTR, Mode.DONT_CHECK)));
+
     String remoteExecutionProperties =
         ruleContext.attributes().get(PlatformRule.REMOTE_EXECUTION_PROPS_ATTR, Type.STRING);
+    if (platformBuilder.getRemoteExecutionProperties() == null && isHostPlatform) {
+      // Use the default override.
+      PlatformOptions platformOptions =
+          ruleContext.getConfiguration().getOptions().get(PlatformOptions.class);
+      remoteExecutionProperties = platformOptions.hostPlatformRemotePropertiesOverride;
+    }
     platformBuilder.setRemoteExecutionProperties(remoteExecutionProperties);
 
     PlatformInfo platformInfo;