Additional functionalities to the existing scoping API. This API handles the following cases: 1. flags with `scope="target"` should not propagate to the exec configuration unless `on_leave_scope` attribute is specified with a value that is different than the default value. 2. flags with `scope="--<another_flag>"` indicates that this flag should propagate to the exec configuration with the value of `--<another_flag>` PiperOrigin-RevId: 849255631 Change-Id: I51e803c6d1315d38c1d52f7677f7567bfd30b880
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/Scope.java b/src/main/java/com/google/devtools/build/lib/analysis/config/Scope.java index 10b423e..b5070e8 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/Scope.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/Scope.java
@@ -24,6 +24,8 @@ * Scope.ScopeDefinition}. */ public class Scope { + public static final String CUSTOM_EXEC_SCOPE_PREFIX = "exec:--"; + /** Type of supported scopes. */ @AutoCodec public record ScopeType(String scopeType) {
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/producers/BuildConfigurationKeyProducer.java b/src/main/java/com/google/devtools/build/lib/analysis/producers/BuildConfigurationKeyProducer.java index 78efc7a..815cc56 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/producers/BuildConfigurationKeyProducer.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/producers/BuildConfigurationKeyProducer.java
@@ -183,7 +183,9 @@ this.postPlatformProcessedOptions.getScopeTypeMap().get(entry.getKey()); // scope is null is applicable for cases where a transition applies starlark flags that are // not already part of the baseline configuration. - if (scopeType == null || scopeType.scopeType().equals(Scope.ScopeType.PROJECT)) { + if (scopeType == null + || scopeType.scopeType().equals(Scope.ScopeType.PROJECT) + || scopeType.scopeType().startsWith(Scope.CUSTOM_EXEC_SCOPE_PREFIX)) { flagsWithIncompleteScopeInfo.add(entry.getKey()); } }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java index c0e4a2d..bff1f2a 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java
@@ -262,7 +262,12 @@ if (scopeType.equals(Scope.ScopeType.UNIVERSAL)) { ans.put(entry); } else if (scopeType.equals(Scope.ScopeType.TARGET)) { - // Don't propagate this flag. + Object onLeaveScopeValue = options.getOnLeaveScopeValues().get(entry.getKey()); + if (onLeaveScopeValue != null) { + // if on_leave_scope is set, propagate to exec config with this value. + ans.put(entry.getKey(), onLeaveScopeValue); + } + // else Don't propagate this flag. } else if (customPropagatingFlags.contains(entry.getKey().getUnambiguousCanonicalForm())) { ans.put(entry); } else if (customPropagatingFlagPatterns.stream() @@ -275,8 +280,32 @@ pattern.substring( 0, pattern.lastIndexOf(CustomFlagConverter.SUBPACKAGES_SUFFIX))))) { ans.put(entry); + } else if (scopeType.startsWith(Scope.CUSTOM_EXEC_SCOPE_PREFIX)) { + Label anotherFlag = Label.parseCanonicalUnchecked(scopeType.substring(7)); + if (starlarkOptions.containsKey(anotherFlag)) { + ans.put(entry.getKey(), starlarkOptions.get(anotherFlag)); + } else { + boolean found = false; + for (FragmentOptions fragment : options.getNativeOptions()) { + Map<String, Object> nativeOptions = fragment.asMap(); + if (nativeOptions.containsKey(anotherFlag.getUnambiguousCanonicalForm())) { + ans.put(entry.getKey(), nativeOptions.get(anotherFlag.getUnambiguousCanonicalForm())); + found = true; + break; + } + } + // if the flag is not found in both starlark and the native options, it's an error. + if (!found) { + throw new IllegalStateException( + "Flag " + + anotherFlag + + " is not found in the starlark options or native options. It should be one of" + + " them."); + } + } } } + return ans.buildOrThrow(); }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/LabelBuildSettings.java b/src/main/java/com/google/devtools/build/lib/rules/LabelBuildSettings.java index 90ec90d..5f3e8c5 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/LabelBuildSettings.java +++ b/src/main/java/com/google/devtools/build/lib/rules/LabelBuildSettings.java
@@ -22,9 +22,7 @@ import com.google.common.base.Preconditions; import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment; import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; -import com.google.devtools.build.lib.analysis.config.Scope; import com.google.devtools.build.lib.cmdline.Label; -import com.google.devtools.build.lib.packages.Attribute.AllowedValueSet; import com.google.devtools.build.lib.packages.Attribute.LabelLateBoundDefault; import com.google.devtools.build.lib.packages.BuildSetting; import com.google.devtools.build.lib.packages.RawAttributeMapper; @@ -92,8 +90,7 @@ .add( attr("scope", STRING) .value("universal") - .nonconfigurable(NONCONFIGURABLE_ATTRIBUTE_REASON) - .allowedValues(new AllowedValueSet(Scope.ScopeType.allowedAttributeValues()))) + .nonconfigurable(NONCONFIGURABLE_ATTRIBUTE_REASON)) .add(attr("on_leave_scope", NODEP_LABEL).nonconfigurable(NONCONFIGURABLE_ATTRIBUTE_REASON)) .setBuildSetting(BuildSetting.create(flag, NODEP_LABEL)) .canHaveAnyProvider()
diff --git a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigRuleClasses.java index 4207f02..8643118 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigRuleClasses.java +++ b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigRuleClasses.java
@@ -27,10 +27,8 @@ import com.google.devtools.build.lib.analysis.RuleDefinition; import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment; import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; -import com.google.devtools.build.lib.analysis.config.Scope; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.packages.AllowlistChecker; -import com.google.devtools.build.lib.packages.Attribute.AllowedValueSet; import com.google.devtools.build.lib.packages.Attribute.LabelListLateBoundDefault; import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper; import com.google.devtools.build.lib.packages.RawAttributeMapper; @@ -479,8 +477,7 @@ .add( attr("scope", STRING) .value("universal") - .nonconfigurable(NONCONFIGURABLE_ATTRIBUTE_REASON) - .allowedValues(new AllowedValueSet(Scope.ScopeType.allowedAttributeValues()))) + .nonconfigurable(NONCONFIGURABLE_ATTRIBUTE_REASON)) .add(ConfigFeatureFlag.getAllowlistAttribute(env)) .addAllowlistChecker(ALWAYS_CHECK_ALLOWLIST) .removeAttribute(BaseRuleClasses.TAGGED_TRIMMING_ATTR)
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java b/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java index 0f6ddcc..eb04c01 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java
@@ -26,6 +26,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.LinkedListMultimap; import com.google.common.collect.Multimap; +import com.google.devtools.build.lib.analysis.config.Scope; import com.google.devtools.build.lib.analysis.config.Scope.ScopeType; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.TargetParsingException; @@ -230,6 +231,7 @@ Map<String, Object> parsedOptions = new HashMap<>(); Map<String, String> scopeTypeMap = new HashMap<>(); Map<String, Object> onLeaveScopeMap = new HashMap<>(); + List<String> customExecFlags = new ArrayList<>(); for (String buildSetting : buildSettingWithTargetAndValue.keySet()) { Pair<Target, Object> buildSettingAndFinalValue = buildSettingWithTargetAndValue.get(buildSetting); @@ -262,7 +264,8 @@ String scopeType = ScopeType.DEFAULT.toString(); if (attrMap.isAttributeValueExplicitlySpecified("scope")) { scopeType = attrMap.get("scope", Type.STRING); - if (!ScopeType.allowedAttributeValues().contains(scopeType.toLowerCase(Locale.ROOT))) { + if (!ScopeType.allowedAttributeValues().contains(scopeType.toLowerCase(Locale.ROOT)) + && !scopeType.startsWith(Scope.CUSTOM_EXEC_SCOPE_PREFIX)) { throw new OptionsParsingException( String.format( "Can't load flag --%s: Invalid \"scope\" attribute value \"%s\". Allowed values:" @@ -277,21 +280,52 @@ scopeTypeMap.put(buildSetting, scopeType); nativeOptionsParser.setScopesAttributes(ImmutableMap.copyOf(scopeTypeMap)); + if (scopeType.startsWith(Scope.CUSTOM_EXEC_SCOPE_PREFIX)) { + customExecFlags.add(scopeType.substring(7)); + } + if (attrMap.isAttributeValueExplicitlySpecified("on_leave_scope")) { var onLeaveScopeValue = attrMap.get("on_leave_scope", buildSettingObject.getType()); - if (onLeaveScopeValue != null) { - onLeaveScopeMap.put(buildSetting, onLeaveScopeValue); - } + onLeaveScopeMap.put(buildSetting, onLeaveScopeValue); } } + // handling custom exec case with scope "exec:--<another_flag_name>". + // For example: --python_launcher=--host_python_launcher + // have the --<another_flag_name> flag in the target config but also make sure that it + // won't propagate to the exec config by setting the scope to "target". + for (String customExecFlag : customExecFlags) { + // if the custom exec flag is already in the parsedOptions, we use that value. + if (parsedOptions.containsKey(customExecFlag)) { + continue; + } + + // get the default value for the custom exec flag if it's not set yet. + parsedOptions.put(customExecFlag, getDefaultValueForAnyBuildSetting(customExecFlag)); + scopeTypeMap.put(customExecFlag, ScopeType.TARGET); + } + nativeOptionsParser.setStarlarkOptions(ImmutableMap.copyOf(parsedOptions)); + nativeOptionsParser.setOnLeaveScopeValues(ImmutableMap.copyOf(onLeaveScopeMap)); this.starlarkOptions.putAll(parsedOptions); this.scopes.putAll(scopeTypeMap); this.onLeaveScopeValues.putAll(onLeaveScopeMap); return true; } + public Object getDefaultValueForAnyBuildSetting(String buildSetting) + throws InterruptedException, OptionsParsingException { + Target buildSettingTarget = loadBuildSetting(buildSetting); + BuildSetting buildSettingObject = + buildSettingTarget.getAssociatedRule().getRuleClassObject().getBuildSetting(); + Object defaultValue = + buildSettingTarget.getAssociatedRule().getAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME); + if (buildSettingObject.allowsMultiple()) { + return ImmutableList.of(Objects.requireNonNull(defaultValue)); + } + return defaultValue; + } + /** * Parses the given {@code flag=value} setting. *
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildOptionsScopeFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildOptionsScopeFunction.java index 44dd9d2..2bb3d27 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BuildOptionsScopeFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BuildOptionsScopeFunction.java
@@ -13,6 +13,7 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe; +import static com.google.devtools.build.lib.packages.RuleClass.Builder.STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME; import static com.google.devtools.build.lib.server.FailureDetails.TargetPatterns.Code.DEPENDENCY_NOT_FOUND; import com.google.common.collect.ImmutableMultimap; @@ -83,6 +84,27 @@ .addScopeType(scopedFlag, scopeType) .addOnLeaveScopeValue(scopedFlag, onLeaveScopeValue) : fullyResolvedBuildOptionsBuilder.addScopeType(scopedFlag, scopeType); + + if (scopeType.scopeType().startsWith(Scope.CUSTOM_EXEC_SCOPE_PREFIX)) { + // handling custom exec case with scope "exec:--<another_flag_name>". + // For example: --python_launcher=--host_python_launcher + // have the --<another_flag_name> flag default value in the target config but also make sure + // that it won't propagate to the exec config by setting the scope to "target". + Label anotherFlag = Label.parseCanonicalUnchecked(scopeType.scopeType().substring(7)); + Target anotherFlagTarget = getTarget(env, anotherFlag, scopedFlag.getPackageIdentifier()); + if (anotherFlagTarget == null) { + return null; + } + + if (!key.getBuildOptions().getStarlarkOptions().containsKey(anotherFlag)) { + fullyResolvedBuildOptionsBuilder = + fullyResolvedBuildOptionsBuilder.addStarlarkOption( + anotherFlag, + anotherFlagTarget + .getAssociatedRule() + .getAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME)); + } + } } // get PROJECT.scl files for each scoped flag that is not universal @@ -202,7 +224,7 @@ @Nullable private Object getOnleaveScopeValue(Target target) { var attrs = RawAttributeMapper.of(target.getAssociatedRule()); - if (!attrs.has("on_leave_scope")) { + if (!attrs.isAttributeValueExplicitlySpecified("on_leave_scope")) { // do nothing if on_leave_scope is not set. return null; }
diff --git a/src/main/java/com/google/devtools/common/options/OptionsParser.java b/src/main/java/com/google/devtools/common/options/OptionsParser.java index 59dff45..94ddaf4 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsParser.java +++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java
@@ -285,7 +285,7 @@ private ImmutableSortedMap<String, Object> starlarkOptions = ImmutableSortedMap.of(); // scopes for starlark options private ImmutableSortedMap<String, String> scopesAttributes = ImmutableSortedMap.of(); - private final ImmutableSortedMap<String, Object> onLeaveScopeValues = ImmutableSortedMap.of(); + private ImmutableSortedMap<String, Object> onLeaveScopeValues = ImmutableSortedMap.of(); private final Map<String, String> aliases = new HashMap<>(); private boolean success = true; @@ -356,6 +356,10 @@ this.scopesAttributes = ImmutableSortedMap.copyOf(scopesAttributes); } + public void setOnLeaveScopeValues(Map<String, Object> onLeaveScopeValues) { + this.onLeaveScopeValues = ImmutableSortedMap.copyOf(onLeaveScopeValues); + } + public void parseAndExitUponError(String[] args) { parseAndExitUponError(OptionPriority.PriorityCategory.COMMAND_LINE, "unknown", args); }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValueTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValueTest.java index d62904a..d246cc0 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValueTest.java
@@ -471,7 +471,7 @@ string_flag = rule( implementation = lambda ctx: [], build_setting = config.string(flag = True), - attrs = {"scope": attr.string(values = ["target", "universal"])}, + attrs = {"scope": attr.string(), "on_leave_scope": attr.string()}, ) """); scratch.file( @@ -492,6 +492,21 @@ build_setting_default = "default", scope = "universal", ) + string_flag( + name = "flag_in_exec_config_set_to_another_value", + build_setting_default = "default", + scope = "target", + on_leave_scope = "another_value" + ) + string_flag( + name = "another_flag", + build_setting_default = "default", + ) + string_flag( + name = "flag_in_exec_config_reference_another_flag_value", + build_setting_default = "default", + scope = "exec:--//test:another_flag", + ) """); BuildConfigurationValue execConfig = @@ -502,7 +517,13 @@ "//test:target_scope", "custom", "//test:universal_scope", - "custom"), + "custom", + "//test:flag_in_exec_config_set_to_another_value", + "target_value", + "//test:flag_in_exec_config_reference_another_flag_value", + "target_value", + "//test:another_flag", + "default"), "--experimental_exclude_starlark_flags_from_exec_config=" + (propagateByDefault ? "false" : "true")); @@ -512,10 +533,21 @@ Label.parseCanonicalUnchecked("//test:universal_scope"), "custom", Label.parseCanonicalUnchecked("//test:default_scope"), - "custom"); + "custom", + Label.parseCanonicalUnchecked("//test:another_flag"), + "default"); } else { assertThat(execConfig.getOptions().getStarlarkOptions()) - .containsExactly(Label.parseCanonicalUnchecked("//test:universal_scope"), "custom"); + .containsExactly( + Label.parseCanonicalUnchecked("//test:universal_scope"), + "custom", + Label.parseCanonicalUnchecked("//test:flag_in_exec_config_set_to_another_value"), + "another_value", + Label.parseCanonicalUnchecked( + "//test:flag_in_exec_config_reference_another_flag_value"), + "default", + Label.parseCanonicalUnchecked("//test:another_flag"), + "default"); } }