Skip to content

Commit

Permalink
Allows using split_attr on Starlark split transitions. This is a modi…
Browse files Browse the repository at this point in the history 
…fied clone of 18bd105, which was rolled back due to b/152078818, with a new test case specifically designed for the bug.

RELNOTES: ctx.split_attr now includes attributes with Starlark split transitions.
PiperOrigin-RevId: 309445624
  • Loading branch information
@copybara-github
Googler authored and copybara-github committed May 1, 2020
1 parent f7bb602 commit 3862f87
Show file tree
Hide file tree
Showing 2 changed files with 154 additions and 23 deletions.
18 changes: 13 additions & 5 deletions src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

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.STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME;

import com.google.common.base.Function;
Expand Down Expand Up @@ -428,7 +429,7 @@ private static StructImpl buildSplitAttributeInfo(

ImmutableMap.Builder<String, Object> splitAttrInfos = ImmutableMap.builder();
for (Attribute attr : attributes) {
if (!attr.getTransitionFactory().isSplit() || attr.hasStarlarkDefinedTransition()) {
if (!attr.getTransitionFactory().isSplit()) {
continue;
}
Map<Optional<String>, ? extends List<? extends TransitiveInfoCollection>> splitPrereqs =
Expand All @@ -438,6 +439,13 @@ private static StructImpl buildSplitAttributeInfo(
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);
Expand All @@ -447,7 +455,8 @@ private static StructImpl buildSplitAttributeInfo(
value = StarlarkList.immutableCopyOf(splitPrereq.getValue());
}

if (splitPrereq.getKey().isPresent()) {
if (splitPrereq.getKey().isPresent()
&& !splitPrereq.getKey().get().equals(PATCH_TRANSITION_KEY)) {
splitPrereqsMap.put(splitPrereq.getKey().get(), value);
} else {
// If the split transition is not in effect, then the key will be missing since there's
Expand All @@ -464,9 +473,8 @@ private static StructImpl buildSplitAttributeInfo(

return StructProvider.STRUCT.create(
splitAttrInfos.build(),
"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.");
"No attribute '%s' in split_attr."
+ " This attribute is not defined with a split configuration.");
}

@Override
Expand Down
159 changes: 141 additions & 18 deletions src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,18 @@
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.StarlarkProvider;
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;
Expand Down Expand Up @@ -115,7 +117,7 @@ private void writeBasicTestFiles() throws Exception {
}

@Test
public void testSplitAttrDoesNotIncludeStarlarkSplit() throws Exception {
public void testStarlarkSplitTransitionSplitAttr() throws Exception {
setStarlarkSemanticsOptions("--experimental_starlark_config_transitions=true");
writeWhitelistFile();
scratch.file(
Expand All @@ -132,7 +134,7 @@ public void testSplitAttrDoesNotIncludeStarlarkSplit() throws Exception {
" outputs = ['//command_line_option:test_arg']",
")",
"def _impl(ctx): ",
" return MyInfo(split_attr = ctx.split_attr)",
" return MyInfo(split_attr_dep = ctx.split_attr.dep)",
"my_rule = rule(",
" implementation = _impl,",
" attrs = {",
Expand All @@ -152,26 +154,87 @@ public void testSplitAttrDoesNotIncludeStarlarkSplit() throws Exception {
"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();
@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 testAccessStarlarkSplitThrowsError() throws Exception {
public void testStarlarkListSplitTransitionSplitAttr() throws Exception {
setStarlarkSemanticsOptions("--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']},",
" 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 {
setStarlarkSemanticsOptions("--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 = [],",
Expand All @@ -198,12 +261,72 @@ public void testAccessStarlarkSplitThrowsError() throws Exception {
"my_rule(name = 'test', dep = ':dep')",
"simple_rule(name = 'dep')");

reporter.removeHandler(failFastHandler);
@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");
}

@Test
public void testStarlarkConfigSplitAttr() throws Exception {
// This is a customized test case for b/152078818, where a starlark transition that takes a
// starlark config as input caused a failure when no custom values were provided for the config.
setStarlarkSemanticsOptions("--experimental_starlark_config_transitions=true");
writeWhitelistFile();
scratch.file(
"test/skylark/rules.bzl",
"load('//myinfo:myinfo.bzl', 'MyInfo')",
"def _build_setting_impl(ctx):",
" return []",
"string_flag = rule(",
" implementation = _build_setting_impl,",
" build_setting = config.string(flag=True)",
")",
"def transition_func(settings, attr):",
" return {'amsterdam': {'//command_line_option:test_arg': ['stroopwafel']}}",
"my_transition = transition(",
" implementation = transition_func,",
" inputs = ['//test/skylark:custom_arg'],",
" 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', 'string_flag')",
"string_flag(name='custom_arg', build_setting_default='ski')",
"my_rule(name = 'test', dep = ':dep')",
"simple_rule(name = 'dep')");

// Run the analysis phase with the default options, i.e. no custom flags first.
getConfiguredTarget("//test/skylark:test");

// b/152078818 was unique in that an error was hidden until the next run due to how event replay
// was done. Test it by supplying a value to the starlark config, which should trigger the
// analysis phase again.
useConfiguration(ImmutableMap.of("//test/skylark:custom_arg", "snowboard"));
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
Expand Down

0 comments on commit 3862f87

Please sign in to comment.