Skip to content

Commit

Permalink
Clean up the confusing distinction between "BaseRule" and "RuleBase".
Browse files Browse the repository at this point in the history
Summary:
 - RootRule -> <removed>. The most basic rule definition, just adds the
     "name" attribute. Moved directly to RuleClass.Builder.
 - BaseRule -> NativeBuildRule = all non-Starlark BUILD rules. Includes rules
     that create actions (like compilation) and all other purposes (like aliases,
     filegroups, toolchain definitions that only provide metadata to other rules)
 - RuleBase -> NativeActionCreatingRule = all non-Starlark BUILD rules that
     create actions

Starlark rule classes are defined in StarlarkRuleClassFunctions. These remain unchanged.

This is a best-effort attempt. There seem to be some exceptions to every case,
but this covers the overarching themes as clearly as I can see them (code search
for "RuleBase.class)" and "BaseRule.class)").

Even if not 100% accurate, this communicates a standard to aspire to and make
adjustments around. This alone is a good step up from the current wording.

** See BaseRuleClasses.java for the main change. **

PiperOrigin-RevId: 353944214
  • Loading branch information
gregestren authored and copybara-github committed Jan 26, 2021
1 parent 1165182 commit 9868455
Show file tree
Hide file tree
Showing 69 changed files with 131 additions and 134 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.analysis.config.HostTransition;
import com.google.devtools.build.lib.analysis.config.RunUnder;
Expand Down Expand Up @@ -248,7 +247,7 @@ public Metadata getMetadata() {
return RuleDefinition.Metadata.builder()
.name("$test_base_rule")
.type(RuleClassType.ABSTRACT)
.ancestors(RootRule.class, MakeVariableExpandingRule.class)
.ancestors(MakeVariableExpandingRule.class)
.build();
}
}
Expand All @@ -275,6 +274,9 @@ public static synchronized ImmutableList<Label> getTestRuntimeLabelList(
public static final String TAGGED_TRIMMING_ATTR = "transitive_configs";

/** Share common attributes across both base and Starlark base rules. */
// TODO(bazel-team): replace this with a common RuleDefinition ancestor of NativeBuildRule
// and StarlarkRuleClassFunctions.baseRule. This requires refactoring StarlarkRuleClassFunctions
// to instantiate its RuleClasses through RuleDefinition.
public static RuleClass.Builder commonCoreAndStarlarkAttributes(RuleClass.Builder builder) {
return builder
// The visibility attribute is special: it is a nodep label, and loading the
Expand Down Expand Up @@ -349,41 +351,24 @@ public static RuleClass.Builder commonCoreAndStarlarkAttributes(RuleClass.Builde
.nonconfigurable("applicable_licenses is not configurable"));
}

public static RuleClass.Builder nameAttribute(RuleClass.Builder builder) {
return builder.add(attr("name", STRING).nonconfigurable("Rule name"));
}

public static RuleClass.Builder execPropertiesAttribute(RuleClass.Builder builder)
throws ConversionException {
return builder.add(
attr(RuleClass.EXEC_PROPERTIES, STRING_DICT).defaultValue(ImmutableMap.of()));
}

/**
* Ancestor of every rule.
* Ancestor of every native rule in BUILD files (not WORKSPACE files).
*
* <p>Adds the name attribute to every rule.
*/
public static final class RootRule implements RuleDefinition {

@Override
public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment environment) {
return nameAttribute(builder).build();
}

@Override
public Metadata getMetadata() {
return RuleDefinition.Metadata.builder()
.name("$root_rule")
.type(RuleClassType.ABSTRACT)
.build();
}
}

/**
* Common parts of some rules.
* <p>This includes:
*
* <ul>
* <li>rules that create actions ({@link NativeActionCreatingRule})
* <li>rules that encapsulate toolchain and build environment context
* <li>rules that aggregate other rules (like file groups, test suites, or aliases)
* </ul>
*/
public static final class BaseRule implements RuleDefinition {
public static final class NativeBuildRule implements RuleDefinition {
@Override
public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) {
return commonCoreAndStarlarkAttributes(builder)
Expand All @@ -399,9 +384,8 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
@Override
public Metadata getMetadata() {
return RuleDefinition.Metadata.builder()
.name("$base_rule")
.name("$native_build_rule")
.type(RuleClassType.ABSTRACT)
.ancestors(RootRule.class)
.build();
}
}
Expand Down Expand Up @@ -433,9 +417,13 @@ public Metadata getMetadata() {
}

/**
* Common ancestor class for some rules.
* Ancestor of every native BUILD rule that creates actions.
*
* <p>This is a subset of all BUILD rules. Filegroups and aliases, for example, simply encapsulate
* other rules. Toolchain rules provide metadata for actions of other rules. See {@link
* NativeBuildRule} for these.
*/
public static final class RuleBase implements RuleDefinition {
public static final class NativeActionCreatingRule implements RuleDefinition {
@Override
public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) {
return builder
Expand All @@ -461,16 +449,13 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
@Override
public Metadata getMetadata() {
return RuleDefinition.Metadata.builder()
.name("$rule")
.name("$native_buildable_rule")
.type(RuleClassType.ABSTRACT)
.ancestors(BaseRule.class)
.ancestors(BaseRuleClasses.NativeBuildRule.class)
.build();
}
}

public static final ImmutableSet<String> ALLOWED_RULE_CLASSES =
ImmutableSet.of("filegroup", "genrule", "Fileset");

/** A base rule for all binary rules. */
public static final class BinaryBaseRule implements RuleDefinition {
@Override
Expand All @@ -491,7 +476,7 @@ public Metadata getMetadata() {
return RuleDefinition.Metadata.builder()
.name("$binary_base_rule")
.type(RuleClassType.ABSTRACT)
.ancestors(RootRule.class, MakeVariableExpandingRule.class)
.ancestors(MakeVariableExpandingRule.class)
.build();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
public Metadata getMetadata() {
return RuleDefinition.Metadata.builder()
.name(ConstraintConstants.ENVIRONMENT_RULE)
.ancestors(BaseRuleClasses.BaseRule.class)
.ancestors(BaseRuleClasses.NativeBuildRule.class)
.factoryClass(Environment.class)
.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,7 @@ public Label load(String from) throws Exception {
/** Parent rule class for non-executable non-test Starlark rules. */
public static final RuleClass baseRule =
BaseRuleClasses.commonCoreAndStarlarkAttributes(
BaseRuleClasses.nameAttribute(
new RuleClass.Builder("$base_rule", RuleClassType.ABSTRACT, true))
new RuleClass.Builder("$base_rule", RuleClassType.ABSTRACT, true)
.add(attr("expect_failure", STRING)))
// TODO(skylark-team): Allow Starlark rules to extend native rules and remove duplication.
.add(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ public StarlarkCallable repositoryRule(
BaseRuleClasses.execPropertiesAttribute(builder);
}
builder.addAttribute(attr("$environ", STRING_LIST).defaultValue(environ).build());
BaseRuleClasses.nameAttribute(builder);
BaseRuleClasses.commonCoreAndStarlarkAttributes(builder);
builder.add(attr("expect_failure", STRING));
if (attrs != Starlark.NONE) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi
public Metadata getMetadata() {
return RuleDefinition.Metadata.builder()
.name("android_sdk")
.ancestors(AndroidSdkBaseRule.class, BaseRuleClasses.BaseRule.class)
.ancestors(AndroidSdkBaseRule.class, BaseRuleClasses.NativeBuildRule.class)
.factoryClass(BazelAndroidSdk.class)
.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
public Metadata getMetadata() {
return RuleDefinition.Metadata.builder()
.name("filegroup")
.ancestors(BaseRuleClasses.BaseRule.class)
.ancestors(BaseRuleClasses.NativeBuildRule.class)
.factoryClass(Filegroup.class)
.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
public Metadata getMetadata() {
return RuleDefinition.Metadata.builder()
.name("cc_import")
.ancestors(BaseRuleClasses.BaseRule.class, CcImportRule.class)
.ancestors(BaseRuleClasses.NativeBuildRule.class, CcImportRule.class)
.factoryClass(BazelCcImport.class)
.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ public Metadata getMetadata() {
return RuleDefinition.Metadata.builder()
.name("$cc_decl_rule")
.type(RuleClassType.ABSTRACT)
.ancestors(BaseRuleClasses.RuleBase.class)
.ancestors(BaseRuleClasses.NativeActionCreatingRule.class)
.build();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ public Metadata getMetadata() {
return RuleDefinition.Metadata.builder()
.name("$java_rule")
.type(RuleClassType.ABSTRACT)
.ancestors(BaseRuleClasses.RuleBase.class, JavaBaseRule.class)
.ancestors(BaseRuleClasses.NativeActionCreatingRule.class, JavaBaseRule.class)
.build();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public Metadata getMetadata() {
return RuleDefinition.Metadata.builder()
.name("java_lite_proto_library")
.factoryClass(JavaLiteProtoLibrary.class)
.ancestors(BaseRuleClasses.RuleBase.class, JavaToolchainBaseRule.class)
.ancestors(BaseRuleClasses.NativeActionCreatingRule.class, JavaToolchainBaseRule.class)
.build();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public Metadata getMetadata() {
return RuleDefinition.Metadata.builder()
.name("java_proto_library")
.factoryClass(JavaProtoLibrary.class)
.ancestors(BaseRuleClasses.RuleBase.class)
.ancestors(BaseRuleClasses.NativeActionCreatingRule.class)
.build();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ public Metadata getMetadata() {
return RuleDefinition.Metadata.builder()
.name("ninja_build")
.type(RuleClassType.NORMAL)
.ancestors(BaseRuleClasses.BaseRule.class)
.ancestors(BaseRuleClasses.NativeBuildRule.class)
.factoryClass(NinjaBuild.class)
.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ public Metadata getMetadata() {
return RuleDefinition.Metadata.builder()
.name("ninja_graph")
.type(RuleClassType.NORMAL)
.ancestors(BaseRuleClasses.BaseRule.class)
.ancestors(BaseRuleClasses.NativeBuildRule.class)
.factoryClass(NinjaGraph.class)
.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ public Metadata getMetadata() {
return RuleDefinition.Metadata.builder()
.name("$base_py")
.type(RuleClassType.ABSTRACT)
.ancestors(BaseRuleClasses.RuleBase.class)
.ancestors(BaseRuleClasses.NativeActionCreatingRule.class)
.build();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ public Metadata getMetadata() {
return RuleDefinition.Metadata.builder()
.name("$sh_target")
.type(RuleClassType.ABSTRACT)
.ancestors(BaseRuleClasses.RuleBase.class)
.ancestors(BaseRuleClasses.NativeActionCreatingRule.class)
.build();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
import static com.google.devtools.build.lib.packages.ExecGroup.COPY_FROM_RULE_EXEC_GROUP;
import static com.google.devtools.build.lib.packages.Type.BOOLEAN;
import static com.google.devtools.build.lib.packages.Type.STRING;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Function;
Expand Down Expand Up @@ -749,6 +750,9 @@ public Builder(String name, RuleClassType type, boolean starlark, RuleClass... p
this.type = type;
Preconditions.checkState(starlark || type != RuleClassType.PLACEHOLDER, name);
this.documented = type != RuleClassType.ABSTRACT;
add(
attr("name", STRING)
.nonconfigurable("All rules have a non-customizable \"name\" attribute"));
for (RuleClass parent : parents) {
if (parent.getValidityPredicate() != PredicatesWithMessage.<Rule>alwaysTrue()) {
setValidityPredicate(parent.getValidityPredicate());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ public Metadata getMetadata() {
return Metadata.builder()
.name(RULE_NAME)
.factoryClass(Alias.class)
.ancestors(BaseRuleClasses.BaseRule.class)
.ancestors(BaseRuleClasses.NativeBuildRule.class)
.build();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi
public Metadata getMetadata() {
return Metadata.builder()
.name(ruleName)
.ancestors(BaseRuleClasses.BaseRule.class)
.ancestors(BaseRuleClasses.NativeBuildRule.class)
.factoryClass(LateBoundAlias.class)
.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public Metadata getMetadata() {
return Metadata.builder()
.name("toolchain_type")
.factoryClass(ToolchainType.class)
.ancestors(BaseRuleClasses.BaseRule.class)
.ancestors(BaseRuleClasses.NativeBuildRule.class)
.build();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ a real device (In particular controlling its UserAgent strings and other
public Metadata getMetadata() {
return RuleDefinition.Metadata.builder()
.name("android_device")
.ancestors(BaseRuleClasses.RuleBase.class)
.ancestors(BaseRuleClasses.NativeActionCreatingRule.class)
.factoryClass(factoryClass)
.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi
public Metadata getMetadata() {
return RuleDefinition.Metadata.builder()
.name("android_device_script_fixture")
.ancestors(BaseRuleClasses.RuleBase.class)
.ancestors(BaseRuleClasses.NativeActionCreatingRule.class)
.factoryClass(factoryClass)
.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
public Metadata getMetadata() {
return RuleDefinition.Metadata.builder()
.name("android_host_service_fixture")
.ancestors(BaseRuleClasses.RuleBase.class)
.ancestors(BaseRuleClasses.NativeActionCreatingRule.class)
.factoryClass(factoryClass)
.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,7 @@ public Metadata getMetadata() {
return RuleDefinition.Metadata.builder()
.name("$android_base")
.type(RuleClassType.ABSTRACT)
.ancestors(BaseRuleClasses.RuleBase.class)
.ancestors(BaseRuleClasses.NativeActionCreatingRule.class)
.build();
}
}
Expand Down Expand Up @@ -949,7 +949,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi
public Metadata getMetadata() {
return Metadata.builder()
.name("android_tools_defaults_jar")
.ancestors(BaseRuleClasses.BaseRule.class)
.ancestors(BaseRuleClasses.NativeBuildRule.class)
.factoryClass(factoryClass)
.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
public Metadata getMetadata() {
return RuleDefinition.Metadata.builder()
.name("available_xcodes")
.ancestors(BaseRuleClasses.BaseRule.class)
.ancestors(BaseRuleClasses.NativeBuildRule.class)
.factoryClass(AvailableXcodes.class)
.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi
public Metadata getMetadata() {
return Metadata.builder()
.name("xcode_config_alias")
.ancestors(BaseRuleClasses.BaseRule.class, AppleToolchain.RequiresXcodeConfigRule.class)
.ancestors(
BaseRuleClasses.NativeBuildRule.class, AppleToolchain.RequiresXcodeConfigRule.class)
.factoryClass(XcodeConfigAlias.class)
.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
public Metadata getMetadata() {
return RuleDefinition.Metadata.builder()
.name("xcode_config")
.ancestors(BaseRuleClasses.BaseRule.class)
.ancestors(BaseRuleClasses.NativeBuildRule.class)
.factoryClass(XcodeConfig.class)
.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
public Metadata getMetadata() {
return RuleDefinition.Metadata.builder()
.name("xcode_version")
.ancestors(BaseRuleClasses.BaseRule.class)
.ancestors(BaseRuleClasses.NativeBuildRule.class)
.factoryClass(XcodeVersion.class)
.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi
public Metadata getMetadata() {
return RuleDefinition.Metadata.builder()
.name("apple_cc_toolchain")
.ancestors(BaseRuleClasses.BaseRule.class, CcToolchainRule.class,
.ancestors(
BaseRuleClasses.NativeBuildRule.class,
CcToolchainRule.class,
AppleToolchain.RequiresXcodeConfigRule.class)
.factoryClass(AppleCcToolchain.class)
.build();
Expand Down
Loading

0 comments on commit 9868455

Please sign in to comment.