From 22de6cdb95b9f730372221f3f048ec55e24d9841 Mon Sep 17 00:00:00 2001 From: juliexxia Date: Wed, 9 Jan 2019 07:25:23 -0800 Subject: [PATCH] Allow starlark-defined transitions to write build settings. Previously we just operated on native options (for a few whitelisted users). While this CL allows writing build options, it does not validate that the values the transitions set for build settings are of the proper type. Nor does it allow reading of all build options. That will be coming in a follow up CL. Test added to note these gaps. RELNOTES: None. PiperOrigin-RevId: 228513456 --- .../skylark/FunctionTransitionUtil.java | 124 ++++++++-------- .../lib/rules/config/ConfigGlobalLibrary.java | 16 ++- .../StarlarkAttrTransitionProviderTest.java | 74 ++++++++++ .../StarlarkRuleTransitionProviderTest.java | 136 ++++++++++++++++++ 4 files changed, 278 insertions(+), 72 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/FunctionTransitionUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/FunctionTransitionUtil.java index d0ee6509daf563..9014d6e7601a8d 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/FunctionTransitionUtil.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/FunctionTransitionUtil.java @@ -26,6 +26,7 @@ import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.analysis.config.FragmentOptions; import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition; +import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.packages.StructImpl; import com.google.devtools.build.lib.syntax.EvalException; import com.google.devtools.build.lib.syntax.Mutability; @@ -80,9 +81,9 @@ static List applyAndValidate( validateFunctionOutputs(transitions, starlarkTransition); for (Map transition : transitions) { - BuildOptions options = buildOptions.clone(); - applyTransition(options, transition, optionInfoMap, starlarkTransition); - splitBuildOptions.add(options); + BuildOptions transitionedOptions = + applyTransition(buildOptions, transition, optionInfoMap, starlarkTransition); + splitBuildOptions.add(transitionedOptions); } return splitBuildOptions.build(); @@ -117,29 +118,6 @@ private static void validateFunctionOutputs( } } - /** - * Given a label-like string representing a command line option, returns the command line option - * string that it represents. This is a temporary measure to support command line options with - * strings that look "label-like", so that migrating users using this experimental syntax is - * easier later. - * - * @throws EvalException if the given string is not a valid format to represent to a command line - * option - */ - private static String commandLineOptionLabelToOption( - String label, StarlarkDefinedConfigTransition starlarkTransition) throws EvalException { - if (label.startsWith(COMMAND_LINE_OPTION_PREFIX)) { - return label.substring(COMMAND_LINE_OPTION_PREFIX.length()); - } else { - throw new EvalException( - starlarkTransition.getLocationForErrorReporting(), - String.format( - "Option key '%s' is of invalid form. " - + "Expected command line option to begin with %s", - label, COMMAND_LINE_OPTION_PREFIX)); - } - } - /** For all the options in the BuildOptions, build a map from option name to its information. */ private static Map buildOptionInfo(BuildOptions buildOptions) { ImmutableMap.Builder builder = new ImmutableMap.Builder<>(); @@ -220,62 +198,70 @@ static SkylarkDict buildSettings( * Apply the transition dictionary to the build option, using optionInfoMap to look up the option * info. * - * @param buildOptions the pre-transition build options - * @param newValues a map of option name: option value entries to override current option - * values in the buildOptions param - * @param optionInfoMap a map of option name: option info for all native options that may - * be accessed in this transition - * @param starlarkTransition transition object that is being applied. Used for error reporting - * and checking for analysis testing + * @param buildOptionsToTransition the pre-transition build options + * @param newValues a map of option name: option value entries to override current option values + * in the buildOptions param + * @param optionInfoMap a map of option name: option info for all native options that may be + * accessed in this transition + * @param starlarkTransition transition object that is being applied. Used for error reporting and + * checking for analysis testing + * @return the post-transition build options * @throws EvalException If a requested option field is inaccessible */ - static void applyTransition( - BuildOptions buildOptions, + private static BuildOptions applyTransition( + BuildOptions buildOptionsToTransition, Map newValues, Map optionInfoMap, StarlarkDefinedConfigTransition starlarkTransition) throws EvalException { + BuildOptions buildOptions = buildOptionsToTransition.clone(); for (Map.Entry entry : newValues.entrySet()) { - String optionKey = entry.getKey(); - - // TODO(juliexxia): Handle keys which correspond to build_setting target labels instead - // of assuming every key is for a command line option. - String optionName = commandLineOptionLabelToOption(optionKey, starlarkTransition); + String optionName = entry.getKey(); Object optionValue = entry.getValue(); - // Convert NoneType to null. - if (optionValue instanceof NoneType) { - optionValue = null; - } + if (!optionName.startsWith(COMMAND_LINE_OPTION_PREFIX)) { + buildOptions = + BuildOptions.builder() + .merge(buildOptions) + .addStarlarkOption(Label.parseAbsoluteUnchecked(optionName), optionValue) + .build(); + } else { + optionName = optionName.substring(COMMAND_LINE_OPTION_PREFIX.length()); - try { - if (!optionInfoMap.containsKey(optionName)) { - throw new EvalException( - starlarkTransition.getLocationForErrorReporting(), - String.format( - "transition output '%s' does not correspond to a valid setting", optionKey)); + // Convert NoneType to null. + if (optionValue instanceof NoneType) { + optionValue = null; } - - OptionInfo optionInfo = optionInfoMap.get(optionName); - OptionDefinition def = optionInfo.getDefinition(); - Field field = def.getField(); - FragmentOptions options = buildOptions.get(optionInfo.getOptionClass()); - if (optionValue == null || def.getType().isInstance(optionValue)) { - field.set(options, optionValue); - } else if (optionValue instanceof String) { - field.set(options, def.getConverter().convert((String) optionValue)); - } else { + try { + if (!optionInfoMap.containsKey(optionName)) { + throw new EvalException( + starlarkTransition.getLocationForErrorReporting(), + String.format( + "transition output '%s' does not correspond to a valid setting", + entry.getKey())); + } + + OptionInfo optionInfo = optionInfoMap.get(optionName); + OptionDefinition def = optionInfo.getDefinition(); + Field field = def.getField(); + FragmentOptions options = buildOptions.get(optionInfo.getOptionClass()); + if (optionValue == null || def.getType().isInstance(optionValue)) { + field.set(options, optionValue); + } else if (optionValue instanceof String) { + field.set(options, def.getConverter().convert((String) optionValue)); + } else { + throw new EvalException( + starlarkTransition.getLocationForErrorReporting(), + "Invalid value type for option '" + optionName + "'"); + } + } catch (IllegalAccessException e) { + throw new RuntimeException( + "IllegalAccess for option " + optionName + ": " + e.getMessage()); + } catch (OptionsParsingException e) { throw new EvalException( starlarkTransition.getLocationForErrorReporting(), - "Invalid value type for option '" + optionName + "'"); + "OptionsParsingError for option '" + optionName + "': " + e.getMessage()); } - } catch (IllegalAccessException e) { - throw new RuntimeException( - "IllegalAccess for option " + optionName + ": " + e.getMessage()); - } catch (OptionsParsingException e) { - throw new EvalException( - starlarkTransition.getLocationForErrorReporting(), - "OptionsParsingError for option '" + optionName + "': " + e.getMessage()); } } @@ -286,6 +272,8 @@ static void applyTransition( buildConfigOptions.evaluatingForAnalysisTest = true; } updateOutputDirectoryNameFragment(buildConfigOptions, newValues); + + return buildOptions; } /** diff --git a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigGlobalLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigGlobalLibrary.java index eb9f1317ea37c2..be757be167bc31 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigGlobalLibrary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigGlobalLibrary.java @@ -16,6 +16,7 @@ import com.google.common.collect.Sets; import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition; +import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.events.Location; import com.google.devtools.build.lib.skylarkbuildapi.config.ConfigGlobalLibraryApi; import com.google.devtools.build.lib.skylarkbuildapi.config.ConfigurationTransitionApi; @@ -72,15 +73,22 @@ public ConfigurationTransitionApi analysisTestTransition( private void validateBuildSettingKeys( Iterable optionKeys, String keyErrorDescriptor, Location location) throws EvalException { - // TODO(juliexxia): Allow real labels, not just command line option placeholders. HashSet processedOptions = Sets.newHashSet(); for (String optionKey : optionKeys) { if (!optionKey.startsWith(COMMAND_LINE_OPTION_PREFIX)) { - throw new EvalException(location, - String.format("invalid transition %s '%s'. If this is intended as a native option, " - + "it must begin with //command_line_option:", keyErrorDescriptor, optionKey)); + try { + Label.parseAbsoluteUnchecked(optionKey); + } catch (IllegalArgumentException e) { + throw new EvalException( + location, + String.format( + "invalid transition %s '%s'. If this is intended as a native option, " + + "it must begin with //command_line_option:", + keyErrorDescriptor, optionKey), + e); + } } if (!processedOptions.add(optionKey)) { throw new EvalException(location, diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java index 803d59557909d1..2e3766f718d06d 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java @@ -18,8 +18,12 @@ import static org.junit.Assert.fail; import com.google.common.collect.ArrayListMultimap; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Iterables; import com.google.common.collect.ListMultimap; +import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; +import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.packages.util.BazelMockAndroidSupport; import java.util.List; import java.util.Map; @@ -702,6 +706,76 @@ public void testInvalidOutputKey_analysisTest() throws Exception { + "it must begin with //command_line_option:"); } + @Test + public void testTransitionOnBuildSetting() throws Exception { + setSkylarkSemanticsOptions( + "--experimental_starlark_config_transitions=true", "--experimental_build_setting_api"); + writeWhitelistFile(); + + scratch.file( + "test/skylark/build_settings.bzl", + "BuildSettingInfo = provider(fields = ['value'])", + "def _impl(ctx):", + " return [BuildSettingInfo(value = ctx.build_setting_value)]", + "string_flag = rule(implementation = _impl, build_setting = config.string(flag=True))"); + + scratch.file( + "test/skylark/rules.bzl", + "load('//test/skylark:build_settings.bzl', 'BuildSettingInfo')", + "def _transition_impl(settings, attr):", + " return {'//test:cute-animal-fact': 'puffins mate for life'}", + "my_transition = transition(", + " implementation = _transition_impl,", + " inputs = [],", + " outputs = ['//test:cute-animal-fact']", + ")", + "def _rule_impl(ctx):", + " return struct(dep = ctx.attr.dep)", + "my_rule = rule(", + " implementation = _rule_impl,", + " attrs = {", + " 'dep': attr.label(cfg = my_transition),", + " '_whitelist_function_transition': attr.label(", + " default = '//tools/whitelists/function_transition_whitelist'),", + " }", + ")", + "def _dep_rule_impl(ctx):", + " return [BuildSettingInfo(value = ctx.attr.fact[BuildSettingInfo].value)]", + "dep_rule_impl = rule(", + " implementation = _dep_rule_impl,", + " attrs = {", + " 'fact': attr.label(default = '//test/skylark:cute-animal-fact'),", + " }", + ")"); + + scratch.file( + "test/skylark/BUILD", + "load('//test/skylark:rules.bzl', 'my_rule')", + "load('//test/skylark:build_settings.bzl', 'string_flag')", + "my_rule(name = 'test', dep = ':dep')", + "my_rule(name = 'dep')", + "string_flag(", + " name = 'cute-animal-fact',", + " build_setting_default = 'cows produce more milk when they listen to soothing music',", + ")"); + + useConfiguration(ImmutableMap.of("//test:cute-animal-fact", "cats can't taste sugar")); + getConfiguredTarget("//test/skylark:test"); + + @SuppressWarnings("unchecked") + BuildConfiguration depConfiguration = + getConfiguration( + Iterables.getOnlyElement( + (List) getConfiguredTarget("//test/skylark:test").get("dep"))); + + assertThat( + depConfiguration + .getOptions() + .getStarlarkOptions() + .get(Label.parseAbsoluteUnchecked("//test:cute-animal-fact"))) + .isEqualTo("puffins mate for life"); + } + @Test public void testOptionConversionDynamicMode() throws Exception { // TODO(waltl): check that dynamic_mode is parsed properly. diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java index b77875ee26b598..eed27cc204f4f5 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java @@ -16,9 +16,11 @@ import static com.google.common.truth.Truth.assertThat; import static junit.framework.TestCase.fail; +import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.test.TestConfiguration.TestOptions; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; +import com.google.devtools.build.lib.cmdline.Label; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -270,4 +272,138 @@ public void testCannotTransitionWithoutFlag() throws Exception { + "development and subject to change at any time. Use " + "--experimental_starlark_config_transitions to use this experimental API."); } + + private void writeRulesBuildSettingsAndBUILDforBuildSettingTransitionTests() throws Exception { + scratch.file( + "test/rules.bzl", + "load('//test:transitions.bzl', 'my_transition')", + "def _rule_impl(ctx):", + " return []", + "my_rule = rule(", + " implementation = _rule_impl,", + " cfg = my_transition,", + ")"); + + 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:rules.bzl', 'my_rule')", + "load('//test:build_settings.bzl', 'string_flag')", + "my_rule(name = 'test')", + "string_flag(", + " name = 'cute-animal-fact',", + " build_setting_default = 'cows produce more milk when they listen to soothing music',", + ")"); + } + + @Test + public void testTransitionOnBuildSetting() throws Exception { + setSkylarkSemanticsOptions( + "--experimental_starlark_config_transitions=true", "--experimental_build_setting_api"); + scratch.file( + "test/transitions.bzl", + "def _transition_impl(settings, attr):", + " return {'//test:cute-animal-fact': 'puffins mate for life'}", + "my_transition = transition(", + " implementation = _transition_impl,", + " inputs = [],", + " outputs = ['//test:cute-animal-fact']", + ")"); + writeRulesBuildSettingsAndBUILDforBuildSettingTransitionTests(); + + useConfiguration(ImmutableMap.of("//test:cute-animal-fact", "cats can't taste sugar")); + BuildConfiguration configuration = getConfiguration(getConfiguredTarget("//test")); + assertThat( + configuration + .getOptions() + .getStarlarkOptions() + .get(Label.parseAbsoluteUnchecked("//test:cute-animal-fact"))) + .isEqualTo("puffins mate for life"); + } + + // TODO(juliexxia): flip this test when post-transition validation is implemented to prevent + // transitions from setting (for example) string-typed build settings to ints. + @Test + public void testCanSetBuildSettingToBadValue() throws Exception { + setSkylarkSemanticsOptions( + "--experimental_starlark_config_transitions=true", "--experimental_build_setting_api"); + scratch.file( + "test/transitions.bzl", + "def _transition_impl(settings, attr):", + " return {'//test:cute-animal-fact': 24}", + "my_transition = transition(", + " implementation = _transition_impl,", + " inputs = [],", + " outputs = ['//test:cute-animal-fact']", + ")"); + writeRulesBuildSettingsAndBUILDforBuildSettingTransitionTests(); + + useConfiguration(ImmutableMap.of("//test:cute-animal-fact", "cats can't taste sugar")); + BuildConfiguration configuration = getConfiguration(getConfiguredTarget("//test")); + assertThat( + configuration + .getOptions() + .getStarlarkOptions() + .get(Label.parseAbsoluteUnchecked("//test:cute-animal-fact"))) + .isEqualTo(24); + } + + // TODO(juliexxia): flip this test when post-transition validation is implemented to prevent + // transitions from setting non-existent build settings (note - we'll need to verify we can't + // set non-build setting targets either). + @Test + public void testCanSetNonexistantBuildSetting() throws Exception { + setSkylarkSemanticsOptions( + "--experimental_starlark_config_transitions=true", "--experimental_build_setting_api"); + scratch.file( + "test/transitions.bzl", + "def _transition_impl(settings, attr):", + " return {'//test:i-am-not-real': 'imaginary-friend'}", + "my_transition = transition(", + " implementation = _transition_impl,", + " inputs = [],", + " outputs = ['//test:i-am-not-real']", + ")"); + writeRulesBuildSettingsAndBUILDforBuildSettingTransitionTests(); + + BuildConfiguration configuration = getConfiguration(getConfiguredTarget("//test")); + assertThat( + configuration + .getOptions() + .getStarlarkOptions() + .get(Label.parseAbsoluteUnchecked("//test:i-am-not-real"))) + .isEqualTo("imaginary-friend"); + } + + // TODO(juliexxia): flip this test when we can read build settings. + @Test + public void testCantReadNonNativeBuildSetting() throws Exception { + setSkylarkSemanticsOptions( + "--experimental_starlark_config_transitions=true", "--experimental_build_setting_api"); + scratch.file( + "test/transitions.bzl", + "def _transition_impl(settings, attr):", + " return {'//test:cute-animal-fact': settings['//test:cute-animal-fact']+' ADDED'}", + "my_transition = transition(", + " implementation = _transition_impl,", + " inputs = ['//test:cute-animal-fact'],", + " outputs = ['//test:i-am-not-real']", + ")"); + writeRulesBuildSettingsAndBUILDforBuildSettingTransitionTests(); + + try { + getConfiguredTarget("//test"); + fail("Should have failed"); + } catch (RuntimeException e) { + assertThat(e) + .hasMessageThat() + .contains( + "transition inputs [//test:cute-animal-fact] do not correspond to valid settings"); + } + } }