Tags propagation: added incompatible flag reverted rollback of tags propagation: https://github.com/bazelbuild/bazel/commit/29eecb56c5f0f26b4751ec9eb79954412fb2991c 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
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 e632839..330dffa 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. + * + * <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 {
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 29b7e5b..16dea75 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,14 @@ 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")) {
diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java index 868697d..73879e3 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java +++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java
@@ -177,6 +177,21 @@ 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", documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, @@ -697,6 +712,7 @@ .incompatibleDisallowSplitEmptySeparator(incompatibleDisallowSplitEmptySeparator) .incompatibleDisallowDictLookupUnhashableKeys( incompatibleDisallowDictLookupUnhashableKeys) + .incompatibleAllowTagsPropagation(incompatibleAllowTagsPropagation) .build(); return INTERNER.intern(semantics); }
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 acb0352..3ab123a 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 @@ // 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,19 +219,15 @@ } /** - * 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, ""); } } @@ -240,11 +235,48 @@ } /** + * 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); } /**
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java index 87b8c5f..f2181f4 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java
@@ -60,6 +60,7 @@ 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 abstract boolean incompatibleDisallowDictLookupUnhashableKeys(); + public abstract boolean incompatibleAllowTagsPropagation(); + @Memoized @Override public abstract int hashCode(); @@ -287,6 +290,7 @@ .incompatibleRestrictStringEscapes(false) .incompatibleDisallowSplitEmptySeparator(false) .incompatibleDisallowDictLookupUnhashableKeys(false) + .incompatibleAllowTagsPropagation(false) .build(); /** Builder for {@link StarlarkSemantics}. All fields are mandatory. */ @@ -312,6 +316,8 @@ public abstract Builder experimentalStarlarkUnusedInputsList(boolean value); + public abstract Builder incompatibleAllowTagsPropagation(boolean value); + public abstract Builder incompatibleBzlDisallowLoadAfterStatement(boolean value); public abstract Builder incompatibleDepsetIsNotIterable(boolean value);