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 07f2649a755bec..da863168ae8d0f 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 @@ -33,12 +33,16 @@ import com.google.devtools.build.lib.syntax.Runtime; import com.google.devtools.build.lib.syntax.Runtime.NoneType; import com.google.devtools.build.lib.syntax.SkylarkDict; +import com.google.devtools.build.lib.syntax.SkylarkList; +import com.google.devtools.build.lib.syntax.SkylarkList.MutableList; import com.google.devtools.common.options.OptionDefinition; import com.google.devtools.common.options.OptionsParser; import com.google.devtools.common.options.OptionsParsingException; import java.lang.reflect.Field; import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; +import java.util.ArrayList; +import java.util.Collections; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; @@ -175,7 +179,33 @@ static SkylarkDict buildSettings( Field field = optionInfo.getDefinition().getField(); FragmentOptions options = buildOptions.get(optionInfo.getOptionClass()); Object optionValue = field.get(options); - + if (optionInfo.getDefinition().allowsMultiple()) { + List optionValueList = (List) optionValue; + if (optionValueList.isEmpty()) { + optionValue = MutableList.empty(); + } else { + if (optionValueList.get(0) instanceof Map.Entry) { + SkylarkDict valueDict = SkylarkDict.withMutability(mutability); + for (Map.Entry singleValue : (List) optionValueList) { + valueDict.put( + singleValue.getKey().toString(), + singleValue.getValue().toString(), + starlarkTransition.getLocationForErrorReporting(), + mutability); + } + optionValue = valueDict; + } + } + } else { + if (optionValue instanceof Map.Entry) { + SkylarkDict valueDict = SkylarkDict.withMutability(mutability); + valueDict.put( + ((Map.Entry) optionValue).getKey().toString(), + ((Map.Entry) optionValue).getValue().toString(), + starlarkTransition.getLocationForErrorReporting(), + mutability); + } + } dict.put(optionKey, optionValue == null ? Runtime.NONE : optionValue, null, mutability); } catch (IllegalAccessException e) { // These exceptions should not happen, but if they do, throw a RuntimeException. @@ -254,17 +284,35 @@ private static BuildOptions applyTransition( 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)); + + if (!def.allowsMultiple()) { + 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 + "'"); + } } else { - throw new EvalException( - starlarkTransition.getLocationForErrorReporting(), - "Invalid value type for option '" + optionName + "'"); + SkylarkList rawValues = + optionValue instanceof SkylarkList + ? (SkylarkList) optionValue + : SkylarkList.createImmutable(Collections.singletonList(optionValue)); + List allValues = new ArrayList<>(rawValues.size()); + for (Object singleValue : rawValues) { + if (singleValue instanceof String) { + allValues.add(def.getConverter().convert((String) singleValue)); + } else { + allValues.add(singleValue); + } + } + field.set(options, ImmutableList.copyOf(allValues)); } } catch (IllegalAccessException e) { - throw new RuntimeException( + throw new EvalException( + starlarkTransition.getLocationForErrorReporting(), "IllegalAccess for option " + optionName + ": " + e.getMessage()); } catch (OptionsParsingException e) { throw new EvalException( 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 c5bf9d0132e5b2..ad5ab6c9f557d7 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 @@ -17,6 +17,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Maps; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.config.BuildConfiguration.EmptyToNullLabelConverter; import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment; @@ -28,9 +29,12 @@ import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.testutil.TestRuleClassProvider; +import com.google.devtools.common.options.Converters; import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionDocumentationCategory; import com.google.devtools.common.options.OptionEffectTag; +import java.util.List; +import java.util.Map; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -54,6 +58,16 @@ public static class DummyTestOptions extends FragmentOptions { effectTags = {OptionEffectTag.NO_OP}, help = "An option that is sometimes set to null.") public Label nullable; + + @Option( + name = "allow_multiple", + converter = Converters.AssignmentConverter.class, + defaultValue = "", + allowMultiple = true, + documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS, + effectTags = {OptionEffectTag.CHANGES_INPUTS, OptionEffectTag.AFFECTS_OUTPUTS}, + help = "Each --define option specifies an assignment for a build variable.") + public List> allowMultiple; } /** Loads a new {link @DummyTestFragment} instance. */ @@ -854,4 +868,38 @@ public void testWhitelistOnTargetsStillWorks() throws Exception { assertThat(configuration.getOptions().get(TestOptions.class).testArguments) .containsExactly("post-transition"); } + + @Test + public void testWriteNativeOption_allowMultipleOption() throws Exception { + writeWhitelistFile(); + scratch.file( + "test/transitions.bzl", + "def _impl(settings, attr):", + " return {", + " '//command_line_option:allow_multiple': ['APRIL=SHOWERS', 'MAY=FLOWERS'],", + " }", + "my_transition = transition(implementation = _impl, inputs = [],", + " outputs = ['//command_line_option:allow_multiple'])"); + scratch.file( + "test/rules.bzl", + "load('//test:transitions.bzl', 'my_transition')", + "def _impl(ctx):", + " return []", + "my_rule = rule(", + " implementation = _impl,", + " cfg = my_transition,", + " attrs = {", + " '_whitelist_function_transition': attr.label(", + " default = '//tools/whitelists/function_transition_whitelist',", + " ),", + " })"); + scratch.file("test/BUILD", "load('//test:rules.bzl', 'my_rule')", "my_rule(name = 'test')"); + + useConfiguration("--allow_multiple=APRIL=FOOLS"); + + BuildConfiguration configuration = getConfiguration(getConfiguredTarget("//test")); + assertThat(configuration.getOptions().get(DummyTestOptions.class).allowMultiple) + .containsExactly( + Maps.immutableEntry("APRIL", "SHOWERS"), Maps.immutableEntry("MAY", "FLOWERS")); + } } diff --git a/src/test/shell/bazel/whitelist_test.sh b/src/test/shell/bazel/whitelist_test.sh index e3c2b1216b63c4..8c4ea1b1144623 100755 --- a/src/test/shell/bazel/whitelist_test.sh +++ b/src/test/shell/bazel/whitelist_test.sh @@ -99,7 +99,7 @@ EOF --noimplicit_deps --experimental_starlark_config_transitions \ >& $TEST_log || fail "failed to query //vinegar" expect_log "@secret_ingredient//hotsauce" - expect_log "test_arg:\[hotlanta\] -> \[\[\"tapatio\"\]\]" + expect_log "test_arg:\[hotlanta\] -> \[\[tapatio\]\]" }