Whitelist Starlark rule class transitions in order to guard roll out.
We already whitelist attribute transitions so a decent amount of testing is done in StarlarkAttrTransitionProviderTest.
Actually checking of the whitelist hinges on having the special whitelist attribute (with proper name and value). So just adding checks that that attribute is properly around when we have rule class transitions hooks in the whitelist checking (see SkylarkRuleConfiguredTargetUtil#buildRule for context).
PiperOrigin-RevId: 238052200
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java
index 24e1241..0c358e2 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java
@@ -374,6 +374,7 @@
StarlarkDefinedConfigTransition starlarkDefinedConfigTransition =
(StarlarkDefinedConfigTransition) cfg;
builder.cfg(new StarlarkRuleTransitionProvider(starlarkDefinedConfigTransition));
+ builder.setHasStarlarkRuleTransition();
}
for (Object o : providesArg) {
@@ -668,7 +669,9 @@
throw new EvalException(definitionLocation, "Invalid rule class name '" + ruleClassName
+ "', test rule class names must end with '_test' and other rule classes must not");
}
- boolean hasStarlarkDefinedTransition = false;
+ // Thus far, we only know if we have a rule transition. While iterating through attributes,
+ // check if we have an attribute transition.
+ boolean hasStarlarkDefinedTransition = builder.hasStarlarkRuleTransition();
boolean hasFunctionTransitionWhitelist = false;
for (Pair<String, SkylarkAttr.Descriptor> attribute : attributes) {
String name = attribute.getFirst();
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
index 3905d0a..c19694a 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
@@ -676,6 +676,7 @@
private boolean isAnalysisTest = false;
private boolean hasAnalysisTestTransition = false;
private boolean hasFunctionTransitionWhitelist = false;
+ private boolean hasStarlarkRuleTransition = false;
private boolean ignorePackageLicenses = false;
private ImplicitOutputsFunction implicitOutputsFunction = ImplicitOutputsFunction.NONE;
private RuleTransitionFactory transitionFactory;
@@ -1074,6 +1075,14 @@
return this;
}
+ public void setHasStarlarkRuleTransition() {
+ hasStarlarkRuleTransition = true;
+ }
+
+ public boolean hasStarlarkRuleTransition() {
+ return hasStarlarkRuleTransition;
+ }
+
public Builder factory(ConfiguredTargetFactory<?, ?, ?> factory) {
this.configuredTargetFactory = factory;
return this;
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 ee38a4f..29d18b7 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
@@ -43,8 +43,20 @@
@RunWith(JUnit4.class)
public class StarlarkRuleTransitionProviderTest extends BuildViewTestCase {
+ private void writeWhitelistFile() throws Exception {
+ scratch.file(
+ "tools/whitelists/function_transition_whitelist/BUILD",
+ "package_group(",
+ " name = 'function_transition_whitelist',",
+ " packages = [",
+ " '//test/...',",
+ " ],",
+ ")");
+ }
+
@Test
public void testOutputOnlyTransition() throws Exception {
+ writeWhitelistFile();
scratch.file(
"test/transitions.bzl",
"def _impl(settings, attr):",
@@ -56,7 +68,14 @@
"load('//test:transitions.bzl', 'my_transition')",
"def _impl(ctx):",
" return []",
- "my_rule = rule(implementation = _impl, cfg = my_transition)");
+ "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("--test_arg=pre-transition");
@@ -68,6 +87,7 @@
@Test
public void testInputAndOutputTransition() throws Exception {
+ writeWhitelistFile();
scratch.file(
"test/transitions.bzl",
"def _impl(settings, attr):",
@@ -84,7 +104,14 @@
"load('//test:transitions.bzl', 'my_transition')",
"def _impl(ctx):",
" return []",
- "my_rule = rule(implementation = _impl, cfg = my_transition)");
+ "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')");
@@ -98,6 +125,7 @@
@Test
public void testBuildSettingCannotTransition() throws Exception {
setSkylarkSemanticsOptions("--experimental_build_setting_api=true");
+ writeWhitelistFile();
scratch.file(
"test/transitions.bzl",
"def _impl(settings, attr):",
@@ -112,8 +140,12 @@
"my_rule = rule(",
" implementation = _impl,",
" cfg = my_transition,",
- " build_setting = config.string()",
- ")");
+ " build_setting = config.string(),",
+ " 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);
@@ -124,6 +156,7 @@
@Test
public void testBadCfgInput() throws Exception {
+ writeWhitelistFile();
scratch.file(
"test/rules.bzl",
"def _impl(ctx):",
@@ -131,7 +164,11 @@
"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);
@@ -142,6 +179,7 @@
@Test
public void testMultipleReturnConfigs() throws Exception {
+ writeWhitelistFile();
scratch.file(
"test/transitions.bzl",
"def _impl(settings, attr):",
@@ -156,7 +194,14 @@
"load('//test:transitions.bzl', 'my_transition')",
"def _impl(ctx):",
" return []",
- "my_rule = rule(implementation = _impl, cfg = my_transition)");
+ "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')");
AssertionError error = assertThrows(AssertionError.class, () -> getConfiguredTarget("//test"));
@@ -166,6 +211,7 @@
@Test
public void testCanDoBadStuffWithParameterizedTransitionsAndSelects() throws Exception {
+ writeWhitelistFile();
scratch.file(
"test/transitions.bzl",
"def _impl(settings, attr):",
@@ -183,7 +229,12 @@
"my_rule = rule(",
" implementation = _impl,",
" cfg = my_transition,",
- " attrs = {'my_configurable_attr': attr.bool(default = False)},",
+ " attrs = {",
+ " 'my_configurable_attr': attr.bool(default = False),",
+ " '_whitelist_function_transition': attr.label(",
+ " default = '//tools/whitelists/function_transition_whitelist',",
+ " ),",
+ " },",
")");
scratch.file(
"test/BUILD",
@@ -210,6 +261,7 @@
@Test
public void testLabelTypedAttrReturnsLabelNotDep() throws Exception {
+ writeWhitelistFile();
scratch.file(
"test/transitions.bzl",
"def _impl(settings, attr):",
@@ -227,7 +279,12 @@
"my_rule = rule(",
" implementation = _impl,",
" cfg = my_transition,",
- " attrs = {'dict_attr': attr.label_keyed_string_dict()},",
+ " attrs = {",
+ " 'dict_attr': attr.label_keyed_string_dict(),",
+ " '_whitelist_function_transition': attr.label(",
+ " default = '//tools/whitelists/function_transition_whitelist',",
+ " ),",
+ " },",
")",
"simple_rule = rule(_impl)");
scratch.file(
@@ -246,6 +303,40 @@
.containsExactly("post-transition");
}
+ private void writeRulesBuildSettingsAndBUILDforBuildSettingTransitionTests() throws Exception {
+ writeWhitelistFile();
+ scratch.file(
+ "test/rules.bzl",
+ "load('//test:transitions.bzl', 'my_transition')",
+ "def _rule_impl(ctx):",
+ " return []",
+ "my_rule = rule(",
+ " implementation = _rule_impl,",
+ " cfg = my_transition,",
+ " attrs = {",
+ " '_whitelist_function_transition': attr.label(",
+ " default = '//tools/whitelists/function_transition_whitelist',",
+ " ),",
+ " },",
+ ")");
+
+ scratch.file(
+ "test/build_settings.bzl",
+ "def _impl(ctx):",
+ " return []",
+ "string_flag = rule(implementation = _impl, build_setting = config.string(flag=True))");
+
+ scratch.file(
+ "test/BUILD",
+ "load('//test:rules.bzl', 'my_rule')",
+ "load('//test:build_settings.bzl', 'string_flag')",
+ "my_rule(name = 'test')",
+ "string_flag(",
+ " name = 'cute-animal-fact',",
+ " build_setting_default = 'cows produce more milk when they listen to soothing music',",
+ ")");
+ }
+
@Test
public void testCannotTransitionOnBuildSettingWithoutFlag() throws Exception {
setSkylarkSemanticsOptions(
@@ -266,34 +357,6 @@
assertContainsEvent("transitions on Starlark-defined build settings is experimental");
}
- private void writeRulesBuildSettingsAndBUILDforBuildSettingTransitionTests() throws Exception {
- scratch.file(
- "test/rules.bzl",
- "load('//test:transitions.bzl', 'my_transition')",
- "def _rule_impl(ctx):",
- " return []",
- "my_rule = rule(",
- " implementation = _rule_impl,",
- " cfg = my_transition,",
- ")");
-
- scratch.file(
- "test/build_settings.bzl",
- "def _impl(ctx):",
- " return []",
- "string_flag = rule(implementation = _impl, build_setting = config.string(flag=True))");
-
- scratch.file(
- "test/BUILD",
- "load('//test:rules.bzl', 'my_rule')",
- "load('//test:build_settings.bzl', 'string_flag')",
- "my_rule(name = 'test')",
- "string_flag(",
- " name = 'cute-animal-fact',",
- " build_setting_default = 'cows produce more milk when they listen to soothing music',",
- ")");
- }
-
@Test
public void testTransitionOnBuildSetting() throws Exception {
setSkylarkSemanticsOptions(
@@ -398,6 +461,7 @@
@Test
public void testOneParamTransitionFunctionApiFails() throws Exception {
setSkylarkSemanticsOptions("--experimental_starlark_config_transitions=true");
+ writeWhitelistFile();
scratch.file(
"test/transitions.bzl",
"def _impl(settings):",
@@ -409,7 +473,15 @@
"load('//test:transitions.bzl', 'my_transition')",
"def _impl(ctx):",
" return []",
- "my_rule = rule(implementation = _impl, cfg = my_transition)");
+ "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')");
AssertionError error = assertThrows(AssertionError.class, () -> getConfiguredTarget("//test"));
@@ -419,6 +491,7 @@
@Test
public void testCannotTransitionOnExperimentalFlag() throws Exception {
+ writeWhitelistFile();
scratch.file(
"test/transitions.bzl",
"def _impl(settings, attr):",
@@ -430,11 +503,51 @@
"load('//test:transitions.bzl', 'my_transition')",
"def _impl(ctx):",
" return []",
- "my_rule = rule(implementation = _impl, cfg = my_transition)");
+ "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("Cannot transition on --experimental_* or --incompatible_* options");
}
+
+ @Test
+ public void testCannotTransitionWithoutWhitelist() throws Exception {
+ scratch.file(
+ "tools/whitelists/function_transition_whitelist/BUILD",
+ "package_group(",
+ " name = 'function_transition_whitelist',",
+ " packages = [],",
+ ")");
+ scratch.file(
+ "test/transitions.bzl",
+ "def _impl(settings, attr):",
+ " return {'//command_line_option:test_arg': ['post-transition']}",
+ "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,",
+ ")");
+ scratch.file("test/BUILD", "load('//test:rules.bzl', 'my_rule')", "my_rule(name = 'test')");
+
+ useConfiguration("--test_arg=pre-transition");
+
+ reporter.removeHandler(failFastHandler);
+ getConfiguredTarget("//test");
+ assertContainsEvent("Use of function-based split transition without whitelist");
+ }
}
