Automated rollback of commit 18bd1056b00e1fd58638d4db81e02cfb7df8ce69.
*** Reason for rollback ***
This CL has introduced a bug where using a starlark transition that outputs user-defined build configs can cause a build error. Details are still being investigated. - b/152078818
*** Original change description ***
Allows using split_attr on Starlark split transitions.
RELNOTES: ctx.split_attr now includes attributes with Starlark split transitions.
PiperOrigin-RevId: 302525924
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java
index 9651024..4cdbab6 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java
@@ -14,7 +14,6 @@
package com.google.devtools.build.lib.analysis.skylark;
-import static com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition.PATCH_TRANSITION_KEY;
import static com.google.devtools.build.lib.packages.RuleClass.Builder.SKYLARK_BUILD_SETTING_DEFAULT_ATTR_NAME;
import com.google.common.base.Function;
@@ -427,7 +426,7 @@
ImmutableMap.Builder<String, Object> splitAttrInfos = ImmutableMap.builder();
for (Attribute attr : attributes) {
- if (!attr.getTransitionFactory().isSplit()) {
+ if (!attr.getTransitionFactory().isSplit() || attr.hasStarlarkDefinedTransition()) {
continue;
}
Map<Optional<String>, ? extends List<? extends TransitiveInfoCollection>> splitPrereqs =
@@ -437,13 +436,6 @@
for (Map.Entry<Optional<String>, ? extends List<? extends TransitiveInfoCollection>>
splitPrereq : splitPrereqs.entrySet()) {
- // Skip a split with an empty dependency list.
- // TODO(jungjw): Figure out exactly which cases trigger this and see if this can be made
- // more error-proof.
- if (splitPrereq.getValue().isEmpty()) {
- continue;
- }
-
Object value;
if (attr.getType() == BuildType.LABEL) {
Preconditions.checkState(splitPrereq.getValue().size() == 1);
@@ -453,8 +445,7 @@
value = StarlarkList.immutableCopyOf(splitPrereq.getValue());
}
- if (splitPrereq.getKey().isPresent()
- && !splitPrereq.getKey().get().equals(PATCH_TRANSITION_KEY)) {
+ if (splitPrereq.getKey().isPresent()) {
splitPrereqsMap.put(splitPrereq.getKey().get(), value);
} else {
// If the split transition is not in effect, then the key will be missing since there's
@@ -471,8 +462,9 @@
return StructProvider.STRUCT.create(
splitAttrInfos.build(),
- "No attribute '%s' in split_attr."
- + " This attribute is not defined with a split configuration.");
+ "No attribute '%s' in split_attr. This attribute is either not defined with a split"
+ + " configuration OR is defined with a Starlark split transition, the results of which"
+ + " cannot be accessed from split_attr.");
}
@Override
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 8716954..5e263a7 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
@@ -25,18 +25,16 @@
import com.google.common.eventbus.EventBus;
import com.google.devtools.build.lib.analysis.StarlarkRuleTransitionProviderTest.DummyTestLoader;
import com.google.devtools.build.lib.analysis.config.CoreOptions;
-import com.google.devtools.build.lib.analysis.test.TestConfiguration.TestOptions;
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.packages.Provider;
+import com.google.devtools.build.lib.packages.SkylarkInfo;
import com.google.devtools.build.lib.packages.SkylarkProvider;
import com.google.devtools.build.lib.packages.StructImpl;
import com.google.devtools.build.lib.packages.util.BazelMockAndroidSupport;
-import com.google.devtools.build.lib.syntax.Starlark;
import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
import com.google.devtools.build.lib.util.Fingerprint;
import java.util.List;
-import java.util.Map;
import java.util.stream.Collectors;
import org.junit.Before;
import org.junit.Test;
@@ -117,7 +115,53 @@
}
@Test
- public void testStarlarkSplitTransitionSplitAttr() throws Exception {
+ public void testSplitAttrDoesNotIncludeStarlarkSplit() throws Exception {
+ setSkylarkSemanticsOptions("--experimental_starlark_config_transitions=true");
+ writeWhitelistFile();
+ scratch.file(
+ "test/skylark/rules.bzl",
+ "load('//myinfo:myinfo.bzl', 'MyInfo')",
+ "def transition_func(settings, attr):",
+ " return {",
+ " 'amsterdam': {'//command_line_option:test_arg': ['stroopwafel']},",
+ " 'paris': {'//command_line_option:test_arg': ['crepe']},",
+ " }",
+ "my_transition = transition(",
+ " implementation = transition_func,",
+ " inputs = [],",
+ " outputs = ['//command_line_option:test_arg']",
+ ")",
+ "def _impl(ctx): ",
+ " return MyInfo(split_attr = ctx.split_attr)",
+ "my_rule = rule(",
+ " implementation = _impl,",
+ " attrs = {",
+ " 'dep': attr.label(cfg = my_transition),",
+ " '_whitelist_function_transition': attr.label(",
+ " default = '//tools/whitelists/function_transition_whitelist',",
+ " ),",
+ " }",
+ ")",
+ "def _s_impl_e(ctx):",
+ " return []",
+ "simple_rule = rule(_s_impl_e)");
+
+ scratch.file(
+ "test/skylark/BUILD",
+ "load('//test/skylark:rules.bzl', 'simple_rule', 'my_rule')",
+ "my_rule(name = 'test', dep = ':dep')",
+ "simple_rule(name = 'dep')");
+
+ SkylarkInfo splitAttr =
+ (SkylarkInfo)
+ getMyInfoFromTarget(getConfiguredTarget("//test/skylark:test")).getValue("split_attr");
+
+ assertThat(splitAttr.getValue("dep")).isNull();
+ assertThat(splitAttr.getFieldNames()).isEmpty();
+ }
+
+ @Test
+ public void testAccessStarlarkSplitThrowsError() throws Exception {
setSkylarkSemanticsOptions("--experimental_starlark_config_transitions=true");
writeWhitelistFile();
scratch.file(
@@ -154,125 +198,12 @@
"my_rule(name = 'test', dep = ':dep')",
"simple_rule(name = 'dep')");
- @SuppressWarnings("unchecked")
- Map<Object, ConfiguredTarget> splitAttr =
- (Map<Object, ConfiguredTarget>)
- getMyInfoFromTarget(getConfiguredTarget("//test/skylark:test"))
- .getValue("split_attr_dep");
- assertThat(splitAttr.keySet()).containsExactly("amsterdam", "paris");
- assertThat(
- getConfiguration(splitAttr.get("amsterdam"))
- .getOptions()
- .get(TestOptions.class)
- .testArguments)
- .containsExactly("stroopwafel");
- assertThat(
- getConfiguration(splitAttr.get("paris"))
- .getOptions()
- .get(TestOptions.class)
- .testArguments)
- .containsExactly("crepe");
- }
-
- @Test
- public void testStarlarkListSplitTransitionSplitAttr() throws Exception {
- setSkylarkSemanticsOptions("--experimental_starlark_config_transitions=true");
- writeWhitelistFile();
- scratch.file(
- "test/skylark/rules.bzl",
- "load('//myinfo:myinfo.bzl', 'MyInfo')",
- "def transition_func(settings, attr):",
- " return [",
- " {'//command_line_option:test_arg': ['stroopwafel']},",
- " {'//command_line_option:test_arg': ['crepe']},",
- " ]",
- "my_transition = transition(",
- " implementation = transition_func,",
- " inputs = [],",
- " outputs = ['//command_line_option:test_arg']",
- ")",
- "def _impl(ctx): ",
- " return MyInfo(split_attr_dep = ctx.split_attr.dep)",
- "my_rule = rule(",
- " implementation = _impl,",
- " attrs = {",
- " 'dep': attr.label(cfg = my_transition),",
- " '_whitelist_function_transition': attr.label(",
- " default = '//tools/whitelists/function_transition_whitelist',",
- " ),",
- " }",
- ")",
- "def _s_impl_e(ctx):",
- " return []",
- "simple_rule = rule(_s_impl_e)");
-
- scratch.file(
- "test/skylark/BUILD",
- "load('//test/skylark:rules.bzl', 'simple_rule', 'my_rule')",
- "my_rule(name = 'test', dep = ':dep')",
- "simple_rule(name = 'dep')");
-
- @SuppressWarnings("unchecked")
- Map<Object, ConfiguredTarget> splitAttr =
- (Map<Object, ConfiguredTarget>)
- getMyInfoFromTarget(getConfiguredTarget("//test/skylark:test"))
- .getValue("split_attr_dep");
- assertThat(splitAttr.keySet()).containsExactly("0", "1");
- assertThat(
- getConfiguration(splitAttr.get("0")).getOptions().get(TestOptions.class).testArguments)
- .containsExactly("stroopwafel");
- assertThat(
- getConfiguration(splitAttr.get("1")).getOptions().get(TestOptions.class).testArguments)
- .containsExactly("crepe");
- }
-
- @Test
- public void testStarlarkPatchTransitionSplitAttr() throws Exception {
- setSkylarkSemanticsOptions("--experimental_starlark_config_transitions=true");
- writeWhitelistFile();
- scratch.file(
- "test/skylark/rules.bzl",
- "load('//myinfo:myinfo.bzl', 'MyInfo')",
- "def transition_func(settings, attr):",
- " return {'//command_line_option:test_arg': ['stroopwafel']}",
- "my_transition = transition(",
- " implementation = transition_func,",
- " inputs = [],",
- " outputs = ['//command_line_option:test_arg']",
- ")",
- "def _impl(ctx): ",
- " return MyInfo(split_attr_dep = ctx.split_attr.dep)",
- "my_rule = rule(",
- " implementation = _impl,",
- " attrs = {",
- " 'dep': attr.label(cfg = my_transition),",
- " '_whitelist_function_transition': attr.label(",
- " default = '//tools/whitelists/function_transition_whitelist',",
- " ),",
- " }",
- ")",
- "def _s_impl_e(ctx):",
- " return []",
- "simple_rule = rule(_s_impl_e)");
-
- scratch.file(
- "test/skylark/BUILD",
- "load('//test/skylark:rules.bzl', 'simple_rule', 'my_rule')",
- "my_rule(name = 'test', dep = ':dep')",
- "simple_rule(name = 'dep')");
-
- @SuppressWarnings("unchecked")
- Map<Object, ConfiguredTarget> splitAttr =
- (Map<Object, ConfiguredTarget>)
- getMyInfoFromTarget(getConfiguredTarget("//test/skylark:test"))
- .getValue("split_attr_dep");
- assertThat(splitAttr.keySet()).containsExactly(Starlark.NONE);
- assertThat(
- getConfiguration(splitAttr.get(Starlark.NONE))
- .getOptions()
- .get(TestOptions.class)
- .testArguments)
- .containsExactly("stroopwafel");
+ reporter.removeHandler(failFastHandler);
+ getConfiguredTarget("//test/skylark:test");
+ assertContainsEvent(
+ "No attribute 'dep' in split_attr. This attribute is either not defined with a split"
+ + " configuration OR is defined with a Starlark split transition, the results of which"
+ + " cannot be accessed from split_attr.");
}
@Test
