Integrate starlark options with select() via the flag_values attribute of config_setting. This includes adding an implicit provider to every build setting rule that knows its type and its default value. PiperOrigin-RevId: 247508938
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildSettingProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildSettingProvider.java new file mode 100644 index 0000000..e4aa803 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildSettingProvider.java
@@ -0,0 +1,48 @@ +// Copyright 2019 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.analysis; + +import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.packages.BuildSetting; +import com.google.devtools.build.lib.packages.RequiredProviders; +import com.google.devtools.build.lib.syntax.Type; + +/** + * A native provider to allow select()s to know the type and default value when selecting on build + * settings + */ +public class BuildSettingProvider implements TransitiveInfoProvider { + + public static final RequiredProviders REQUIRE_BUILD_SETTING_PROVIDER = + RequiredProviders.acceptAnyBuilder() + .addNativeSet(ImmutableSet.of(BuildSettingProvider.class)) + .build(); + + private final BuildSetting buildSetting; + private final Object defaultValue; + + public BuildSettingProvider(BuildSetting buildSetting, Object defaultValue) { + this.buildSetting = buildSetting; + this.defaultValue = defaultValue; + } + + public Type<?> getType() { + return buildSetting.getType(); + } + + public Object getDefaultValue() { + return defaultValue; + } +}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java index 2ee642b..7c2c089 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
@@ -14,6 +14,7 @@ package com.google.devtools.build.lib.analysis; import static com.google.devtools.build.lib.analysis.ExtraActionUtils.createExtraActionProvider; +import static com.google.devtools.build.lib.packages.RuleClass.Builder.SKYLARK_BUILD_SETTING_DEFAULT_ATTR_NAME; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; @@ -44,6 +45,7 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.events.Location; +import com.google.devtools.build.lib.packages.BuildSetting; import com.google.devtools.build.lib.packages.InfoInterface; import com.google.devtools.build.lib.packages.NativeProvider; import com.google.devtools.build.lib.packages.Provider; @@ -184,6 +186,15 @@ } } + if (ruleContext.getRule().isBuildSetting()) { + BuildSetting buildSetting = ruleContext.getRule().getRuleClassObject().getBuildSetting(); + Object defaultValue = + ruleContext + .attributes() + .get(SKYLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, buildSetting.getType()); + addProvider(BuildSettingProvider.class, new BuildSettingProvider(buildSetting, defaultValue)); + } + TransitiveInfoProviderMap providers = providersBuilder.build(); if (ruleContext.getRule().isAnalysisTest()) {
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkDefinedConfigTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkDefinedConfigTransition.java index 67968e7..ee16fef 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkDefinedConfigTransition.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkDefinedConfigTransition.java
@@ -74,6 +74,7 @@ public List<String> getOutputs() { return outputs; } + /** * Returns the location of the Starlark code responsible for determining the transition's changed
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkRuleTransitionProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkRuleTransitionProvider.java index bf7db5a..355b636 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkRuleTransitionProvider.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkRuleTransitionProvider.java
@@ -79,7 +79,6 @@ FunctionPatchTransition( StarlarkDefinedConfigTransition starlarkDefinedConfigTransition, Rule rule) { super(starlarkDefinedConfigTransition); - LinkedHashMap<String, Object> attributes = new LinkedHashMap<>(); RawAttributeMapper attributeMapper = RawAttributeMapper.of(rule); for (Attribute attribute : rule.getAttributes()) {
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Rule.java b/src/main/java/com/google/devtools/build/lib/packages/Rule.java index bf3ceaf..8de293a 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Rule.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Rule.java
@@ -195,6 +195,10 @@ return ruleClass.hasAnalysisTestTransition(); } + public boolean isBuildSetting() { + return ruleClass.getBuildSetting() != null; + } + /** * Returns true iff there were errors while constructing this rule, such as * attributes with missing values or values of the wrong type.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagProvider.java b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagProvider.java index cbeec71..fe7fb32 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagProvider.java +++ b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagProvider.java
@@ -16,11 +16,13 @@ import com.google.common.base.Predicate; import com.google.common.base.Predicates; +import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.analysis.TransitiveInfoCollection; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.events.Location; import com.google.devtools.build.lib.packages.NativeInfo; import com.google.devtools.build.lib.packages.NativeProvider; +import com.google.devtools.build.lib.packages.RequiredProviders; import com.google.devtools.build.lib.packages.SkylarkProviderIdentifier; import com.google.devtools.build.lib.skylarkbuildapi.config.ConfigFeatureFlagProviderApi; import com.google.devtools.build.lib.syntax.Environment; @@ -37,6 +39,9 @@ /** Skylark constructor and identifier for ConfigFeatureFlagProvider. */ static final NativeProvider<ConfigFeatureFlagProvider> SKYLARK_CONSTRUCTOR = new Constructor(); + static final RequiredProviders REQUIRE_CONFIG_FEATURE_FLAG_PROVIDER = + RequiredProviders.acceptAnyBuilder().addSkylarkSet(ImmutableSet.of(id())).build(); + private final String value; private final Predicate<String> validityPredicate;
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 2ce0e7b..828f0a7 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
@@ -238,7 +238,6 @@ attr(FLAG_SETTINGS_ATTRIBUTE, LABEL_KEYED_STRING_DICT) .undocumented("the feature flag feature has not yet been launched") .allowedFileTypes() - .mandatoryProviders(ImmutableList.of(ConfigFeatureFlagProvider.id())) .nonconfigurable(NONCONFIGURABLE_ATTRIBUTE_REASON)) /* <!-- #BLAZE_RULE(config_setting).ATTRIBUTE(constraint_values) --> The minimum set of <code>constraint_values</code> that the target platform must specify
diff --git a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java index 51b44d3..5d16409 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java +++ b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java
@@ -29,6 +29,7 @@ import com.google.common.collect.Multiset; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; import com.google.devtools.build.lib.analysis.AliasProvider; +import com.google.devtools.build.lib.analysis.BuildSettingProvider; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.FileProvider; import com.google.devtools.build.lib.analysis.FilesToRunProvider; @@ -57,6 +58,7 @@ import com.google.devtools.build.lib.packages.RuleErrorConsumer; import com.google.devtools.build.lib.rules.config.ConfigRuleClasses.ConfigSettingRule; import com.google.devtools.build.lib.syntax.Type; +import com.google.devtools.build.lib.syntax.Type.ConversionException; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.common.options.OptionsParser; import com.google.devtools.common.options.OptionsParsingException; @@ -108,16 +110,17 @@ return null; } - boolean nativeFlagsMatch = - matchesConfig( - nativeFlagSettings.entries(), - BuildConfigurationOptionDetails.get(ruleContext.getConfiguration()), - ruleContext); + TransitiveOptionDetails optionDetails = + BuildConfigurationOptionDetails.get(ruleContext.getConfiguration()); - ConfigFeatureFlagMatch featureFlags = - ConfigFeatureFlagMatch.fromAttributeValueAndPrerequisites( + boolean nativeFlagsMatch = + matchesConfig(nativeFlagSettings.entries(), optionDetails, ruleContext); + + UserDefinedFlagMatch userDefinedFlags = + UserDefinedFlagMatch.fromAttributeValueAndPrerequisites( userDefinedFlagSettings, ruleContext.getPrerequisites(ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE, Mode.TARGET), + optionDetails, ruleContext); boolean constraintValuesMatch = constraintValuesMatch(ruleContext); @@ -130,8 +133,8 @@ new ConfigMatchingProvider( ruleContext.getLabel(), nativeFlagSettings, - featureFlags.getSpecifiedFlagValues(), - nativeFlagsMatch && featureFlags.matches() && constraintValuesMatch); + userDefinedFlags.getSpecifiedFlagValues(), + nativeFlagsMatch && userDefinedFlags.matches() && constraintValuesMatch); return new RuleConfiguredTargetBuilder(ruleContext) .addProvider(RunfilesProvider.class, RunfilesProvider.EMPTY) @@ -368,14 +371,13 @@ return actualList.contains(expectedSingleValue); } - private static final class ConfigFeatureFlagMatch { + private static final class UserDefinedFlagMatch { private final boolean matches; private final ImmutableMap<Label, String> specifiedFlagValues; private static final Joiner QUOTED_COMMA_JOINER = Joiner.on("', '"); - private ConfigFeatureFlagMatch( - boolean matches, ImmutableMap<Label, String> specifiedFlagValues) { + private UserDefinedFlagMatch(boolean matches, ImmutableMap<Label, String> specifiedFlagValues) { this.matches = matches; this.specifiedFlagValues = specifiedFlagValues; } @@ -386,7 +388,7 @@ } /** Gets the specified flag values, with aliases converted to their original targets' labels. */ - public ImmutableMap<Label, String> getSpecifiedFlagValues() { + ImmutableMap<Label, String> getSpecifiedFlagValues() { return specifiedFlagValues; } @@ -401,20 +403,16 @@ return targetsToAliases.build(); } - public static ConfigFeatureFlagMatch fromAttributeValueAndPrerequisites( + static UserDefinedFlagMatch fromAttributeValueAndPrerequisites( Map<Label, String> attributeValue, Iterable<? extends TransitiveInfoCollection> prerequisites, + TransitiveOptionDetails optionDetails, RuleErrorConsumer errors) { Map<Label, String> specifiedFlagValues = new LinkedHashMap<>(); boolean matches = true; boolean foundDuplicate = false; for (TransitiveInfoCollection target : prerequisites) { - ConfigFeatureFlagProvider provider = ConfigFeatureFlagProvider.fromTarget(target); - // We know the provider exists because only labels with ConfigFeatureFlagProvider can be - // added to this attribute. - assert provider != null; - Label actualLabel = target.getLabel(); Label specifiedLabel = AliasProvider.getDependencyLabel(target); String specifiedValue = attributeValue.get(specifiedLabel); @@ -423,17 +421,52 @@ } specifiedFlagValues.put(actualLabel, specifiedValue); - if (!provider.isValidValue(specifiedValue)) { + if (target.satisfies(ConfigFeatureFlagProvider.REQUIRE_CONFIG_FEATURE_FLAG_PROVIDER)) { + // config_feature_flag + ConfigFeatureFlagProvider provider = ConfigFeatureFlagProvider.fromTarget(target); + if (!provider.isValidValue(specifiedValue)) { + errors.attributeError( + ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE, + String.format( + "error while parsing user-defined configuration values: " + + "'%s' is not a valid value for '%s'", + specifiedValue, specifiedLabel)); + matches = false; + continue; + } + if (!provider.getFlagValue().equals(specifiedValue)) { + matches = false; + } + } else if (target.satisfies(BuildSettingProvider.REQUIRE_BUILD_SETTING_PROVIDER)) { + // build setting + BuildSettingProvider provider = target.getProvider(BuildSettingProvider.class); + Object configurationValue = + optionDetails.getOptionValue(specifiedLabel) != null + ? optionDetails.getOptionValue(specifiedLabel) + : provider.getDefaultValue(); + Object convertedSpecifiedValue; + try { + convertedSpecifiedValue = provider.getType().convert(specifiedValue, specifiedLabel); + } catch (ConversionException e) { + errors.attributeError( + ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE, + String.format( + "error while parsing user-defined configuration values: " + + "'%s' cannot be converted to %s type %s", + specifiedValue, specifiedLabel, provider.getType())); + matches = false; + continue; + } + if (!configurationValue.equals(convertedSpecifiedValue)) { + matches = false; + } + } else { errors.attributeError( ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE, String.format( "error while parsing user-defined configuration values: " - + "'%s' is not a valid value for '%s'", - specifiedValue, specifiedLabel)); - matches = false; - continue; - } - if (!provider.getFlagValue().equals(specifiedValue)) { + + "%s keys must be build settings or feature flags and %s is not", + ConfigSettingRule.FLAG_SETTINGS_ATTRIBUTE, specifiedLabel)); matches = false; } } @@ -454,11 +487,10 @@ actualLabel, QUOTED_COMMA_JOINER.join(aliasList))); } } - matches = false; } - return new ConfigFeatureFlagMatch(matches, ImmutableMap.copyOf(specifiedFlagValues)); + return new UserDefinedFlagMatch(matches, ImmutableMap.copyOf(specifiedFlagValues)); } } }
diff --git a/src/test/java/com/google/devtools/build/lib/rules/config/ConfigSettingTest.java b/src/test/java/com/google/devtools/build/lib/rules/config/ConfigSettingTest.java index 0e018c4..b40c670 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/config/ConfigSettingTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/config/ConfigSettingTest.java
@@ -1227,23 +1227,6 @@ } @Test - public void forbidsNonConfigFeatureFlagRulesForFlagValues() throws Exception { - checkError("test", "invalid_flag", - "in flag_values attribute of config_setting rule //test:invalid_flag: " - + "'//test:genrule' does not have mandatory providers: 'FeatureFlagInfo'", - "config_setting(", - " name = 'invalid_flag',", - " flag_values = {", - " ':genrule': 'lolz',", - " })", - "genrule(", - " name = 'genrule',", - " outs = ['output'],", - " cmd = 'echo >$@',", - " )"); - } - - @Test public void requiresValidValueForFlagValues() throws Exception { useConfiguration("--enforce_transitive_configs_for_config_feature_flag"); checkError( @@ -1293,6 +1276,215 @@ } @Test + public void buildsettings_matchesFromDefault() throws Exception { + setSkylarkSemanticsOptions("--experimental_build_setting_api=true"); + + scratch.file( + "test/build_settings.bzl", + "def _impl(ctx):", + " return []", + "string_flag = rule(implementation = _impl, build_setting = config.string(flag = True))"); + scratch.file( + "test/BUILD", + "load('//test:build_settings.bzl', 'string_flag')", + "config_setting(", + " name = 'match',", + " flag_values = {", + " ':cheese': 'parmesan',", + " },", + ")", + "string_flag(name = 'cheese', build_setting_default = 'parmesan')"); + assertThat(getConfigMatchingProvider("//test:match").matches()).isTrue(); + } + + @Test + public void buildsettings_matchesFromCommandLine() throws Exception { + setSkylarkSemanticsOptions("--experimental_build_setting_api=true"); + useConfiguration(ImmutableMap.of("//test:cheese", "gouda")); + + scratch.file( + "test/build_settings.bzl", + "def _impl(ctx):", + " return []", + "string_flag = rule(implementation = _impl, build_setting = config.string(flag = True))"); + scratch.file( + "test/BUILD", + "load('//test:build_settings.bzl', 'string_flag')", + "config_setting(", + " name = 'match',", + " flag_values = {", + " ':cheese': 'gouda',", + " },", + ")", + "string_flag(name = 'cheese', build_setting_default = 'parmesan')"); + assertThat(getConfigMatchingProvider("//test:match").matches()).isTrue(); + } + + @Test + public void buildsettings_doesntMatch() throws Exception { + setSkylarkSemanticsOptions("--experimental_build_setting_api=true"); + useConfiguration(ImmutableMap.of("//test:cheese", "gouda")); + + scratch.file( + "test/build_settings.bzl", + "def _impl(ctx):", + " return []", + "string_flag = rule(implementation = _impl, build_setting = config.string(flag = True))"); + scratch.file( + "test/BUILD", + "load('//test:build_settings.bzl', 'string_flag')", + "config_setting(", + " name = 'match',", + " flag_values = {", + " ':cheese': 'parmesan',", + " },", + ")", + "string_flag(name = 'cheese', build_setting_default = 'parmesan')"); + assertThat(getConfigMatchingProvider("//test:match").matches()).isFalse(); + } + + @Test + public void buildsettings_badType() throws Exception { + setSkylarkSemanticsOptions("--experimental_build_setting_api=true"); + + scratch.file( + "test/build_settings.bzl", + "def _impl(ctx):", + " return []", + "int_flag = rule(implementation = _impl, build_setting = config.int(flag = True))"); + scratch.file( + "test/BUILD", + "load('//test:build_settings.bzl', 'int_flag')", + "config_setting(", + " name = 'match',", + " flag_values = {", + " ':wishes': 'gouda',", + " },", + ")", + "int_flag(name = 'wishes', build_setting_default = 3)"); + + reporter.removeHandler(failFastHandler); + getConfiguredTarget("//test:match"); + assertContainsEvent("'gouda' cannot be converted to //test:wishes type int"); + } + + @Test + public void notBuildSettingOrFeatureFlag() throws Exception { + setSkylarkSemanticsOptions("--experimental_build_setting_api=true"); + + scratch.file( + "test/rules.bzl", + "def _impl(ctx):", + " return DefaultInfo()", + "default_info_rule = rule(implementation = _impl)"); + scratch.file( + "test/BUILD", + "load('//test:rules.bzl', 'default_info_rule')", + "config_setting(", + " name = 'match',", + " flag_values = {", + " ':cheese': 'gouda',", + " },", + ")", + "default_info_rule(name = 'cheese')"); + + reporter.removeHandler(failFastHandler); + getConfiguredTarget("//test:match"); + assertContainsEvent( + "flag_values keys must be build settings or feature flags and //test:cheese is not"); + } + + @Test + public void buildsettingsMatch_featureFlagsMatch() throws Exception { + setSkylarkSemanticsOptions("--experimental_build_setting_api=true"); + useConfiguration("--enforce_transitive_configs_for_config_feature_flag"); + + scratch.file( + "test/build_settings.bzl", + "def _impl(ctx):", + " return []", + "string_flag = rule(implementation = _impl, build_setting = config.string(flag = True))"); + scratch.file( + "test/BUILD", + "load('//test:build_settings.bzl', 'string_flag')", + "config_setting(", + " name = 'match',", + " flag_values = {", + " ':cheese': 'parmesan',", + " ':flag': 'right',", + " },", + " transitive_configs = [':flag'],", + ")", + "string_flag(name = 'cheese', build_setting_default = 'parmesan')", + "config_feature_flag(", + " name = 'flag',", + " allowed_values = ['right'],", + " default_value = 'right',", + ")"); + assertThat(getConfigMatchingProvider("//test:match").matches()).isTrue(); + } + + @Test + public void buildsettingsMatch_featureFlagsDontMatch() throws Exception { + setSkylarkSemanticsOptions("--experimental_build_setting_api=true"); + useConfiguration("--enforce_transitive_configs_for_config_feature_flag"); + + scratch.file( + "test/build_settings.bzl", + "def _impl(ctx):", + " return []", + "string_flag = rule(implementation = _impl, build_setting = config.string(flag = True))"); + scratch.file( + "test/BUILD", + "load('//test:build_settings.bzl', 'string_flag')", + "config_setting(", + " name = 'match',", + " flag_values = {", + " ':cheese': 'parmesan',", + " ':flag': 'wrong',", + " },", + " transitive_configs = [':flag'],", + ")", + "string_flag(name = 'cheese', build_setting_default = 'parmesan')", + "config_feature_flag(", + " name = 'flag',", + " allowed_values = ['right', 'wrong'],", + " default_value = 'right',", + ")"); + assertThat(getConfigMatchingProvider("//test:match").matches()).isFalse(); + } + + @Test + public void buildsettingsDontMatch_featureFlagsMatch() throws Exception { + setSkylarkSemanticsOptions("--experimental_build_setting_api=true"); + useConfiguration("--enforce_transitive_configs_for_config_feature_flag"); + + scratch.file( + "test/build_settings.bzl", + "def _impl(ctx):", + " return []", + "string_flag = rule(implementation = _impl, build_setting = config.string(flag = True))"); + scratch.file( + "test/BUILD", + "load('//test:build_settings.bzl', 'string_flag')", + "config_setting(", + " name = 'match',", + " flag_values = {", + " ':cheese': 'gouda',", + " ':flag': 'right',", + " },", + " transitive_configs = [':flag'],", + ")", + "string_flag(name = 'cheese', build_setting_default = 'parmesan')", + "config_feature_flag(", + " name = 'flag',", + " allowed_values = ['right'],", + " default_value = 'right',", + ")"); + assertThat(getConfigMatchingProvider("//test:match").matches()).isFalse(); + } + + @Test public void constraintValue() throws Exception { scratch.file( "test/BUILD",