Automated rollback of commit 5f53ab6.

*** 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 bazelbuild#7766 for details), set o...

***

RELNOTES: None.
PiperOrigin-RevId: 256561364

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.
- *
- * <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,11 +558,15 @@ private void registerStarlarkAction(
     if (EvalUtils.toBoolean(useDefaultShellEnv)) {
       builder.useDefaultShellEnvironment();
     }
-
-    ImmutableMap<String, String> 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")) {

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<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 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<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)) {
-      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<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, TargetUtils::legalExecInfoKeys);
+    return Maps.filterKeys(executionInfo, LEGAL_EXEC_INFO_KEYS);
   }
 
   /**

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

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