-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add incompatible_disallow_rule_execution_platform_constraints_allowed flag. #8145
Changes from 5 commits
34b77b7
9e98b45
ff760aa
dfea49a
ad3d17b
c5b3b75
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,6 +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), | ||
NONE(null); | ||
|
||
// Using a Function here makes the enum definitions far cleaner, and, since this is | ||
|
@@ -164,6 +166,8 @@ public boolean flagValue(FlagIdentifier flagIdentifier) { | |
|
||
public abstract boolean incompatibleDisallowStructProviderSyntax(); | ||
|
||
public abstract boolean incompatibleDisallowRuleExecutionPlatformConstraintsAllowed(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, I'll fix that on the imported version. |
||
|
||
public abstract boolean incompatibleExpandDirectories(); | ||
|
||
public abstract boolean incompatibleNewActionsApi(); | ||
|
@@ -235,6 +239,7 @@ public static Builder builderWithDefaults() { | |
.incompatibleDisallowOldOctalNotation(false) | ||
.incompatibleDisallowOldStyleArgsAdd(true) | ||
.incompatibleDisallowStructProviderSyntax(false) | ||
.incompatibleDisallowRuleExecutionPlatformConstraintsAllowed(false) | ||
.incompatibleExpandDirectories(true) | ||
.incompatibleNewActionsApi(false) | ||
.incompatibleNoAttrLicense(true) | ||
|
@@ -306,6 +311,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); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Same for the other new tests There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was just copying registerDummyUserDefinedFunction, which I have switched back to using and also fixed. |
||
"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", | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When introducing new flags, you also need to update SkylarkSemanticsConsistencyTest.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.