Skip to content
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

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -398,8 +398,11 @@ public BaseFunction rule(
execCompatibleWith.getContents(String.class, "exec_compatile_with"),
ast.getLocation()));
}

if (executionPlatformConstraintsAllowed) {
builder.executionPlatformConstraintsAllowed(ExecutionPlatformConstraintsAllowed.PER_TARGET);
} else {
builder.executionPlatformConstraintsAllowed(ExecutionPlatformConstraintsAllowed.PER_RULE);
}

return new SkylarkRuleFunction(builder, type, attributes, ast.getLocation());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -623,6 +637,8 @@ public StarlarkSemantics toSkylarkSemantics() {
.incompatibleDisallowOldOctalNotation(incompatibleDisallowOldOctalNotation)
.incompatibleDisallowOldStyleArgsAdd(incompatibleDisallowOldStyleArgsAdd)
.incompatibleDisallowStructProviderSyntax(incompatibleDisallowStructProviderSyntax)
.incompatibleDisallowRuleExecutionPlatformConstraintsAllowed(
Copy link
Contributor

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

incompatibleDisallowRuleExecutionPlatformConstraintsAllowed)
.incompatibleExpandDirectories(incompatibleExpandDirectories)
.incompatibleNewActionsApi(incompatibleNewActionsApi)
.incompatibleNoAttrLicense(incompatibleNoAttrLicense)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -282,12 +282,16 @@ public interface SkylarkRuleFunctionsApi<FileApiT extends FileApi> {
named = true,
positional = false,
defaultValue = "False",
disableWithFlag = FlagIdentifier.INCOMPATIBLE_DISALLOW_RULE_EXECUTION_PLATFORM_CONSTRAINTS_ALLOWED,
valueWhenDisabled = "True",
doc =
"If true, a special attribute named <code>exec_compatible_with</code> of "
+ "label-list type is added, which must not already exist in "
+ "<code>attrs</code>. 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>."),
+ "<code>exec_compatible_with</code> argument to <code>rule()</code>. "
+ "This will be deprecated and removed in the near future, and all rules will "
+ "be able to use <code>exec_compatible_with</code>."),
@Param(
name = "exec_compatible_with",
type = SkylarkList.class,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -164,6 +166,8 @@ public boolean flagValue(FlagIdentifier flagIdentifier) {

public abstract boolean incompatibleDisallowStructProviderSyntax();

public abstract boolean incompatibleDisallowRuleExecutionPlatformConstraintsAllowed();
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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();
Expand Down Expand Up @@ -235,6 +239,7 @@ public static Builder builderWithDefaults() {
.incompatibleDisallowOldOctalNotation(false)
.incompatibleDisallowOldStyleArgsAdd(true)
.incompatibleDisallowStructProviderSyntax(false)
.incompatibleDisallowRuleExecutionPlatformConstraintsAllowed(false)
.incompatibleExpandDirectories(true)
.incompatibleNewActionsApi(false)
.incompatibleNoAttrLicense(true)
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,",
Expand All @@ -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",
Copy link
Contributor

Choose a reason for hiding this comment

The 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. return [] instead or just pass

Same for the other new tests

Copy link
Member Author

Choose a reason for hiding this comment

The 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",
Expand Down