diff --git a/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java b/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java index 94576c64fa41c6..3089c4ff61bf3e 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java @@ -118,7 +118,31 @@ public String parseIfMatches(String tag) throws ValidationException { } /** If specified, the timeout of this action in seconds. Must be decimal integer. */ - public static final String TIMEOUT = "timeout"; + public static final ParseableRequirement TIMEOUT = + ParseableRequirement.create( + "timeout:", + Pattern.compile("timeout:(.+)"), + s -> { + Preconditions.checkNotNull(s); + + int value; + try { + value = Integer.parseInt(s); + } catch (NumberFormatException e) { + return "can't be parsed as an integer"; + } + + // De-and-reserialize & compare to only allow canonical integer formats. + if (!Integer.toString(value).equals(s)) { + return "must be in canonical format (e.g. '4' instead of '+04')"; + } + + if (value < 1) { + return "can't be zero or negative"; + } + + return null; + }); /** If an action would not successfully run other than on Darwin. */ public static final String REQUIRES_DARWIN = "requires-darwin"; diff --git a/src/main/java/com/google/devtools/build/lib/actions/Spawns.java b/src/main/java/com/google/devtools/build/lib/actions/Spawns.java index 428075539e5b41..34e5c11f69a6b3 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Spawns.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Spawns.java @@ -14,9 +14,12 @@ package com.google.devtools.build.lib.actions; +import com.google.devtools.build.lib.actions.ExecutionRequirements.ParseableRequirement.ValidationException; +import com.google.devtools.build.lib.actions.ExecutionRequirements.ParseableRequirement; import com.google.devtools.build.lib.server.FailureDetails; import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; import com.google.devtools.build.lib.server.FailureDetails.Spawn.Code; + import java.io.IOException; import java.time.Duration; import java.util.Map; @@ -152,21 +155,72 @@ public static Duration getTimeout(Spawn spawn) throws ExecException { * defaultTimeout, or {@code Duration.ZERO} if that is null. */ public static Duration getTimeout(Spawn spawn, Duration defaultTimeout) throws ExecException { - String timeoutStr = spawn.getExecutionInfo().get(ExecutionRequirements.TIMEOUT); - if (timeoutStr == null) { - return defaultTimeout == null ? Duration.ZERO : defaultTimeout; + Duration timeout = null; + + for (Map.Entry entry: spawn.getExecutionInfo().entrySet()) { + String value = getTimeoutValueFromExecutionInfoKeyOrValue(spawn,entry); + if (value != null) { + if (timeout != null) { + throw new UserExecException( + FailureDetail.newBuilder() + .setMessage("multiple timeout values specified") + .setSpawn(FailureDetails.Spawn.newBuilder().setCode(Code.DUPLICATE_TIMEOUT_TAGS)) + .build()); + } + timeout = parseTimeoutValue(value); + } + } + + if (timeout == null) { + timeout = defaultTimeout == null ? Duration.ZERO : defaultTimeout; } + return timeout; +} + +/** + * Retrieves the timeout value from the execution info key or value. + * Timeout may either be specified as a tag on format timeout: + * or as a key-value pair when specified as an execution_requirement. + */ +private static String getTimeoutValueFromExecutionInfoKeyOrValue(Spawn spawn, Map.Entry entry) throws ExecException { + ParseableRequirement requirement = ExecutionRequirements.TIMEOUT; + String value = null; + + if (entry.getKey().equals("timeout")) { + value = entry.getValue(); + } else { + try { + value = requirement.parseIfMatches(entry.getKey()); + } catch (ValidationException e) { + throw new UserExecException( + e, + FailureDetail.newBuilder() + .setMessage( + String.format( + "%s has a '%s' tag, but its value '%s' didn't pass validation: %s", + spawn.getTargetLabel(), requirement.userFriendlyName(), e.getTagValue(), e.getMessage())) + .setSpawn(FailureDetails.Spawn.newBuilder().setCode(Code.INVALID_TIMEOUT_TAG)) + .build()); + } + } + return value; +} + +/** + * Parses a timeout value from a string and returns it as a {@link Duration}. + */ +private static Duration parseTimeoutValue(String value) throws ExecException { try { - return Duration.ofSeconds(Integer.parseInt(timeoutStr)); + return Duration.ofSeconds(Integer.parseInt(value)); } catch (NumberFormatException e) { - throw new UserExecException( - e, - FailureDetail.newBuilder() - .setMessage("could not parse timeout") - .setSpawn(FailureDetails.Spawn.newBuilder().setCode(Code.INVALID_TIMEOUT)) - .build()); + throw new UserExecException( + e, + FailureDetail.newBuilder() + .setMessage("could not parse timeout") + .setSpawn(FailureDetails.Spawn.newBuilder().setCode(Code.INVALID_TIMEOUT)) + .build()); } - } +} /** * Returns whether a local {@link Spawn} runner implementation should prefetch the inputs before diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java index 4d2c212477e87f..61fa77c89ee396 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java @@ -119,7 +119,7 @@ public TestRunnerSpawn createTestRunnerSpawn( // for this. executionInfo.put(ExecutionRequirements.NO_CACHE, ""); } - executionInfo.put(ExecutionRequirements.TIMEOUT, "" + getTimeout(action).toSeconds()); + executionInfo.put("timeout", "" + getTimeout(action).toSeconds()); SimpleSpawn.LocalResourcesSupplier localResourcesSupplier = () -> diff --git a/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java b/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java index 55e084acfae340..926c4e06cf5c36 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java +++ b/src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java @@ -58,6 +58,7 @@ private static boolean legalExecInfoKeys(String tag) { || tag.startsWith("disable-") || tag.startsWith("cpu:") || tag.equals(ExecutionRequirements.LOCAL) + || tag.startsWith("timeout") || tag.equals(ExecutionRequirements.WORKER_KEY_MNEMONIC) || tag.startsWith("resources:"); } diff --git a/src/main/protobuf/failure_details.proto b/src/main/protobuf/failure_details.proto index 979f8cb717137f..91b519b6da8488 100644 --- a/src/main/protobuf/failure_details.proto +++ b/src/main/protobuf/failure_details.proto @@ -222,6 +222,8 @@ message Spawn { COMMAND_LINE_EXPANSION_FAILURE = 7 [(metadata) = { exit_code: 1 }]; EXEC_IO_EXCEPTION = 8 [(metadata) = { exit_code: 36 }]; INVALID_TIMEOUT = 9 [(metadata) = { exit_code: 1 }]; + DUPLICATE_TIMEOUT_TAGS = 16 [(metadata) = { exit_code: 1 }]; + INVALID_TIMEOUT_TAG = 17 [(metadata) = { exit_code: 1 }]; INVALID_REMOTE_EXECUTION_PROPERTIES = 10 [(metadata) = { exit_code: 1 }]; NO_USABLE_STRATEGY_FOUND = 11 [(metadata) = { exit_code: 1 }]; // TODO(b/138456686): this code should be deprecated when SpawnResult is diff --git a/src/test/java/com/google/devtools/build/lib/packages/TargetUtilsTest.java b/src/test/java/com/google/devtools/build/lib/packages/TargetUtilsTest.java index 5352474f57b985..9e752b6366cb05 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/TargetUtilsTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/TargetUtilsTest.java @@ -223,6 +223,19 @@ public void testExecutionInfo_withPrefixCpu() throws Exception { assertThat(execInfo).containsExactly("cpu:123", ""); } + @Test + public void testExecutionInfo_withPrefixTimeout() throws Exception { + scratch.file( + "tests/BUILD", + "load('//test_defs:foo_binary.bzl', 'foo_binary')", + "foo_binary(name = 'with-prefix-timeout', srcs=['sh.sh'], tags=['timeout:123', 'wrong-tag'])"); + + Rule withCpuPrefix = (Rule) getTarget("//tests:with-prefix-timeout"); + + Map execInfo = TargetUtils.getExecutionInfo(withCpuPrefix); + assertThat(execInfo).containsExactly("timeout:123", ""); + } + @Test public void testExecutionInfo_withLocalTag() throws Exception { scratch.file( diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java index ad270e0ebfe260..890c1817ef3db1 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java @@ -416,7 +416,7 @@ public void testCreateSpawnActionEnvAndExecInfo() throws Exception { " inputs = ruleContext.files.srcs,", " outputs = ruleContext.files.srcs,", " env = {'a' : 'b'},", - " execution_requirements = {'timeout' : '10', 'block-network' : 'foo'},", + " execution_requirements = {'block-network' : 'foo'},", " mnemonic = 'DummyMnemonic',", " command = 'dummy_command',", " progress_message = 'dummy_message')"); @@ -425,10 +425,31 @@ public void testCreateSpawnActionEnvAndExecInfo() throws Exception { Iterables.getOnlyElement( ruleContext.getRuleContext().getAnalysisEnvironment().getRegisteredActions()); assertThat(action.getIncompleteEnvironmentForTesting()).containsExactly("a", "b"); - // We expect "timeout" to be filtered by TargetUtils. assertThat(action.getExecutionInfo()).containsExactly("block-network", "foo"); } + @Test + public void testCreateSpawnActionEnvAndExecInfo_withTimeout() throws Exception { + StarlarkRuleContext ruleContext = createRuleContext("//foo:foo"); + setRuleContext(ruleContext); + ev.exec( + "ruleContext.actions.run_shell(", + " inputs = ruleContext.files.srcs,", + " outputs = ruleContext.files.srcs,", + " execution_requirements = {", + " 'timeout': '42',", + " },", + " mnemonic = 'DummyMnemonic',", + " command = 'dummy_command',", + " progress_message = 'dummy_message')"); + SpawnAction action = + (SpawnAction) + Iterables.getOnlyElement( + ruleContext.getRuleContext().getAnalysisEnvironment().getRegisteredActions()); + assertThat(action.getExecutionInfo()) + .containsExactly("timeout", "42"); + } + @Test public void testCreateSpawnActionEnvAndExecInfo_withWorkerKeyMnemonic() throws Exception { StarlarkRuleContext ruleContext = createRuleContext("//foo:foo");