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
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 c014b8a6..07f2649 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 @@
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(
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 c56d4eb..fd9bbe0 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 @@
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 @@
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 8c4ea1b..e3c2b12 100755
--- a/src/test/shell/bazel/whitelist_test.sh
+++ b/src/test/shell/bazel/whitelist_test.sh
@@ -99,7 +99,7 @@
--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\"\]\]"
}
