Have the RemoteSpawnRunner use the execution platform present in the …

…Spawn to get the remote execution properties.

Fixes bazelbuild#4128.

Change-Id: I7e71caef2465204d2dd8225448d54e52366807e6
PiperOrigin-RevId: 179595126

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 PlatformOptions getHost() {
     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;
   }
 

src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformInfo.java

@@ -208,6 +208,11 @@ public Builder addConstraints(Iterable<ConstraintValueInfo> constraints) {
       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.
      *

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",

src/main/java/com/google/devtools/build/lib/remote/RemoteOptions.java

@@ -20,9 +20,6 @@
 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 {
@@ -112,16 +109,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 = "",
@@ -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;
-    }
-  }
 }

src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java

@@ -32,7 +32,6 @@
 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 @@
 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 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionPolicy policy)
             spawn.getOutputFiles(),
             digestUtil.compute(command),
             repository.getMerkleDigest(inputRoot),
-            platform,
+            spawn.getExecutionPlatform(),
             policy.getTimeout(),
             Spawns.mayBeCached(spawn));
 

src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java

@@ -26,6 +26,8 @@
 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.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 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionPolicy policy)
             spawn.getOutputFiles(),
             digestUtil.compute(command),
             repository.getMerkleDigest(inputRoot),
-            platform,
+            spawn.getExecutionPlatform(),
             policy.getTimeout(),
             Spawns.mayBeCached(spawn));
 
@@ -262,9 +263,10 @@ static Action buildAction(
       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 @@ static Action buildAction(
     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 @@ static Action buildAction(
     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);

src/main/java/com/google/devtools/build/lib/rules/platform/Platform.java

@@ -17,6 +17,7 @@
 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 ConfiguredTarget create(RuleContext ruleContext)
 
     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;