diff options
author | John Cater <jcater@google.com> | 2018-01-10 19:01:14 -0800 |
---|---|---|
committer | Copybara-Service <copybara-piper@google.com> | 2018-01-10 19:03:02 -0800 |
commit | 38ba193c60c027157b55088a7aebb76ef293caf2 (patch) | |
tree | 69512d1a1161b637c7cbf3d0164f8bf95904379b /src/main/java/com | |
parent | 9694cae29752d205edcf45cb89e37f440eda6b0d (diff) |
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
Diffstat (limited to 'src/main/java/com')
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 a3aa9f7f81..7f7e65ff60 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 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( @@ -138,6 +151,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 9cf68d114c..e8af197907 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 AbstractRemoteActionCache 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; @@ -103,7 +99,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 b8a1080c7b..e0ba1fb4f4 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; @@ -45,6 +47,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; @@ -69,8 +73,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; @@ -97,7 +99,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)); @@ -263,9 +264,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); @@ -282,9 +284,14 @@ class RemoteSpawnRunner implements SpawnRunner { 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 @@ 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; |