Add incompatible_disallow_rule_execution_platform_constraints_allowed flag. #8145

katre · 2019-04-25T14:50:17Z

RELNOTES: Adds
incompatible_disallow_rule_execution_platform_constraints_allowed, which
disallows the use of the "execution_platform_constraints_allowed"
attribute when defining new rules.

flag. Part of bazelbuild#8136 and bazelbuild#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.

c-parsons · 2019-04-25T15:20:28Z

src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java

@@ -398,8 +398,13 @@ public BaseFunction rule(
              execCompatibleWith.getContents(String.class, "exec_compatile_with"),
              ast.getLocation()));
    }
-    if (executionPlatformConstraintsAllowed) {
+
+    if (funcallEnv.getSemantics().incompatibleDisallowRuleExecutionPlatformConstraintsAllowed()) {


This doesn't look like the fully incompatible change you want -- a user can still specify rule(..., execution_platform_constraints_allowed = False), it just would have no effect.

I'd recommend the following instead of any change in this class:

Introduce a new FlagIdentifier entry in StarlarkSemantics.java for the new flag

In SkylarkRuleFunctionsApi.java, find the @param for execution_platform_constraints_allowed, and set the following fields on it:
A. disableWithFlag = [new flag identifier]
B. valueWhenDisabled = "True"

This should work out-of-the-box (but you should write a specific test for it, just in case :)) Let me know if you run into any issues!

Thanks, this is much cleaner.

Don't forget to add a test~!

c-parsons · 2019-04-25T15:21:11Z

src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java

@@ -623,6 +637,8 @@ public StarlarkSemantics toSkylarkSemantics() {
        .incompatibleDisallowOldOctalNotation(incompatibleDisallowOldOctalNotation)
        .incompatibleDisallowOldStyleArgsAdd(incompatibleDisallowOldStyleArgsAdd)
        .incompatibleDisallowStructProviderSyntax(incompatibleDisallowStructProviderSyntax)
+        .incompatibleDisallowRuleExecutionPlatformConstraintsAllowed(


When introducing new flags, you also need to update SkylarkSemanticsConsistencyTest.java

c-parsons · 2019-04-25T15:38:05Z

src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java

@@ -54,6 +54,7 @@
    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),


wrap to avoid line length limit

Grr my formatter script didn't work, sorry.

c-parsons · 2019-04-25T15:38:43Z

src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java

@@ -398,8 +398,13 @@ public BaseFunction rule(
              execCompatibleWith.getContents(String.class, "exec_compatile_with"),
              ast.getLocation()));
    }
-    if (executionPlatformConstraintsAllowed) {
+
+    if (funcallEnv.getSemantics().incompatibleDisallowRuleExecutionPlatformConstraintsAllowed()) {


Don't forget to add a test~!

katre · 2019-04-25T17:15:12Z

Tests added.

c-parsons · 2019-04-25T17:19:24Z

src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java

+    scratch.file("test/BUILD", "toolchain_type(name = 'my_toolchain_type')");
+    evalAndExport(
+        "def impl():",
+        "  return 0",


Nit: AFAIK creating a target with these rules in these tests would fail with a different error anyway because you can't return an int from a rule implementation. return [] instead or just pass

Same for the other new tests

I was just copying registerDummyUserDefinedFunction, which I have switched back to using and also fixed.

gregestren · 2019-04-25T18:07:12Z

src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java

@@ -164,6 +166,8 @@ public boolean flagValue(FlagIdentifier flagIdentifier) {

  public abstract boolean incompatibleDisallowStructProviderSyntax();

+  public abstract boolean incompatibleDisallowRuleExecutionPlatformConstraintsAllowed();


I know this is nitty, but the docs above mandate listing variables here and in the other places alphabetically, and I know there's some precedent for enforcing that.

Good point, I'll fix that on the imported version.

katre requested review from gregestren and c-parsons April 25, 2019 14:50

katre requested review from brandjon, dkelmer, dslomov, laurentlb and vladmos as code owners April 25, 2019 14:50

googlebot added the cla: yes label Apr 25, 2019

brandjon removed their request for review April 25, 2019 14:53

c-parsons reviewed Apr 25, 2019

View reviewed changes

katre added 2 commits April 25, 2019 11:29

Fix to use a FlagIdentifier and the disable checks.

9e98b45

Fix SkylarkSemanticsConsistencyTest

ff760aa

c-parsons reviewed Apr 25, 2019

View reviewed changes

katre added 2 commits April 25, 2019 11:42

Line-wrap

dfea49a

Update the tests

ad3d17b

c-parsons approved these changes Apr 25, 2019

View reviewed changes

Fix tests some more

c5b3b75

gregestren approved these changes Apr 25, 2019

View reviewed changes

bazel-io closed this in 5688793 Apr 26, 2019

katre deleted the i8136-epca-flag branch April 26, 2019 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add incompatible_disallow_rule_execution_platform_constraints_allowed flag. #8145

Add incompatible_disallow_rule_execution_platform_constraints_allowed flag. #8145

katre commented Apr 25, 2019

c-parsons Apr 25, 2019 •

edited

Loading

katre Apr 25, 2019

c-parsons Apr 25, 2019

c-parsons Apr 25, 2019

katre Apr 25, 2019

c-parsons Apr 25, 2019

katre Apr 25, 2019

c-parsons Apr 25, 2019

katre commented Apr 25, 2019

c-parsons Apr 25, 2019

katre Apr 25, 2019

gregestren Apr 25, 2019

katre Apr 25, 2019

		@@ -164,6 +166,8 @@ public boolean flagValue(FlagIdentifier flagIdentifier) {

		public abstract boolean incompatibleDisallowStructProviderSyntax();

		public abstract boolean incompatibleDisallowRuleExecutionPlatformConstraintsAllowed();

Add incompatible_disallow_rule_execution_platform_constraints_allowed flag. #8145

Add incompatible_disallow_rule_execution_platform_constraints_allowed flag. #8145

Conversation

katre commented Apr 25, 2019

c-parsons Apr 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katre commented Apr 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c-parsons Apr 25, 2019 •

edited

Loading