Allow Skylark rules to specify whether targets can add execution plat… #5341

katre · 2018-06-07T15:43:29Z

…form constraints.

Change-Id: Ic937b011d1d95aac7dc6d715fa418f54d89198a9

…form constraints. Change-Id: Ic937b011d1d95aac7dc6d715fa418f54d89198a9

brandjon · 2018-06-07T19:39:30Z

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

@@ -397,6 +400,14 @@ public BaseFunction rule(
      builder.advertiseSkylarkProvider(skylarkProvider);
    }

+    // TODO(katre): convert from bool to enum
+    if (!execCompatibleWith.isEmpty()) {
+      builder.addExecutionPlatformConstraints(collectConstraintLabels(execCompatibleWith, ast));


Ideally, builtin functions shouldn't depend on the AST of their call site in any way (it's kind of a wart that breaks encapsulation). This function already violates that, but as a tiny step to not making it worse, please replace ast with ast.getLocation() in the signature of collectConstraintLabels().

I'll send another PR shortly making these same changes for toolchains.

brandjon · 2018-06-07T19:42:42Z

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

@@ -316,6 +317,8 @@ public BaseFunction rule(
      SkylarkList<String> toolchains,
      String doc,
      SkylarkList<?> providesArg,
+      Boolean executionPlatformConstraintsAllowed,
+      SkylarkList<String> execCompatibleWith,


The generic type is unchecked by the Skylark calling interface. Change this to SkylarkList<?>, and use SkylarkList#getContents to cast to a list of strings.

brandjon · 2018-06-07T19:43:37Z

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

@@ -397,6 +400,14 @@ public BaseFunction rule(
      builder.advertiseSkylarkProvider(skylarkProvider);
    }

+    // TODO(katre): convert from bool to enum


Outdated comment?

brandjon · 2018-06-07T20:33:45Z

src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleFunctionsApi.java

@@ -283,6 +288,8 @@ public BaseFunction rule(
      SkylarkList<String> toolchains,
      String doc,
      SkylarkList<?> providesArg,
+      Boolean executionPlatformConstraintsAllowed,
+      SkylarkList<String> execCompatibleWith,


<String> to <?> as mentioned earlier. (Yes, I see you use generic1 = String.class, but IIRC that affects documentation generation but isn't actually enforced. Regardless, let's be consistent with the other string list params here and worry about fixing that in the future.)

brandjon · 2018-06-07T20:38:24Z

src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleFunctionsApi.java

+            positional = false,
+            defaultValue = "False",
+            doc =
+                "Whether targets of this rule can set exec_compatible_with to add extra constraints on the execution platform."),


Suggested rephrasing:

If true, a special attribute named <code>exec_compatible_with</code> of label-list type is added. Targets may use this attribute to specify additional constraints on the execution platform beyond those given in the <code>exec_compatible_with</code> argument to <code>rule()</code>.

Is this wording accurate? Specifically, is exec_compatible_with considered a real label-list attribute, or is it some kind of string-list with special processing? If it's really a label list, the user would expect ctx.attr.exec_compatible_with to return a Target object.

Yes: it's a list of labels, which must exist.
No: They can't be resolved as Targets, because execution constraints have to be checked before dependencies are resolved (because the selected toolchains are dependencies, and we need the execution constraints in order to select the proper toolchain).

The user can probably access ctx.attr.exec_compatible_with, I'm just not sure if there's ever a use for that.

With or without a use for ctx.attr.exec_compatible_with, if it exists it should be documented, and the easiest way to do that is to refer to it as an "attribute" here.

If setting this bool causes it to be illegal for the user to define their own "exec_compatible_with" attribute in the attrs dict, we should also say so here, e.g.:

If true, a special attribute named <code>exec_compatible_with</code> of label-list type is added, which must not already exist in `attrs`. ...

(backticks to <code> and </code>)

brandjon · 2018-06-07T20:39:08Z

src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleFunctionsApi.java

+            positional = false,
+            defaultValue = "[]",
+            doc =
+                "A list of constraints on the execution platform that targets of this rule will use.")


Suggested rephrasing:

A list of constraints on the execution platform that apply to all targets of this rule type.

brandjon · 2018-06-07T20:55:42Z

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

+  public void testRuleAddExecutionConstraints() throws Exception {
+    scratch.file("test/BUILD", "toolchain_type(name = 'my_toolchain_type')");
+    evalAndExport(
+        "def impl(ctx): return None",


Nit: pass is more idiomatic than return None

brandjon · 2018-06-07T20:56:45Z

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

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


Nit: use implementation=impl (on a new line) instead of a positional arg.

brandjon · 2018-06-07T21:03:33Z

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

+  }
+
+  @Test
+  public void testTargetsCanAddExecutionPlatformConstraints() throws Exception {


Add a new test to illustrate what happens when a user defines the exec_compatible_with attribute themselves (using attrs = ...) -- whatever that behavior is supposed to be.

I have added such a tes, but it doesn't seem to work, as this is only checked when RuleClass.Builder.build() is called, and for Skylark rules that happens when the rule is exported, and I can't figure out how to write a test that exports a rule. Any pointers?

That's understandable. Check out SkylarkImportLookupFunction#execAndExport, and SkylarkRuleClassFunctionsTest#evalAndExport.

Thanks, that worked!

Change-Id: If9886663c578297d8c7bba5355735ffb2354f39e

Change-Id: I4f19dddc8ccbaf10853b5b842e2597f39fe0911d

Change-Id: I7fb0ed49df7bab74912740571813ef332e729ee9

brandjon · 2018-06-11T15:59:06Z

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

-      this.ruleClass = builder.build(ruleClassName, skylarkLabel + "%" + ruleClassName);
+      try {
+        this.ruleClass = builder.build(ruleClassName, skylarkLabel + "%" + ruleClassName);
+      } catch (IllegalArgumentException | IllegalStateException ex) {


I'm not too familiar with the rule class builder, but I'm a little wary when I see broad unchecked exception types reraised as a checked exception like this. Can you add a comment mentioning that it's in case there's a conflict in the attribute builder?

Change-Id: Ib74e59fec48102469a5039e045e3f3d0e0d86d8c

…form constraints. Closes bazelbuild#5341. Change-Id: Ib74e59fec48102469a5039e045e3f3d0e0d86d8c PiperOrigin-RevId: 200526448

Allow Skylark rules to specify whether targets can add execution plat…

abbd64a

…form constraints. Change-Id: Ic937b011d1d95aac7dc6d715fa418f54d89198a9

katre requested review from brandjon and dslomov June 7, 2018 15:43

googlebot added the cla: yes label Jun 7, 2018

brandjon requested changes Jun 7, 2018

View reviewed changes

Review fixes.

f878f3a

Change-Id: If9886663c578297d8c7bba5355735ffb2354f39e

laszlocsomor assigned katre Jun 11, 2018

katre added 2 commits June 11, 2018 10:14

Fixed test for bad use of exec_compatible_with

4944aef

Change-Id: I4f19dddc8ccbaf10853b5b842e2597f39fe0911d

Documentation fixes

14a798b

Change-Id: I7fb0ed49df7bab74912740571813ef332e729ee9

brandjon approved these changes Jun 11, 2018

View reviewed changes

Added comment

4b4d4d2

Change-Id: Ib74e59fec48102469a5039e045e3f3d0e0d86d8c

bazel-io closed this in d79e977 Jun 14, 2018

katre deleted the per-target-constraints-skylark branch June 20, 2018 17:14

werkt pushed a commit to werkt/bazel that referenced this pull request Aug 2, 2018

Allow Skylark rules to specify whether targets can add execution plat…

cc12e10

…form constraints. Closes bazelbuild#5341. Change-Id: Ib74e59fec48102469a5039e045e3f3d0e0d86d8c PiperOrigin-RevId: 200526448

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Skylark rules to specify whether targets can add execution plat… #5341

Allow Skylark rules to specify whether targets can add execution plat… #5341

katre commented Jun 7, 2018

brandjon Jun 7, 2018

katre Jun 11, 2018

brandjon Jun 7, 2018

brandjon Jun 7, 2018

brandjon Jun 7, 2018

brandjon Jun 7, 2018

katre Jun 11, 2018

brandjon Jun 11, 2018

brandjon Jun 11, 2018

brandjon Jun 7, 2018

brandjon Jun 7, 2018

brandjon Jun 7, 2018

brandjon Jun 7, 2018

katre Jun 11, 2018

brandjon Jun 11, 2018

katre Jun 11, 2018

brandjon Jun 11, 2018

Allow Skylark rules to specify whether targets can add execution plat… #5341

Allow Skylark rules to specify whether targets can add execution plat… #5341

Conversation

katre commented Jun 7, 2018

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

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

Choose a reason for hiding this comment