From 34b77b7545f74afc378e8db2b18f00fb06b0b287 Mon Sep 17 00:00:00 2001 From: John Cater Date: Thu, 25 Apr 2019 10:40:33 -0400 Subject: [PATCH 1/6] Add incompatible_disallow_rule_execution_platform_constraints_allowed flag. Part of #8136 and #8134. RELNOTES: Adds incompatible_disallow_rule_execution_platform_constraints_allowed, which disallows the use of the "execution_platform_constraints_allowed" attribute when defining new rules. --- .../skylark/SkylarkRuleClassFunctions.java | 9 +++++++-- .../lib/packages/StarlarkSemanticsOptions.java | 16 ++++++++++++++++ .../skylarkbuildapi/SkylarkRuleFunctionsApi.java | 4 +++- .../build/lib/syntax/StarlarkSemantics.java | 6 ++++++ 4 files changed, 32 insertions(+), 3 deletions(-) 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 1c23c1e2ecb24e..8fabc7fd8db88d 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 @@ -286,7 +286,7 @@ public BaseFunction rule( FuncallExpression ast, Environment funcallEnv, StarlarkContext context) - throws EvalException, ConversionException { + throws EvalException { SkylarkUtils.checkLoadingOrWorkspacePhase(funcallEnv, "rule", ast.getLocation()); BazelStarlarkContext bazelContext = (BazelStarlarkContext) context; @@ -398,8 +398,13 @@ public BaseFunction rule( execCompatibleWith.getContents(String.class, "exec_compatile_with"), ast.getLocation())); } - if (executionPlatformConstraintsAllowed) { + + if (funcallEnv.getSemantics().incompatibleDisallowRuleExecutionPlatformConstraintsAllowed()) { builder.executionPlatformConstraintsAllowed(ExecutionPlatformConstraintsAllowed.PER_TARGET); + } else if (executionPlatformConstraintsAllowed) { + builder.executionPlatformConstraintsAllowed(ExecutionPlatformConstraintsAllowed.PER_TARGET); + } else { + builder.executionPlatformConstraintsAllowed(ExecutionPlatformConstraintsAllowed.PER_RULE); } return new SkylarkRuleFunction(builder, type, attributes, ast.getLocation()); diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java index 24a16990fbe2cc..deb1d5f557301f 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java +++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java @@ -329,6 +329,20 @@ public class StarlarkSemanticsOptions extends OptionsBase implements Serializabl + "Use for example `cc_library` instead of `native.cc_library`.") public boolean incompatibleDisallowNativeInBuildFile; + @Option( + name = "incompatible_disallow_rule_execution_platform_constraints_allowed", + defaultValue = "False", + documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, + effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS}, + metadataTags = { + OptionMetadataTag.INCOMPATIBLE_CHANGE, + OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES + }, + help = + "If set to true, disallow the use of the execution_platform_constraints_allowed " + + "attribute on rule().") + public boolean incompatibleDisallowRuleExecutionPlatformConstraintsAllowed; + @Option( name = "incompatible_string_join_requires_strings", defaultValue = "false", @@ -623,6 +637,8 @@ public StarlarkSemantics toSkylarkSemantics() { .incompatibleDisallowOldOctalNotation(incompatibleDisallowOldOctalNotation) .incompatibleDisallowOldStyleArgsAdd(incompatibleDisallowOldStyleArgsAdd) .incompatibleDisallowStructProviderSyntax(incompatibleDisallowStructProviderSyntax) + .incompatibleDisallowRuleExecutionPlatformConstraintsAllowed( + incompatibleDisallowRuleExecutionPlatformConstraintsAllowed) .incompatibleExpandDirectories(incompatibleExpandDirectories) .incompatibleNewActionsApi(incompatibleNewActionsApi) .incompatibleNoAttrLicense(incompatibleNoAttrLicense) diff --git a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleFunctionsApi.java b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleFunctionsApi.java index 247300a00a1b07..acd73aba1037a9 100644 --- a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleFunctionsApi.java +++ b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleFunctionsApi.java @@ -287,7 +287,9 @@ public interface SkylarkRuleFunctionsApi { + "label-list type is added, which must not already exist in " + "attrs. Targets may use this attribute to specify additional " + "constraints on the execution platform beyond those given in the " - + "exec_compatible_with argument to rule()."), + + "exec_compatible_with argument to rule(). " + + "This will be deprecated and removed in the near future, and all rules will " + + "be able to use exec_compatible_with."), @Param( name = "exec_compatible_with", type = SkylarkList.class, diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java index bc025a36be0ba8..0c44063f399f75 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java @@ -164,6 +164,8 @@ public boolean flagValue(FlagIdentifier flagIdentifier) { public abstract boolean incompatibleDisallowStructProviderSyntax(); + public abstract boolean incompatibleDisallowRuleExecutionPlatformConstraintsAllowed(); + public abstract boolean incompatibleExpandDirectories(); public abstract boolean incompatibleNewActionsApi(); @@ -235,6 +237,7 @@ public static Builder builderWithDefaults() { .incompatibleDisallowOldOctalNotation(false) .incompatibleDisallowOldStyleArgsAdd(true) .incompatibleDisallowStructProviderSyntax(false) + .incompatibleDisallowRuleExecutionPlatformConstraintsAllowed(false) .incompatibleExpandDirectories(true) .incompatibleNewActionsApi(false) .incompatibleNoAttrLicense(true) @@ -306,6 +309,9 @@ public abstract static class Builder { public abstract Builder incompatibleDisallowStructProviderSyntax(boolean value); + public abstract Builder incompatibleDisallowRuleExecutionPlatformConstraintsAllowed( + boolean value); + public abstract Builder incompatibleExpandDirectories(boolean value); public abstract Builder incompatibleNewActionsApi(boolean value); From 9e98b456d33131aa7503f181b9e3bb5f486aef90 Mon Sep 17 00:00:00 2001 From: John Cater Date: Thu, 25 Apr 2019 11:29:39 -0400 Subject: [PATCH 2/6] Fix to use a FlagIdentifier and the disable checks. --- .../build/lib/analysis/skylark/SkylarkRuleClassFunctions.java | 4 +--- .../build/lib/skylarkbuildapi/SkylarkRuleFunctionsApi.java | 2 ++ .../google/devtools/build/lib/syntax/StarlarkSemantics.java | 1 + 3 files changed, 4 insertions(+), 3 deletions(-) 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 8fabc7fd8db88d..d465c70c99da42 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 @@ -399,9 +399,7 @@ public BaseFunction rule( ast.getLocation())); } - if (funcallEnv.getSemantics().incompatibleDisallowRuleExecutionPlatformConstraintsAllowed()) { - builder.executionPlatformConstraintsAllowed(ExecutionPlatformConstraintsAllowed.PER_TARGET); - } else if (executionPlatformConstraintsAllowed) { + if (executionPlatformConstraintsAllowed) { builder.executionPlatformConstraintsAllowed(ExecutionPlatformConstraintsAllowed.PER_TARGET); } else { builder.executionPlatformConstraintsAllowed(ExecutionPlatformConstraintsAllowed.PER_RULE); diff --git a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleFunctionsApi.java b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleFunctionsApi.java index acd73aba1037a9..1d7eda2cc87b44 100644 --- a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleFunctionsApi.java +++ b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleFunctionsApi.java @@ -282,6 +282,8 @@ public interface SkylarkRuleFunctionsApi { named = true, positional = false, defaultValue = "False", + disableWithFlag = FlagIdentifier.INCOMPATIBLE_DISALLOW_RULE_EXECUTION_PLATFORM_CONSTRAINTS_ALLOWED, + valueWhenDisabled = "True", doc = "If true, a special attribute named exec_compatible_with of " + "label-list type is added, which must not already exist in " diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java index 0c44063f399f75..b5fe754c3777d3 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java @@ -54,6 +54,7 @@ public enum FlagIdentifier { INCOMPATIBLE_NO_TARGET_OUTPUT_GROUP(StarlarkSemantics::incompatibleNoTargetOutputGroup), INCOMPATIBLE_NO_ATTR_LICENSE(StarlarkSemantics::incompatibleNoAttrLicense), INCOMPATIBLE_OBJC_FRAMEWORK_CLEANUP(StarlarkSemantics::incompatibleObjcFrameworkCleanup), + INCOMPATIBLE_DISALLOW_RULE_EXECUTION_PLATFORM_CONSTRAINTS_ALLOWED(StarlarkSemantics::incompatibleDisallowRuleExecutionPlatformConstraintsAllowed), NONE(null); // Using a Function here makes the enum definitions far cleaner, and, since this is From ff760aac237828329cb71136090a1fe34ac99791 Mon Sep 17 00:00:00 2001 From: John Cater Date: Thu, 25 Apr 2019 11:36:00 -0400 Subject: [PATCH 3/6] Fix SkylarkSemanticsConsistencyTest --- .../build/lib/packages/SkylarkSemanticsConsistencyTest.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java index 7e2df6fe635fc4..7b5c18a4041515 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java @@ -148,6 +148,7 @@ private static StarlarkSemanticsOptions buildRandomOptions(Random rand) throws E "--incompatible_disallow_old_octal_notation=" + rand.nextBoolean(), "--incompatible_disallow_old_style_args_add=" + rand.nextBoolean(), "--incompatible_disallow_struct_provider_syntax=" + rand.nextBoolean(), + "--incompatible_disallow_rule_execution_platform_constraints_allowed=" + rand.nextBoolean(), "--incompatible_expand_directories=" + rand.nextBoolean(), "--incompatible_new_actions_api=" + rand.nextBoolean(), "--incompatible_no_attr_license=" + rand.nextBoolean(), @@ -199,6 +200,7 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) { .incompatibleDisallowOldOctalNotation(rand.nextBoolean()) .incompatibleDisallowOldStyleArgsAdd(rand.nextBoolean()) .incompatibleDisallowStructProviderSyntax(rand.nextBoolean()) + .incompatibleDisallowRuleExecutionPlatformConstraintsAllowed(rand.nextBoolean()) .incompatibleExpandDirectories(rand.nextBoolean()) .incompatibleNewActionsApi(rand.nextBoolean()) .incompatibleNoAttrLicense(rand.nextBoolean()) From dfea49a8dbbad40753f6267461fa295b519db1da Mon Sep 17 00:00:00 2001 From: John Cater Date: Thu, 25 Apr 2019 11:42:41 -0400 Subject: [PATCH 4/6] Line-wrap --- .../google/devtools/build/lib/syntax/StarlarkSemantics.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java index b5fe754c3777d3..5e25c9631905f4 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java @@ -54,7 +54,8 @@ public enum FlagIdentifier { INCOMPATIBLE_NO_TARGET_OUTPUT_GROUP(StarlarkSemantics::incompatibleNoTargetOutputGroup), INCOMPATIBLE_NO_ATTR_LICENSE(StarlarkSemantics::incompatibleNoAttrLicense), INCOMPATIBLE_OBJC_FRAMEWORK_CLEANUP(StarlarkSemantics::incompatibleObjcFrameworkCleanup), - INCOMPATIBLE_DISALLOW_RULE_EXECUTION_PLATFORM_CONSTRAINTS_ALLOWED(StarlarkSemantics::incompatibleDisallowRuleExecutionPlatformConstraintsAllowed), + INCOMPATIBLE_DISALLOW_RULE_EXECUTION_PLATFORM_CONSTRAINTS_ALLOWED( + StarlarkSemantics::incompatibleDisallowRuleExecutionPlatformConstraintsAllowed), NONE(null); // Using a Function here makes the enum definitions far cleaner, and, since this is From ad3d17b71be897b9299b8a6139c33d0b5a68bcaa Mon Sep 17 00:00:00 2001 From: John Cater Date: Thu, 25 Apr 2019 13:14:41 -0400 Subject: [PATCH 5/6] Update the tests --- .../SkylarkRuleClassFunctionsTest.java | 55 ++++++++++++++++++- 1 file changed, 53 insertions(+), 2 deletions(-) diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java index 8c70a043f02853..0e318c798866a3 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java @@ -1739,10 +1739,18 @@ public void testRuleAddExecutionConstraints() throws Exception { } @Test - public void testTargetsCanAddExecutionPlatformConstraints() throws Exception { - registerDummyUserDefinedFunction(); + public void testTargetsCanAddExecutionPlatformConstraints_enabled() throws Exception { + StarlarkSemantics semantics = + StarlarkSemantics.DEFAULT_SEMANTICS.toBuilder() + .incompatibleDisallowRuleExecutionPlatformConstraintsAllowed(false) + .build(); + ev = createEvaluationTestCase(semantics); + ev.initialize(); + scratch.file("test/BUILD", "toolchain_type(name = 'my_toolchain_type')"); evalAndExport( + "def impl():", + " return 0", "r1 = rule(impl, ", " toolchains=['//test:my_toolchain_type'],", " execution_platform_constraints_allowed=True,", @@ -1752,6 +1760,49 @@ public void testTargetsCanAddExecutionPlatformConstraints() throws Exception { .isEqualTo(ExecutionPlatformConstraintsAllowed.PER_TARGET); } + @Test + public void testTargetsCanAddExecutionPlatformConstraints_notEnabled() throws Exception { + StarlarkSemantics semantics = + StarlarkSemantics.DEFAULT_SEMANTICS.toBuilder() + .incompatibleDisallowRuleExecutionPlatformConstraintsAllowed(false) + .build(); + ev = createEvaluationTestCase(semantics); + ev.initialize(); + + scratch.file("test/BUILD", "toolchain_type(name = 'my_toolchain_type')"); + evalAndExport( + "def impl():", + " return 0", + "r1 = rule(impl, ", + " toolchains=['//test:my_toolchain_type'],", + " execution_platform_constraints_allowed=False,", + ")"); + RuleClass c = ((SkylarkRuleFunction) lookup("r1")).getRuleClass(); + assertThat(c.executionPlatformConstraintsAllowed()) + .isEqualTo(ExecutionPlatformConstraintsAllowed.PER_RULE); + } + + @Test + public void testTargetsCanAddExecutionPlatformConstraints_disallowed() throws Exception { + StarlarkSemantics semantics = + StarlarkSemantics.DEFAULT_SEMANTICS.toBuilder() + .incompatibleDisallowRuleExecutionPlatformConstraintsAllowed(true) + .build(); + ev = createEvaluationTestCase(semantics); + ev.setFailFast(false); + ev.initialize(); + + scratch.file("test/BUILD", "toolchain_type(name = 'my_toolchain_type')"); + evalAndExport( + "def impl():", + " return 0", + "r1 = rule(impl, ", + " toolchains=['//test:my_toolchain_type'],", + " execution_platform_constraints_allowed=True,", + ")"); + ev.assertContainsError("parameter 'execution_platform_constraints_allowed' is deprecated"); + } + @Test public void testRuleFunctionReturnsNone() throws Exception { scratch.file("test/rule.bzl", From c5b3b75276eb63a59ad79f202c921251bd9c6b65 Mon Sep 17 00:00:00 2001 From: John Cater Date: Thu, 25 Apr 2019 13:31:10 -0400 Subject: [PATCH 6/6] Fix tests some more --- .../lib/skylark/SkylarkRuleClassFunctionsTest.java | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java index 0e318c798866a3..7bda34c6a6310f 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java @@ -137,7 +137,7 @@ private RuleClass getRuleClass(String name) throws Exception { } private void registerDummyUserDefinedFunction() throws Exception { - eval("def impl():\n" + " return 0\n"); + eval("def impl():", " pass"); } @Test @@ -1747,10 +1747,9 @@ public void testTargetsCanAddExecutionPlatformConstraints_enabled() throws Excep ev = createEvaluationTestCase(semantics); ev.initialize(); + registerDummyUserDefinedFunction(); scratch.file("test/BUILD", "toolchain_type(name = 'my_toolchain_type')"); evalAndExport( - "def impl():", - " return 0", "r1 = rule(impl, ", " toolchains=['//test:my_toolchain_type'],", " execution_platform_constraints_allowed=True,", @@ -1769,10 +1768,9 @@ public void testTargetsCanAddExecutionPlatformConstraints_notEnabled() throws Ex ev = createEvaluationTestCase(semantics); ev.initialize(); + registerDummyUserDefinedFunction(); scratch.file("test/BUILD", "toolchain_type(name = 'my_toolchain_type')"); evalAndExport( - "def impl():", - " return 0", "r1 = rule(impl, ", " toolchains=['//test:my_toolchain_type'],", " execution_platform_constraints_allowed=False,", @@ -1792,10 +1790,9 @@ public void testTargetsCanAddExecutionPlatformConstraints_disallowed() throws Ex ev.setFailFast(false); ev.initialize(); + registerDummyUserDefinedFunction(); scratch.file("test/BUILD", "toolchain_type(name = 'my_toolchain_type')"); evalAndExport( - "def impl():", - " return 0", "r1 = rule(impl, ", " toolchains=['//test:my_toolchain_type'],", " execution_platform_constraints_allowed=True,",