Simplify split transition semantics: noops now return the input build options.
This results in less special logic in the implementation and a simpler API.
PiperOrigin-RevId: 197772283
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
index dd34f19..a72b346 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
@@ -403,8 +403,15 @@
LateBoundDefault<?, ?> lateBoundDefault = attribute.getLateBoundDefault();
- Collection<BuildOptions> splitOptions = getSplitOptions(attributeMap, attribute, ruleConfig);
- if (!splitOptions.isEmpty() && !ruleConfig.isHostConfiguration()) {
+ boolean hasSplitTransition = false;
+ List<BuildOptions> splitOptions = null;
+ if (attribute.hasSplitConfigurationTransition()) {
+ splitOptions =
+ attribute.getSplitTransition(attributeMap).checkedSplit(ruleConfig.getOptions());
+ hasSplitTransition = !SplitTransition.equals(ruleConfig.getOptions(), splitOptions);
+ }
+
+ if (hasSplitTransition && !ruleConfig.isHostConfiguration()) {
// Late-bound attribute with a split transition:
// Since we want to get the same results as TransitionResolver.evaluateTransition (but
// skip it since we've already applied the split), we want to make sure this logic
@@ -449,24 +456,6 @@
}
/**
- * Returns the BuildOptions if the rule's attribute triggers a split in this configuration, or
- * the empty collection if the attribute does not trigger a split transition or if the split
- * transition does not apply.
- *
- * <p>Even though the attribute may have a split, splits don't have to apply in every
- * configuration (see {@link SplitTransition#split}).
- */
- private static Collection<BuildOptions> getSplitOptions(ConfiguredAttributeMapper attributeMap,
- Attribute attribute,
- BuildConfiguration ruleConfig) {
- if (!attribute.hasSplitConfigurationTransition()) {
- return ImmutableList.<BuildOptions>of();
- }
- SplitTransition transition = attribute.getSplitTransition(attributeMap);
- return transition.split(ruleConfig.getOptions());
- }
-
- /**
* Returns the label dependencies for the given late-bound attribute in this rule.
*
* @param rule the rule being evaluated
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
index adfeb58..fb17814 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -790,10 +790,11 @@
SplitTransition transition =
attributeDefinition.getSplitTransition(
ConfiguredAttributeMapper.of(rule, configConditions));
- List<BuildOptions> splitOptions = transition.split(getConfiguration().getOptions());
+ BuildOptions fromOptions = getConfiguration().getOptions();
+ List<BuildOptions> splitOptions = transition.checkedSplit(fromOptions);
List<ConfiguredTargetAndData> deps = getConfiguredTargetAndTargetDeps(attributeName);
- if (splitOptions.isEmpty()) {
+ if (SplitTransition.equals(fromOptions, splitOptions)) {
// The split transition is not active. Defer the decision on which CPU to use.
return ImmutableMap.of(Optional.<String>absent(), deps);
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
index 677c028..531c6f1 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
@@ -261,7 +261,7 @@
// resolveAspectDependencies don't get a chance to make their own Skyframe requests before
// bailing out of this ConfiguredTargetFunction call. Ideally we could batch all requests
// from all methods into a single Skyframe call, but there are enough subtle data flow
- // dependencies in ConfiguredTargetFucntion to make that impractical.
+ // dependencies in ConfiguredTargetFunction to make that impractical.
Map<SkyKey, ValueOrException<InvalidConfigurationException>> depConfigValues =
env.getValuesOrThrow(keysToEntries.keySet(), InvalidConfigurationException.class);
@@ -429,14 +429,7 @@
// TODO(bazel-team): safety-check that this never mutates fromOptions.
result = ImmutableList.of(((PatchTransition) transition).patch(fromOptions));
} else if (transition instanceof SplitTransition) {
- List<BuildOptions> toOptions = ((SplitTransition) transition).split(fromOptions);
- if (toOptions.isEmpty()) {
- // When the split returns an empty list, it's signaling it doesn't apply to this instance.
- // So return the original options.
- result = ImmutableList.<BuildOptions>of(fromOptions);
- } else {
- result = toOptions;
- }
+ return ((SplitTransition) transition).checkedSplit(fromOptions);
} else {
throw new IllegalStateException(String.format(
"unsupported config transition type: %s", transition.getClass().getName()));
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingSplitTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingSplitTransition.java
index 69970c2..71e3770 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingSplitTransition.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingSplitTransition.java
@@ -120,13 +120,7 @@
if (transition instanceof PatchTransition) {
return ImmutableList.<BuildOptions>of(((PatchTransition) transition).patch(fromOptions));
} else if (transition instanceof SplitTransition) {
- SplitTransition split = (SplitTransition) transition;
- List<BuildOptions> splitOptions = split.split(fromOptions);
- if (splitOptions.isEmpty()) {
- return ImmutableList.<BuildOptions>of(fromOptions);
- } else {
- return splitOptions;
- }
+ return ((SplitTransition) transition).checkedSplit(fromOptions);
} else {
throw new IllegalStateException(
String.format("Unsupported composite transition type: %s",
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/SplitTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/SplitTransition.java
index 05ecfaa..eb71869 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/SplitTransition.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/SplitTransition.java
@@ -14,6 +14,8 @@
package com.google.devtools.build.lib.analysis.config.transitions;
+import com.google.common.base.Verify;
+import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.analysis.config.BuildOptions;
import com.google.devtools.build.lib.concurrent.ThreadSafety;
import java.util.List;
@@ -27,7 +29,29 @@
@FunctionalInterface
public interface SplitTransition extends ConfigurationTransition {
/**
- * Return the list of {@code BuildOptions} after splitting; empty if not applicable.
+ * Returns the list of {@code BuildOptions} after splitting, or the original options if this
+ * split is a noop.
+ *
+ * <p>Returning an empty or null list triggers a {@link RuntimeException}.
*/
List<BuildOptions> split(BuildOptions buildOptions);
+
+ /**
+ * Calls {@link #split} and throws a {@link RuntimeException} if the split output is empty.
+ */
+ default List<BuildOptions> checkedSplit(BuildOptions buildOptions) {
+ List<BuildOptions> splitOptions = split(buildOptions);
+ Verify.verifyNotNull(splitOptions, "Split transition output may not be null");
+ Verify.verify(!splitOptions.isEmpty(), "Split transition output may not be empty");
+ return splitOptions;
+ }
+
+ /**
+ * Returns true iff {@code option} and {@splitOptions} are equal.
+ *
+ * <p>This can be used to determine if a split is a noop.
+ */
+ static boolean equals(BuildOptions options, List<BuildOptions> splitOptions) {
+ return splitOptions.size() == 1 && Iterables.getOnlyElement(splitOptions).equals(options);
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java
index 2035a79..815616d 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidRuleClasses.java
@@ -238,7 +238,7 @@
if (androidOptions.cpu.isEmpty()
|| androidCrosstoolTop == null
|| androidCrosstoolTop.equals(cppOptions.crosstoolTop)) {
- return ImmutableList.of();
+ return ImmutableList.of(buildOptions);
} else {
