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 e63283913b8cbd..330dffa665e930 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 @@ -18,11 +18,23 @@ import com.google.common.base.Function; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableMap; +import com.google.devtools.build.lib.packages.Rule; import java.util.regex.Matcher; import java.util.regex.Pattern; /** * Strings used to express requirements on action execution environments. + * + *
    + * If you are adding a new execution requirement, pay attention to the following: + *
  1. If its name starts with one of the supported prefixes, then it can be also used as a tag on + * a target and will be propagated to the execution requirements, see for prefixes {@link + * com.google.devtools.build.lib.packages.TargetUtils#getExecutionInfo(Rule)} + *
  2. If this is a potentially conflicting execution requirements, e.g. you are adding a pair + * 'requires-x' and 'block-x', you MUST take care of a potential conflict in the Executor that + * is using new execution requirements. As an example, see {@link + * Spawns#requiresNetwork(com.google.devtools.build.lib.actions.Spawn, boolean)}. + *
*/ public class ExecutionRequirements { diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java index 29b7e5bc711df8..4954b6b7cdb587 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java @@ -558,15 +558,11 @@ private void registerStarlarkAction( if (EvalUtils.toBoolean(useDefaultShellEnv)) { builder.useDefaultShellEnvironment(); } - if (executionRequirementsUnchecked != Runtime.NONE) { - builder.setExecutionInfo( - TargetUtils.filter( - SkylarkDict.castSkylarkDictOrNoneToDict( - executionRequirementsUnchecked, - String.class, - String.class, - "execution_requirements"))); - } + + ImmutableMap executionInfo = + TargetUtils.getFilteredExecutionInfo(executionRequirementsUnchecked, ruleContext.getRule()); + builder.setExecutionInfo(executionInfo); + if (inputManifestsUnchecked != Runtime.NONE) { for (RunfilesSupplier supplier : SkylarkList.castSkylarkListOrNoneToList( inputManifestsUnchecked, RunfilesSupplier.class, "runfiles suppliers")) { 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 acb035268a6054..23ba8e07930f7d 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 @@ -24,6 +24,8 @@ import com.google.common.collect.Maps; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.events.Location; +import com.google.devtools.build.lib.syntax.EvalException; +import com.google.devtools.build.lib.syntax.SkylarkDict; import com.google.devtools.build.lib.syntax.Type; import com.google.devtools.build.lib.util.Pair; import java.util.ArrayList; @@ -46,18 +48,15 @@ public final class TargetUtils { // some internal tags that we don't allow to be set on targets. We also don't want to // exhaustively enumerate all the legal values here. Right now, only a ~small set of tags is // recognized by Bazel. - private static final Predicate LEGAL_EXEC_INFO_KEYS = new Predicate() { - @Override - public boolean apply(String tag) { - return tag.startsWith("block-") - || tag.startsWith("requires-") - || tag.startsWith("no-") - || tag.startsWith("supports-") - || tag.startsWith("disable-") - || tag.equals("local") - || tag.startsWith("cpu:"); - } - }; + private static boolean legalExecInfoKeys(String tag) { + return tag.startsWith("block-") + || tag.startsWith("requires-") + || tag.startsWith("no-") + || tag.startsWith("supports-") + || tag.startsWith("disable-") + || tag.equals("local") + || tag.startsWith("cpu:"); + } private TargetUtils() {} // Uninstantiable. @@ -220,31 +219,57 @@ private static boolean hasConstraint(Rule rule, String keyword) { } /** - * Returns the execution info. These include execution requirement tags ('block-*', 'requires-*', - * 'no-*', 'supports-*', 'disable-*', 'local', and 'cpu:*') as keys with empty values. + * Returns the execution info from the tags declared on the target. These include only some tags + * {@link #LEGAL_EXEC_INFO_KEYS} as keys with empty values. */ public static Map getExecutionInfo(Rule rule) { // tags may contain duplicate values. Map map = new HashMap<>(); for (String tag : NonconfigurableAttributeMapper.of(rule).get(CONSTRAINTS_ATTR, Type.STRING_LIST)) { - // We don't want to pollute the execution info with random things, and we also need to reserve - // some internal tags that we don't allow to be set on targets. We also don't want to - // exhaustively enumerate all the legal values here. Right now, only a ~small set of tags is - // recognized by Bazel. - if (LEGAL_EXEC_INFO_KEYS.apply(tag)) { + if (legalExecInfoKeys(tag)) { map.put(tag, ""); } } return ImmutableMap.copyOf(map); } + /** + * Returns the execution info, obtained from the rule's tags and the execution requirements + * provided. Only supported tags are included into the execution info, see {@link + * #LEGAL_EXEC_INFO_KEYS}. + * + * @param executionRequirementsUnchecked execution_requirements of a rule, expected to be of a + * {@code SkylarkDict} type, null or {@link + * com.google.devtools.build.lib.syntax.Runtime#NONE} + * @param rule a rule instance to get tags from + */ + public static ImmutableMap getFilteredExecutionInfo( + Object executionRequirementsUnchecked, Rule rule) throws EvalException { + Map checkedExecutionRequirements = + TargetUtils.filter( + SkylarkDict.castSkylarkDictOrNoneToDict( + executionRequirementsUnchecked, + String.class, + String.class, + "execution_requirements")); + Map checkedTags = getExecutionInfo(rule); + + Map executionInfoBuilder = new HashMap<>(); + // adding filtered execution requirements to the execution info map + executionInfoBuilder.putAll(checkedExecutionRequirements); + // merging filtered tags to the execution info map avoiding duplicates + checkedTags.forEach(executionInfoBuilder::putIfAbsent); + + return ImmutableMap.copyOf(executionInfoBuilder); + } + /** * Returns the execution info. These include execution requirement tags ('block-*', 'requires-*', * 'no-*', 'supports-*', 'disable-*', 'local', and 'cpu:*') as keys with empty values. */ public static Map filter(Map executionInfo) { - return Maps.filterKeys(executionInfo, LEGAL_EXEC_INFO_KEYS); + return Maps.filterKeys(executionInfo, TargetUtils::legalExecInfoKeys); } /** diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTest.java index 5826f5a75ea77f..0788a8970a213a 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTest.java @@ -114,6 +114,14 @@ public void testDestinationArtifactIsOutput() { assertThat(outputs).containsExactly(destinationArtifact); } + @Test + public void testExecutionInfoCopied() { + SpawnAction copyFromWelcomeToDestination = + createCopyFromWelcomeToDestination(ImmutableMap.of()); + Map executionInfo = copyFromWelcomeToDestination.getExecutionInfo(); + assertThat(executionInfo).containsExactly("local", ""); + } + @Test public void testBuilder() throws Exception { Artifact input = getSourceArtifact("input"); @@ -288,7 +296,7 @@ public void testExtraActionInfo() throws Exception { assertThat(info.getMnemonic()).isEqualTo("Dummy"); SpawnInfo spawnInfo = info.getExtension(SpawnInfo.spawnInfo); - assertThat(spawnInfo).isNotNull(); + assertThat(info.hasExtension(SpawnInfo.spawnInfo)).isTrue(); assertThat(spawnInfo.getArgumentList()) .containsExactlyElementsIn(action.getArguments()); 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 eb49366cf26fb0..a0bb8184b7c638 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 @@ -18,6 +18,8 @@ import com.google.common.base.Predicate; import com.google.common.collect.Lists; import com.google.devtools.build.lib.packages.util.PackageLoadingTestCase; +import com.google.devtools.build.lib.syntax.Runtime; +import com.google.devtools.build.lib.syntax.SkylarkDict; import java.util.Map; import org.junit.Test; import org.junit.runner.RunWith; @@ -96,4 +98,154 @@ public void testExecutionInfo() throws Exception { execInfo = TargetUtils.getExecutionInfo(tag1b); assertThat(execInfo).containsExactly("local", "", "cpu:4", ""); } + + @Test + public void testExecutionInfo_withPrefixSupports() throws Exception { + scratch.file( + "tests/BUILD", + "sh_binary(name = 'with-prefix-supports', srcs=['sh.sh'], tags=['supports-workers'," + + " 'supports-whatever', 'my-tag'])"); + + Rule withSupportsPrefix = (Rule) getTarget("//tests:with-prefix-supports"); + + Map execInfo = TargetUtils.getExecutionInfo(withSupportsPrefix); + assertThat(execInfo).containsExactly("supports-whatever", "", "supports-workers", ""); + } + + @Test + public void testExecutionInfo_withPrefixDisable() throws Exception { + scratch.file( + "tests/BUILD", + "sh_binary(name = 'with-prefix-disable', srcs=['sh.sh'], tags=['disable-local-prefetch'," + + " 'disable-something-else', 'another-tag'])"); + + Rule withDisablePrefix = (Rule) getTarget("//tests:with-prefix-disable"); + + Map execInfo = TargetUtils.getExecutionInfo(withDisablePrefix); + assertThat(execInfo) + .containsExactly("disable-local-prefetch", "", "disable-something-else", ""); + } + + @Test + public void testExecutionInfo_withPrefixNo() throws Exception { + scratch.file( + "tests/BUILD", + "sh_binary(name = 'with-prefix-no', srcs=['sh.sh'], tags=['no-remote-imaginary-flag'," + + " 'no-sandbox', 'unknown'])"); + + Rule withNoPrefix = (Rule) getTarget("//tests:with-prefix-no"); + + Map execInfo = TargetUtils.getExecutionInfo(withNoPrefix); + assertThat(execInfo).containsExactly("no-remote-imaginary-flag", "", "no-sandbox", ""); + } + + @Test + public void testExecutionInfo_withPrefixRequires() throws Exception { + scratch.file( + "tests/BUILD", + "sh_binary(name = 'with-prefix-requires', srcs=['sh.sh'], tags=['requires-network'," + + " 'requires-sunlight', 'test-only'])"); + + Rule withRequiresPrefix = (Rule) getTarget("//tests:with-prefix-requires"); + + Map execInfo = TargetUtils.getExecutionInfo(withRequiresPrefix); + assertThat(execInfo).containsExactly("requires-network", "", "requires-sunlight", ""); + } + + @Test + public void testExecutionInfo_withPrefixBlock() throws Exception { + scratch.file( + "tests/BUILD", + "sh_binary(name = 'with-prefix-block', srcs=['sh.sh'], tags=['block-some-feature'," + + " 'block-network', 'wrong-tag'])"); + + Rule withBlockPrefix = (Rule) getTarget("//tests:with-prefix-block"); + + Map execInfo = TargetUtils.getExecutionInfo(withBlockPrefix); + assertThat(execInfo).containsExactly("block-network", "", "block-some-feature", ""); + } + + @Test + public void testExecutionInfo_withPrefixCpu() throws Exception { + scratch.file( + "tests/BUILD", + "sh_binary(name = 'with-prefix-cpu', srcs=['sh.sh'], tags=['cpu:123', 'wrong-tag'])"); + + Rule withCpuPrefix = (Rule) getTarget("//tests:with-prefix-cpu"); + + Map execInfo = TargetUtils.getExecutionInfo(withCpuPrefix); + assertThat(execInfo).containsExactly("cpu:123", ""); + } + + @Test + public void testExecutionInfo_withLocalTag() throws Exception { + scratch.file( + "tests/BUILD", + "sh_binary(name = 'with-local-tag', srcs=['sh.sh'], tags=['local', 'some-tag'])"); + + Rule withLocal = (Rule) getTarget("//tests:with-local-tag"); + + Map execInfo = TargetUtils.getExecutionInfo(withLocal); + assertThat(execInfo).containsExactly("local", ""); + } + + @Test + public void testFilteredExecutionInfo_FromUncheckedExecRequirements() throws Exception { + scratch.file("tests/BUILD", "sh_binary(name = 'no-tag', srcs=['sh.sh'])"); + + Rule noTag = (Rule) getTarget("//tests:no-tag"); + + Map execInfo = + TargetUtils.getFilteredExecutionInfo(SkylarkDict.of(null, "supports-worker", "1"), noTag); + assertThat(execInfo).containsExactly("supports-worker", "1"); + + execInfo = + TargetUtils.getFilteredExecutionInfo( + SkylarkDict.of(null, "some-custom-tag", "1", "no-cache", "1"), noTag); + assertThat(execInfo).containsExactly("no-cache", "1"); + } + + @Test + public void testFilteredExecutionInfo() throws Exception { + scratch.file( + "tests/BUILD", + "sh_binary(name = 'tag1', srcs=['sh.sh'], tags=['supports-workers', 'no-cache'])"); + Rule tag1 = (Rule) getTarget("//tests:tag1"); + SkylarkDict executionRequirementsUnchecked = + SkylarkDict.of(null, "no-remote", "1"); + + Map execInfo = + TargetUtils.getFilteredExecutionInfo(executionRequirementsUnchecked, tag1); + + assertThat(execInfo).containsExactly("no-cache", "", "supports-workers", "", "no-remote", "1"); + } + + @Test + public void testFilteredExecutionInfo_withDuplicateTags() throws Exception { + scratch.file( + "tests/BUILD", + "sh_binary(name = 'tag1', srcs=['sh.sh'], tags=['supports-workers', 'no-cache'])"); + Rule tag1 = (Rule) getTarget("//tests:tag1"); + SkylarkDict executionRequirementsUnchecked = + SkylarkDict.of(null, "no-cache", "1"); + + Map execInfo = + TargetUtils.getFilteredExecutionInfo(executionRequirementsUnchecked, tag1); + + assertThat(execInfo).containsExactly("no-cache", "1", "supports-workers", ""); + } + + @Test + public void testFilteredExecutionInfo_WithNullUncheckedExecRequirements() throws Exception { + scratch.file( + "tests/BUILD", + "sh_binary(name = 'tag1', srcs=['sh.sh'], tags=['supports-workers', 'no-cache'])"); + Rule tag1 = (Rule) getTarget("//tests:tag1"); + + Map execInfo = TargetUtils.getFilteredExecutionInfo(null, tag1); + assertThat(execInfo).containsExactly("no-cache", "", "supports-workers", ""); + + execInfo = TargetUtils.getFilteredExecutionInfo(Runtime.NONE, tag1); + assertThat(execInfo).containsExactly("no-cache", "", "supports-workers", ""); + } } diff --git a/src/test/shell/bazel/BUILD b/src/test/shell/bazel/BUILD index 559c742e3b86f3..2fe034d28dc96d 100644 --- a/src/test/shell/bazel/BUILD +++ b/src/test/shell/bazel/BUILD @@ -698,6 +698,14 @@ sh_test( tags = ["no_windows"], ) +sh_test( + name = "tags_propagation_skylark_test", + size = "large", + srcs = ["tag_propagation_skylark_test.sh"], + data = [":test-deps"], + tags = ["no_windows"], +) + sh_test( name = "disk_cache_test", size = "small", diff --git a/src/test/shell/bazel/tag_propagation_skylark_test.sh b/src/test/shell/bazel/tag_propagation_skylark_test.sh new file mode 100755 index 00000000000000..432bbe08677317 --- /dev/null +++ b/src/test/shell/bazel/tag_propagation_skylark_test.sh @@ -0,0 +1,137 @@ +#!/bin/bash +# +# Copyright 2019 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# Tests target's tags propagation with rules defined in Skylark. +# Tests for https://github.com/bazelbuild/bazel/issues/7766 + +# Load the test setup defined in the parent directory +CURRENT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +source "${CURRENT_DIR}/../integration_test_setup.sh" \ + || { echo "integration_test_setup.sh not found!" >&2; exit 1; } + +# Test a basic skylark ctx.actions.run_shell rule which has tags, that should be propagated +function test_tags_propagated_to_run_shell() { + mkdir -p test + cat << EOF >> test/BUILD +load(":skylark.bzl", "test_rule") + +test_rule( + name = "test", + out = "output.txt", + tags = ["no-cache", "no-remote", "local"] +) +EOF + + cat << 'EOF' >> test/skylark.bzl +def _test_impl(ctx): + ctx.actions.run_shell(outputs = [ctx.outputs.out], + command = ["touch", ctx.outputs.out.path]) + files_to_build = depset([ctx.outputs.out]) + return DefaultInfo( + files = files_to_build, + ) + +test_rule = rule( + implementation=_test_impl, + attrs = { + "out": attr.output(mandatory = True), + }, +) +EOF + + bazel aquery '//test:test' > output1 2> $TEST_log \ + || fail "should have generated output successfully" + + assert_contains "ExecutionInfo: {local: '', no-cache: '', no-remote: ''}" output1 +} + +# Test a basic skylark ctx.actions.run rule which has tags, that should be propagated +function test_tags_propagated_to_run() { + mkdir -p test + cat << EOF >> test/BUILD +load(":skylark.bzl", "test_rule") + +test_rule( + name = "test", + out = "output.txt", + tags = ["no-cache", "no-remote", "no-sandbox", "requires-network", "local"] +) +EOF + + cat << 'EOF' >> test/skylark.bzl +def _test_impl(ctx): + ctx.actions.run( + outputs = [ctx.outputs.out], + executable = 'dummy') + files_to_build = depset([ctx.outputs.out]) + return DefaultInfo( + files = files_to_build, + ) + +test_rule = rule( + implementation=_test_impl, + attrs = { + "out": attr.output(mandatory = True), + }, +) +EOF + + bazel aquery '//test:test' > output1 2> $TEST_log \ + || fail "should have generated output successfully" + + assert_contains "ExecutionInfo: {local: '', no-cache: '', no-remote: '', no-sandbox: '', requires-network: ''}" output1 +} + +# Test a basic skylark ctx.actions.run rule which has tags, that should be propagated, +# when the rule also has execution_info +function test_tags_propagated_to_run_with_exec_info_in_rule() { + mkdir -p test + cat << EOF >> test/BUILD +load(":skylark.bzl", "test_rule") + +test_rule( + name = "test", + out = "output.txt", + tags = ["no-cache", "no-remote", "custom-tag-1", "requires-network", "local"] +) +EOF + + cat << 'EOF' >> test/skylark.bzl +def _test_impl(ctx): + ctx.actions.run( + outputs = [ctx.outputs.out], + executable = 'dummy', + execution_requirements = {"requires-x": "", "custom-tag-whatever": "", "no-cache": "1"}) + files_to_build = depset([ctx.outputs.out]) + return DefaultInfo( + files = files_to_build, + ) + +test_rule = rule( + implementation=_test_impl, + attrs = { + "out": attr.output(mandatory = True), + }, +) +EOF + + bazel aquery '//test:test' > output1 2> $TEST_log \ + || fail "should have generated output successfully" + + assert_contains "ExecutionInfo: {local: '', no-cache: 1, no-remote: '', requires-network: '', requires-x: ''}" output1 +} + +run_suite "tags propagation: skylark rule tests"