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.

Sign up for GitHub

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

Jump to bottom

Add incompatible_disallow_rule_execution_platform_constraints_allowed flag. #8145

Closed
wants to merge 6 commits into from
Closed

Add incompatible_disallow_rule_execution_platform_constraints_allowed flag. #8145

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
34b77b7
Add incompatible_disallow_rule_execution_platform_constraints_allowed
katre Apr 25, 2019
9e98b45
Fix to use a FlagIdentifier and the disable checks.
katre Apr 25, 2019
ff760aa
Fix SkylarkSemanticsConsistencyTest
katre Apr 25, 2019
dfea49a
Line-wrap
katre Apr 25, 2019
ad3d17b
Update the tests
katre Apr 25, 2019
c5b3b75
Fix tests some more
katre Apr 25, 2019
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
9 changes: 7 additions & 2 deletions src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java
View file
Open in desktop
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,13 @@ public BaseFunction rule(
execCompatibleWith.getContents(String.class, "exec_compatile_with"),
ast.getLocation()));
}
if (executionPlatformConstraintsAllowed) {

if (funcallEnv.getSemantics().incompatibleDisallowRuleExecutionPlatformConstraintsAllowed()) {
Copy link
Contributor

@c-parsons c-parsons Apr 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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:

  1. Introduce a new FlagIdentifier entry in StarlarkSemantics.java for the new flag
  2. 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!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, this is much cleaner.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget to add a test~!

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());
Expand Down
16 changes: 16 additions & 0 deletions src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java
View file
Open in desktop
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
4 changes: 3 additions & 1 deletion src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleFunctionsApi.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,9 @@ public interface SkylarkRuleFunctionsApi<FileApiT extends FileApi> {
+ "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
6 changes: 6 additions & 0 deletions src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,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 +237,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 +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);
Expand Down