From e9dc1fd14213cc55ce32c5b39d220979f66e9919 Mon Sep 17 00:00:00 2001 From: rosica Date: Thu, 4 Jul 2019 09:23:38 -0700 Subject: [PATCH] Automated rollback of commit 5f53ab606f9e09076e9c1056378f32a78b04a880. *** Reason for rollback *** Breaks internal tests *** Original change description *** tags propagation: Starlark rules part Tags declared on targets are not propagated to actions and therefore are not taken into consideration by bazel. This causes some issues, for instance, target marked with a tag 'no-remote' will still be executed remotely. As it was agreed in the design doc (see [doc](https://docs.google.com/document/d/1X2GtuuNT6UqYYOK5lJWQEdPjAgsbdB3nFjjmjso-XHo/edit#heading=h.5mcn15i0e1ch) and #7766 for details), set o... *** RELNOTES: None. PiperOrigin-RevId: 256561364 --- .../lib/actions/ExecutionRequirements.java | 12 -- .../skylark/SkylarkActionFactory.java | 14 +- .../build/lib/packages/TargetUtils.java | 65 +++----- .../lib/analysis/actions/SpawnActionTest.java | 10 +- .../build/lib/packages/TargetUtilsTest.java | 152 ------------------ src/test/shell/bazel/BUILD | 8 - .../bazel/tag_propagation_skylark_test.sh | 137 ---------------- 7 files changed, 30 insertions(+), 368 deletions(-) delete mode 100755 src/test/shell/bazel/tag_propagation_skylark_test.sh 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 330dffa665e930..e63283913b8cbd 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,23 +18,11 @@ 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 4954b6b7cdb587..29b7e5bc711df8 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,11 +558,15 @@ private void registerStarlarkAction( if (EvalUtils.toBoolean(useDefaultShellEnv)) { builder.useDefaultShellEnvironment(); } - - ImmutableMap executionInfo = - TargetUtils.getFilteredExecutionInfo(executionRequirementsUnchecked, ruleContext.getRule()); - builder.setExecutionInfo(executionInfo); - + if (executionRequirementsUnchecked != Runtime.NONE) { + builder.setExecutionInfo( + TargetUtils.filter( + SkylarkDict.castSkylarkDictOrNoneToDict( + executionRequirementsUnchecked, + String.class, + String.class, + "execution_requirements"))); + } 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 23ba8e07930f7d..acb035268a6054 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,8 +24,6 @@ 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; @@ -48,15 +46,18 @@ 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 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 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 TargetUtils() {} // Uninstantiable. @@ -219,57 +220,31 @@ private static boolean hasConstraint(Rule rule, String keyword) { } /** - * 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. + * 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 getExecutionInfo(Rule rule) { // tags may contain duplicate values. Map map = new HashMap<>(); for (String tag : NonconfigurableAttributeMapper.of(rule).get(CONSTRAINTS_ATTR, Type.STRING_LIST)) { - if (legalExecInfoKeys(tag)) { + // 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)) { 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, TargetUtils::legalExecInfoKeys); + return Maps.filterKeys(executionInfo, LEGAL_EXEC_INFO_KEYS); } /** 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 0788a8970a213a..5826f5a75ea77f 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,14 +114,6 @@ 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"); @@ -296,7 +288,7 @@ public void testExtraActionInfo() throws Exception { assertThat(info.getMnemonic()).isEqualTo("Dummy"); SpawnInfo spawnInfo = info.getExtension(SpawnInfo.spawnInfo); - assertThat(info.hasExtension(SpawnInfo.spawnInfo)).isTrue(); + assertThat(spawnInfo).isNotNull(); 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 a0bb8184b7c638..eb49366cf26fb0 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,8 +18,6 @@ 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; @@ -98,154 +96,4 @@ 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 2fe034d28dc96d..559c742e3b86f3 100644 --- a/src/test/shell/bazel/BUILD +++ b/src/test/shell/bazel/BUILD @@ -698,14 +698,6 @@ 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 deleted file mode 100755 index 432bbe08677317..00000000000000 --- a/src/test/shell/bazel/tag_propagation_skylark_test.sh +++ /dev/null @@ -1,137 +0,0 @@ -#!/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"