Have the RemoteSpawnRunner use the execution platform present in the Spawn to get the remote execution properties.
Fixes #4128.
This reverts commit 3ce42ef3074ee6d3ac7d9968381c8c0a51d9d38d.
Change-Id: I8b9ad5099f6334c2488a22baf05d0b273e10f776
PiperOrigin-RevId: 181550828
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 a3aa9f7..7f7e65f 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
@@ -55,6 +55,19 @@
)
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(
@@ -138,6 +151,9 @@
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 a5f29fc..bfee814 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 @@
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 8ac275e..51c3384 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 @@
"//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 4b257d7..b9b1f55 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.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 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 @@
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 9cf68d1..e8af197 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.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 AbstractRemoteActionCache remoteCache;
private final String buildRequestId;
@@ -78,7 +75,6 @@
DigestUtil digestUtil) {
this.execRoot = execRoot;
this.options = options;
- this.platform = options.parseRemotePlatformOverride();
this.remoteCache = remoteCache;
this.verboseFailures = verboseFailures;
this.cmdlineReporter = cmdlineReporter;
@@ -103,7 +99,7 @@
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 b8a1080..e0ba1fb 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.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;
@@ -45,6 +47,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;
@@ -69,8 +73,6 @@
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;
@@ -97,7 +99,6 @@
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 @@
spawn.getOutputFiles(),
digestUtil.compute(command),
repository.getMerkleDigest(inputRoot),
- platform,
+ spawn.getExecutionPlatform(),
policy.getTimeout(),
Spawns.mayBeCached(spawn));
@@ -263,9 +264,10 @@
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);
@@ -282,9 +284,14 @@
Collections.sort(outputPaths);
Collections.sort(outputDirectoryPaths);
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()));
}
@@ -294,6 +301,21 @@
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 99cc90b..07196bb 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 @@
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 @@
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;
