Automated rollback of commit 36f093a.

*** 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

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<Object> 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(

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<Map.Entry<String, String>> 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"));
-  }
 }

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\"\]\]"
 
 }