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 bazelbuild#7766 for details), set of tags to be propagated to actions as a first iteration.
This change is responsible for that first step for the Starlark Rules.

RELNOTES: tags 'no-remote', 'no-cache', 'no-remote-cache', 'no-remote-exec', 'no-sandbox' are propagated now to the actions from targets.

Closes bazelbuild#7766

Closes bazelbuild#8612.

PiperOrigin-RevId: 256369636

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.
+ *
+ * <ol>
+ *   If you are adding a new execution requirement, pay attention to the following:
+ *   <li>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)}
+ *   <li>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)}.
+ * </ol>
  */
 public class ExecutionRequirements {
 

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<String, String> executionInfo =
+        TargetUtils.getFilteredExecutionInfo(executionRequirementsUnchecked, ruleContext.getRule());
+    builder.setExecutionInfo(executionInfo);
+
     if (inputManifestsUnchecked != Runtime.NONE) {
       for (RunfilesSupplier supplier : SkylarkList.castSkylarkListOrNoneToList(
           inputManifestsUnchecked, RunfilesSupplier.class, "runfiles suppliers")) {

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<String> LEGAL_EXEC_INFO_KEYS = new Predicate<String>() {
-    @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<String, String> getExecutionInfo(Rule rule) {
     // tags may contain duplicate values.
     Map<String, String> 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<String, String>} type, null or {@link
+   *     com.google.devtools.build.lib.syntax.Runtime#NONE}
+   * @param rule a rule instance to get tags from
+   */
+  public static ImmutableMap<String, String> getFilteredExecutionInfo(
+      Object executionRequirementsUnchecked, Rule rule) throws EvalException {
+    Map<String, String> checkedExecutionRequirements =
+        TargetUtils.filter(
+            SkylarkDict.castSkylarkDictOrNoneToDict(
+                executionRequirementsUnchecked,
+                String.class,
+                String.class,
+                "execution_requirements"));
+    Map<String, String> checkedTags = getExecutionInfo(rule);
+
+    Map<String, String> 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<String, String> filter(Map<String, String> executionInfo) {
-    return Maps.filterKeys(executionInfo, LEGAL_EXEC_INFO_KEYS);
+    return Maps.filterKeys(executionInfo, TargetUtils::legalExecInfoKeys);
   }
 
   /**

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<String, String> 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());

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<String, String> 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<String, String> 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<String, String> 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<String, String> 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<String, String> 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<String, String> 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<String, String> 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<String, String> 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<String, String> executionRequirementsUnchecked =
+        SkylarkDict.of(null, "no-remote", "1");
+
+    Map<String, String> 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<String, String> executionRequirementsUnchecked =
+        SkylarkDict.of(null, "no-cache", "1");
+
+    Map<String, String> 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<String, String> 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", "");
+  }
 }

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",