Tags propagation: added incompatible flag

reverted rollback of tags propagation: 29eecb5

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 #7766 for a link), 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 when '--ncompatible_allow_tags_propagation' flag set to true. See #8830.

Closes #7766
Related to #8830

Closes #8829.

PiperOrigin-RevId: 258521443

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,14 @@ 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(),
+            starlarkSemantics.incompatibleAllowTagsPropagation());
+    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/StarlarkSemanticsOptions.java

@@ -176,6 +176,21 @@ public class StarlarkSemanticsOptions extends OptionsBase implements Serializabl
               + "statement.")
   public boolean incompatibleBzlDisallowLoadAfterStatement;
 
+  @Option(
+      name = "incompatible_allow_tags_propagation",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
+      effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
+      metadataTags = {
+        OptionMetadataTag.INCOMPATIBLE_CHANGE,
+        OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
+      },
+      help =
+          "If set to true, tags will be propagated from a target to the actions' execution"
+              + " requirements; otherwise tags are not propagated. See"
+              + " https://github.com/bazelbuild/bazel/issues/8830 for details.")
+  public boolean incompatibleAllowTagsPropagation;
+
   @Option(
       name = "incompatible_depset_union",
       defaultValue = "true",
@@ -697,6 +712,7 @@ public StarlarkSemantics toSkylarkSemantics() {
             .incompatibleDisallowSplitEmptySeparator(incompatibleDisallowSplitEmptySeparator)
             .incompatibleDisallowDictLookupUnhashableKeys(
                 incompatibleDisallowDictLookupUnhashableKeys)
+            .incompatibleAllowTagsPropagation(incompatibleAllowTagsPropagation)
             .build();
     return INTERNER.intern(semantics);
   }

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,64 @@ 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 #legalExecInfoKeys} 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
+   * #legalExecInfoKeys}.
+   *
+   * @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
+   * @param incompatibleAllowTagsPropagation if set to true, tags will be propagated from a target
+   *     to the actions' execution requirements, for more details {@see
+   *     SkylarkSematicOptions#incompatibleAllowTagsPropagation}
+   */
+  public static ImmutableMap<String, String> getFilteredExecutionInfo(
+      Object executionRequirementsUnchecked, Rule rule, boolean incompatibleAllowTagsPropagation)
+      throws EvalException {
+    Map<String, String> checkedExecutionRequirements =
+        TargetUtils.filter(
+            SkylarkDict.castSkylarkDictOrNoneToDict(
+                executionRequirementsUnchecked,
+                String.class,
+                String.class,
+                "execution_requirements"));
+
+    Map<String, String> executionInfoBuilder = new HashMap<>();
+    // adding filtered execution requirements to the execution info map
+    executionInfoBuilder.putAll(checkedExecutionRequirements);
+
+    if (incompatibleAllowTagsPropagation) {
+      Map<String, String> checkedTags = getExecutionInfo(rule);
+      // 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/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java

@@ -60,6 +60,7 @@ public enum FlagIdentifier {
     INCOMPATIBLE_OBJC_FRAMEWORK_CLEANUP(StarlarkSemantics::incompatibleObjcFrameworkCleanup),
     INCOMPATIBLE_DISALLOW_RULE_EXECUTION_PLATFORM_CONSTRAINTS_ALLOWED(
         StarlarkSemantics::incompatibleDisallowRuleExecutionPlatformConstraintsAllowed),
+    INCOMPATIBLE_ALLOW_TAGS_PROPAGATION(StarlarkSemantics::incompatibleAllowTagsPropagation),
     NONE(null);
 
     // Using a Function here makes the enum definitions far cleaner, and, since this is
@@ -210,6 +211,8 @@ public boolean flagValue(FlagIdentifier flagIdentifier) {
 
   public abstract boolean incompatibleDisallowDictLookupUnhashableKeys();
 
+  public abstract boolean incompatibleAllowTagsPropagation();
+
   @Memoized
   @Override
   public abstract int hashCode();
@@ -287,6 +290,7 @@ public static Builder builderWithDefaults() {
           .incompatibleRestrictStringEscapes(false)
           .incompatibleDisallowSplitEmptySeparator(false)
           .incompatibleDisallowDictLookupUnhashableKeys(false)
+          .incompatibleAllowTagsPropagation(false)
           .build();
 
   /** Builder for {@link StarlarkSemantics}. All fields are mandatory. */
@@ -312,6 +316,8 @@ public abstract static class Builder {
 
     public abstract Builder experimentalStarlarkUnusedInputsList(boolean value);
 
+    public abstract Builder incompatibleAllowTagsPropagation(boolean value);
+
     public abstract Builder incompatibleBzlDisallowLoadAfterStatement(boolean value);
 
     public abstract Builder incompatibleDepsetIsNotIterable(boolean value);

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/SkylarkSemanticsConsistencyTest.java

@@ -133,6 +133,7 @@ private static StarlarkSemanticsOptions buildRandomOptions(Random rand) throws E
         "--experimental_platforms_api=" + rand.nextBoolean(),
         "--experimental_starlark_config_transitions=" + rand.nextBoolean(),
         "--experimental_starlark_unused_inputs_list=" + rand.nextBoolean(),
+        "--incompatible_allow_tags_propagation=" + rand.nextBoolean(),
         "--incompatible_bzl_disallow_load_after_statement=" + rand.nextBoolean(),
         "--incompatible_depset_for_libraries_to_link_getter=" + rand.nextBoolean(),
         "--incompatible_depset_is_not_iterable=" + rand.nextBoolean(),
@@ -188,6 +189,7 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) {
         .experimentalPlatformsApi(rand.nextBoolean())
         .experimentalStarlarkConfigTransitions(rand.nextBoolean())
         .experimentalStarlarkUnusedInputsList(rand.nextBoolean())
+        .incompatibleAllowTagsPropagation(rand.nextBoolean())
         .incompatibleBzlDisallowLoadAfterStatement(rand.nextBoolean())
         .incompatibleDepsetForLibrariesToLinkGetter(rand.nextBoolean())
         .incompatibleDepsetIsNotIterable(rand.nextBoolean())