Skip to content

Commit

Permalink
Allow *allowMultiple* options to be *set* by Starlark transitions.
Browse files Browse the repository at this point in the history 
Specifically helpful for --define.

Work towards #5574

RELNOTES: None.
PiperOrigin-RevId: 242736849
  • Loading branch information
@juliexxia @copybara-github
juliexxia authored and copybara-github committed Apr 9, 2019
1 parent 7b2cb35 commit 36f093a
Show file tree
Hide file tree
Showing 3 changed files with 106 additions and 10 deletions.
66 changes: 57 additions & 9 deletions src/main/java/com/google/devtools/build/lib/analysis/skylark/FunctionTransitionUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -175,7 +179,33 @@ static SkylarkDict<String, Object> 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<String, String> valueDict = SkylarkDict.withMutability(mutability);
for (Map.Entry singleValue : (List<Map.Entry>) optionValueList) {
valueDict.put(
singleValue.getKey().toString(),
singleValue.getValue().toString(),
starlarkTransition.getLocationForErrorReporting(),
mutability);
}
optionValue = valueDict;
}
}
} else {
if (optionValue instanceof Map.Entry) {
SkylarkDict<String, String> 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.
Expand Down Expand Up @@ -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<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));
}
} catch (IllegalAccessException e) {
throw new RuntimeException(
throw new EvalException(
starlarkTransition.getLocationForErrorReporting(),
"IllegalAccess for option " + optionName + ": " + e.getMessage());
} catch (OptionsParsingException e) {
throw new EvalException(
Expand Down
48 changes: 48 additions & 0 deletions src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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<Map.Entry<String, String>> allowMultiple;
}

/** Loads a new {link @DummyTestFragment} instance. */
Expand Down Expand Up @@ -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"));
}
}
2 changes: 1 addition & 1 deletion src/test/shell/bazel/whitelist_test.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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\]\]"

}

Expand Down

0 comments on commit 36f093a

Please sign in to comment.