From 771c6414ccbe064b3d30ac13be384f1ddc5ec09f Mon Sep 17 00:00:00 2001 From: cparsons Date: Fri, 19 Apr 2019 07:45:37 -0700 Subject: [PATCH] Automated rollback of commit 36f093abbfec386ef57f26ac479394f597881169. *** Reason for rollback *** re-evaluation that --define should probably not be exposed as a native option *** Original change description *** Allow *allowMultiple* options to be *set* by Starlark transitions. Specifically helpful for --define. Work towards #5574 RELNOTES: None. PiperOrigin-RevId: 244357629 --- .../skylark/FunctionTransitionUtil.java | 37 ++++---------- .../StarlarkRuleTransitionProviderTest.java | 48 ------------------- src/test/shell/bazel/whitelist_test.sh | 2 +- 3 files changed, 9 insertions(+), 78 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 c014b8a641de1a..07f2649a755bec 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,15 +33,12 @@ 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.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; @@ -257,35 +254,17 @@ private static BuildOptions applyTransition( OptionDefinition def = optionInfo.getDefinition(); Field field = def.getField(); FragmentOptions options = buildOptions.get(optionInfo.getOptionClass()); - - 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 + "'"); - } + 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 { - 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)); + throw new EvalException( + starlarkTransition.getLocationForErrorReporting(), + "Invalid value type for option '" + optionName + "'"); } } catch (IllegalAccessException e) { - throw new EvalException( - starlarkTransition.getLocationForErrorReporting(), + throw new RuntimeException( "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 c56d4eb47007c3..fd9bbe00da9229 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,7 +17,6 @@ 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; @@ -29,12 +28,9 @@ 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; @@ -58,16 +54,6 @@ 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. */ @@ -893,38 +879,4 @@ 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 8c4ea1b1144623..e3c2b1216b63c4 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\"\]\]" }