Support starlark split transitions returning a list of toOptions instead of a dict (the keys of the dict were never used).
RELNOTES: None.
PiperOrigin-RevId: 246008663
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkDefinedConfigTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkDefinedConfigTransition.java
index b6768ac..67968e7 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkDefinedConfigTransition.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkDefinedConfigTransition.java
@@ -27,6 +27,7 @@
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.Mutability;
import com.google.devtools.build.lib.syntax.SkylarkDict;
+import com.google.devtools.build.lib.syntax.SkylarkList;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
import java.util.List;
import java.util.Map;
@@ -98,7 +99,7 @@
* @throws EvalException if there is an error evaluating the transition
* @throws InterruptedException if evaluating the transition is interrupted
*/
- public abstract ImmutableList<Map<String, Object>> getChangedSettings(
+ public abstract ImmutableList<Map<String, Object>> evaluate(
Map<String, Object> previousSettings, StructImpl attributeMap)
throws EvalException, InterruptedException;
@@ -130,7 +131,7 @@
}
@Override
- public ImmutableList<Map<String, Object>> getChangedSettings(
+ public ImmutableList<Map<String, Object>> evaluate(
Map<String, Object> previousSettings, StructImpl attributeMapper) {
return ImmutableList.of(changedSettings);
}
@@ -183,8 +184,25 @@
return false;
}
+ /**
+ * This method evaluates the implementation function of the transition.
+ *
+ * <p>In the case of a {@link
+ * com.google.devtools.build.lib.analysis.config.transitions.PatchTransition}, the impl fxn
+ * returns a {@link SkylarkDict} of option name strings to option value object.
+ *
+ * <p>In the case of {@link
+ * com.google.devtools.build.lib.analysis.config.transitions.SplitTransition}, the impl fxn can
+ * return either a {@link SkylarkDict} of String keys to {@link SkylarkDict} values. Or it can
+ * return a list of {@link SkylarkDict}s in cases where the consumer doesn't care about
+ * differentiating between the splits (i.e. accessing later via {@code ctx.split_attrs}).
+ *
+ * @param previousSettings a map representing the previous build settings
+ * @param attributeMapper a map of attributes
+ */
+ // TODO(bazel-team): integrate dict-of-dicts return type with ctx.split_attr
@Override
- public ImmutableList<Map<String, Object>> getChangedSettings(
+ public ImmutableList<Map<String, Object>> evaluate(
Map<String, Object> previousSettings, StructImpl attributeMapper)
throws EvalException, InterruptedException {
Object result;
@@ -194,43 +212,48 @@
throw new EvalException(impl.getLocation(), e.getMessage());
}
- if (!(result instanceof SkylarkDict<?, ?>)) {
- throw new EvalException(
- impl.getLocation(), "Transition function must return a dictionary.");
- }
-
- // The result is either:
- // 1. a dictionary mapping option name to new option value (for a single transition), or
- // 2. a dictionary of such dictionaries (for a split transition).
- //
- // First try to parse the result as a dictionary of option dictionaries; then try it as an
- // option dictionary.
- SkylarkDict<?, ?> dictOrDictOfDict = (SkylarkDict<?, ?>) result;
-
- try {
- Map<String, SkylarkDict> dictOfDict =
- dictOrDictOfDict.getContents(
- String.class, SkylarkDict.class, "dictionary of option dictionaries");
-
+ if (result instanceof SkylarkDict<?, ?>) {
+ // TODO(bazel-team): integrate keys with ctx.split_attr. Currently ctx.split_attr always
+ // keys on cpu value - we should be able to key on the keys returned here.
+ try {
+ Map<String, SkylarkDict> dictOfDict =
+ ((SkylarkDict<?, ?>) result)
+ .getContents(
+ String.class, SkylarkDict.class, "dictionary of options dictionaries");
+ ImmutableList.Builder<Map<String, Object>> builder = ImmutableList.builder();
+ for (Map.Entry<String, SkylarkDict> entry : dictOfDict.entrySet()) {
+ Map<String, Object> dict =
+ entry.getValue().getContents(String.class, Object.class, "an option dictionary");
+ builder.add(dict);
+ }
+ return builder.build();
+ } catch (EvalException e) {
+ // fall through
+ }
+ try {
+ return ImmutableList.of(
+ ((SkylarkDict<?, ?>) result)
+ .getContents(String.class, Object.class, "dictionary of options"));
+ } catch (EvalException e) {
+ throw new EvalException(impl.getLocation(), e.getMessage());
+ }
+ } else if (result instanceof SkylarkList<?>) {
ImmutableList.Builder<Map<String, Object>> builder = ImmutableList.builder();
- for (Map.Entry<String, SkylarkDict> entry : dictOfDict.entrySet()) {
- Map<String, Object> dict =
- entry.getValue().getContents(String.class, Object.class, "an option dictionary");
- builder.add(dict);
+ try {
+ for (SkylarkDict<?, ?> toOptions :
+ ((SkylarkList<?>) result)
+ .getContents(SkylarkDict.class, "dictionary of options dictionaries")) {
+ builder.add(toOptions.getContents(String.class, Object.class, "dictionary of options"));
+ }
+ } catch (EvalException e) {
+ throw new EvalException(impl.getLocation(), e.getMessage());
}
return builder.build();
- } catch (EvalException e) {
- // Fall through.
+ } else {
+ throw new EvalException(
+ impl.getLocation(),
+ "Transition function must return a dictionary or list of dictionaries.");
}
-
- Map<String, Object> dict;
- try {
- dict = dictOrDictOfDict.getContents(String.class, Object.class, "an option dictionary");
- } catch (EvalException e) {
- throw new EvalException(impl.getLocation(), e.getMessage());
- }
-
- return ImmutableList.of(dict);
}
@Override
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 07f2649..22ced4d 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
@@ -70,22 +70,22 @@
throws EvalException, InterruptedException {
// TODO(waltl): consider building this once and use it across different split
// transitions.
- Map<String, OptionInfo> optionInfoMap = buildOptionInfo(buildOptions);
- SkylarkDict<String, Object> settings =
- buildSettings(buildOptions, optionInfoMap, starlarkTransition);
+ Map<String, OptionInfo> optionInfoMap = buildOptionInfo(buildOptions);
+ SkylarkDict<String, Object> settings =
+ buildSettings(buildOptions, optionInfoMap, starlarkTransition);
- ImmutableList.Builder<BuildOptions> splitBuildOptions = ImmutableList.builder();
+ ImmutableList.Builder<BuildOptions> splitBuildOptions = ImmutableList.builder();
- ImmutableList<Map<String, Object>> transitions =
- starlarkTransition.getChangedSettings(settings, attrObject);
+ ImmutableList<Map<String, Object>> transitions =
+ starlarkTransition.evaluate(settings, attrObject);
validateFunctionOutputsMatchesDeclaredOutputs(transitions, starlarkTransition);
- for (Map<String, Object> transition : transitions) {
- BuildOptions transitionedOptions =
- applyTransition(buildOptions, transition, optionInfoMap, starlarkTransition);
- splitBuildOptions.add(transitionedOptions);
- }
- return splitBuildOptions.build();
+ for (Map<String, Object> transition : transitions) {
+ BuildOptions transitionedOptions =
+ applyTransition(buildOptions, transition, optionInfoMap, starlarkTransition);
+ splitBuildOptions.add(transitionedOptions);
+ }
+ return splitBuildOptions.build();
}
/**
@@ -263,6 +263,10 @@
starlarkTransition.getLocationForErrorReporting(),
"Invalid value type for option '" + optionName + "'");
}
+ } catch (IllegalArgumentException e) {
+ throw new EvalException(
+ starlarkTransition.getLocationForErrorReporting(),
+ "IllegalArgumentError for option '" + optionName + "': " + e.getMessage());
} catch (IllegalAccessException e) {
throw new RuntimeException(
"IllegalAccess for option " + optionName + ": " + e.getMessage());
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
index ce33de9..7965774 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
@@ -70,6 +70,45 @@
"test/skylark/my_rule.bzl",
"load('//myinfo:myinfo.bzl', 'MyInfo')",
"def transition_func(settings, attr):",
+ " return [",
+ " {'//command_line_option:cpu': 'k8'},",
+ " {'//command_line_option:cpu': 'armeabi-v7a'}",
+ " ]",
+ "my_transition = transition(implementation = transition_func, inputs = [],",
+ " outputs = ['//command_line_option:cpu'])",
+ "def impl(ctx): ",
+ " return MyInfo(",
+ " split_attr_deps = ctx.split_attr.deps,",
+ " split_attr_dep = ctx.split_attr.dep,",
+ " k8_deps = ctx.split_attr.deps.get('k8', None),",
+ " attr_deps = ctx.attr.deps,",
+ " attr_dep = ctx.attr.dep)",
+ "my_rule = rule(",
+ " implementation = impl,",
+ " attrs = {",
+ " 'deps': attr.label_list(cfg = my_transition),",
+ " 'dep': attr.label(cfg = my_transition),",
+ " '_whitelist_function_transition': attr.label(",
+ " default = '//tools/whitelists/function_transition_whitelist',",
+ " ),",
+ " })");
+
+ scratch.file(
+ "test/skylark/BUILD",
+ "load('//test/skylark:my_rule.bzl', 'my_rule')",
+ "my_rule(name = 'test', deps = [':main1', ':main2'], dep = ':main1')",
+ "cc_binary(name = 'main1', srcs = ['main1.c'])",
+ "cc_binary(name = 'main2', srcs = ['main2.c'])");
+ }
+
+ private void writeBasicTestFiles_dictOfDict() throws Exception {
+ setSkylarkSemanticsOptions("--experimental_starlark_config_transitions=true");
+ writeWhitelistFile();
+ getAnalysisMock().ccSupport().setupCcToolchainConfigForCpu(mockToolsConfig, "armeabi-v7a");
+ scratch.file(
+ "test/skylark/my_rule.bzl",
+ "load('//myinfo:myinfo.bzl', 'MyInfo')",
+ "def transition_func(settings, attr):",
" return {",
" 't0': {'//command_line_option:cpu': 'k8'},",
" 't1': {'//command_line_option:cpu': 'armeabi-v7a'},",
@@ -102,6 +141,12 @@
}
@Test
+ public void testFunctionSplitTransitionCheckSplitAttrDeps_dictOfDict() throws Exception {
+ writeBasicTestFiles_dictOfDict();
+ testSplitTransitionCheckSplitAttrDeps(getConfiguredTarget("//test/skylark:test"));
+ }
+
+ @Test
public void testFunctionSplitTransitionCheckSplitAttrDeps() throws Exception {
writeBasicTestFiles();
testSplitTransitionCheckSplitAttrDeps(getConfiguredTarget("//test/skylark:test"));
@@ -140,10 +185,10 @@
"test/not_whitelisted/my_rule.bzl",
"load('//myinfo:myinfo.bzl', 'MyInfo')",
"def transition_func(settings, attr):",
- " return {",
- " 't0': {'//command_line_option:cpu': 'k8'},",
- " 't1': {'//command_line_option:cpu': 'armeabi-v7a'},",
- " }",
+ " return [",
+ " {'//command_line_option:cpu': 'k8'},",
+ " {'//command_line_option:cpu': 'armeabi-v7a'}",
+ " ]",
"my_transition = transition(implementation = transition_func, inputs = [],",
" outputs = ['//command_line_option:cpu'])",
"def impl(ctx): ",
@@ -258,11 +303,9 @@
"test/skylark/my_rule.bzl",
"load('//myinfo:myinfo.bzl', 'MyInfo')",
"def transition_func(settings, attr):",
- " transitions = {}",
+ " transitions = []",
" for cpu in settings['//command_line_option:fat_apk_cpu']:",
- " transitions[cpu] = {",
- " '//command_line_option:cpu': cpu,",
- " }",
+ " transitions.append({'//command_line_option:cpu': cpu,})",
" return transitions",
"my_transition = transition(implementation = transition_func, ",
" inputs = ['//command_line_option:fat_apk_cpu'],",
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 fd9bbe0..742a28f 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
@@ -95,6 +95,35 @@
}
@Test
+ public void testBadReturnTypeFromTransition() throws Exception {
+ writeWhitelistFile();
+ scratch.file(
+ "test/transitions.bzl",
+ "def _impl(settings, attr):",
+ " return 'cpu=k8'",
+ "my_transition = transition(implementation = _impl, inputs = [],",
+ " outputs = ['//command_line_option:test_arg'])");
+ 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')");
+
+ reporter.removeHandler(failFastHandler);
+ getConfiguredTarget("//test");
+ assertContainsEvent("Transition function must return a dictionary or list of dictionaries.");
+ }
+
+ @Test
public void testOutputOnlyTransition() throws Exception {
writeWhitelistFile();
scratch.file(
@@ -223,10 +252,10 @@
scratch.file(
"test/transitions.bzl",
"def _impl(settings, attr):",
- " return {",
- " 't0': {'//command_line_option:test_arg': ['split_one']},",
- " 't1': {'//command_line_option:test_arg': ['split_two']},",
- " }",
+ " return [",
+ " {'//command_line_option:test_arg': ['split_one']},",
+ " {'//command_line_option:test_arg': ['split_two']},",
+ " ]",
"my_transition = transition(implementation = _impl, inputs = [],",
" outputs = ['//command_line_option:test_arg'])");
scratch.file(
