From 7b599453389b3a00eebf25c58cde322a0e7bdf02 Mon Sep 17 00:00:00 2001 From: Dmitry Lomov Date: Thu, 26 Nov 2015 10:07:32 +0000 Subject: Refactor Skylark rules and attributes in preparation to Skylark aspects. 1. attr. functions return a wrapper object instead of Attribute.Builder dierctly. 2. RuleClass is created once per the life-time of RuleFunction, during export 3. Attributes are added to the RuleClass at exporting. -- MOS_MIGRATED_REVID=108774581 --- .../devtools/build/lib/rules/SkylarkAttr.java | 67 +++++++++++------- .../build/lib/rules/SkylarkRuleClassFunctions.java | 80 ++++++++++++++-------- .../lib/skyframe/SkylarkImportLookupFunction.java | 7 +- 3 files changed, 98 insertions(+), 56 deletions(-) (limited to 'src/main/java/com') diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java index 5d8775378e..f445eb6699 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java +++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java @@ -184,11 +184,11 @@ public final class SkylarkAttr { return builder; } - private static Attribute.Builder createAttribute( + private static Descriptor createAttribute( Map kwargs, Type type, FuncallExpression ast, Environment env) throws EvalException { try { - return createAttribute(type, kwargs, ast, env); + return new Descriptor(createAttribute(type, kwargs, ast, env)); } catch (ConversionException e) { throw new EvalException(ast.getLocation(), e.getMessage()); } @@ -198,7 +198,7 @@ public final class SkylarkAttr { name = "int", doc = "Creates an attribute of type int.", objectType = SkylarkAttr.class, - returnType = Attribute.Builder.class, + returnType = Descriptor.class, optionalNamedOnly = { @Param( name = DEFAULT_ARG, @@ -221,7 +221,7 @@ public final class SkylarkAttr { ) private static BuiltinFunction integer = new BuiltinFunction("int") { - public Attribute.Builder invoke( + public Descriptor invoke( Integer defaultInt, Boolean mandatory, SkylarkList values, @@ -243,7 +243,7 @@ public final class SkylarkAttr { name = "string", doc = "Creates an attribute of type string.", objectType = SkylarkAttr.class, - returnType = Attribute.Builder.class, + returnType = Descriptor.class, optionalNamedOnly = { @Param( name = DEFAULT_ARG, @@ -266,7 +266,7 @@ public final class SkylarkAttr { ) private static BuiltinFunction string = new BuiltinFunction("string") { - public Attribute.Builder invoke( + public Descriptor invoke( String defaultString, Boolean mandatory, SkylarkList values, @@ -291,7 +291,7 @@ public final class SkylarkAttr { + "If you need a dependency that the user cannot overwrite, make the attribute " + "private (starts with _).", objectType = SkylarkAttr.class, - returnType = Attribute.Builder.class, + returnType = Descriptor.class, optionalNamedOnly = { @Param( name = DEFAULT_ARG, @@ -349,7 +349,7 @@ public final class SkylarkAttr { ) private static BuiltinFunction label = new BuiltinFunction("label") { - public Attribute.Builder invoke( + public Descriptor invoke( Object defaultO, Boolean executable, Object allowFiles, @@ -390,7 +390,7 @@ public final class SkylarkAttr { name = "string_list", doc = "Creates an attribute of type list of strings", objectType = SkylarkAttr.class, - returnType = Attribute.Builder.class, + returnType = Descriptor.class, optionalPositionals = { @Param( name = DEFAULT_ARG, @@ -409,7 +409,7 @@ public final class SkylarkAttr { ) private static BuiltinFunction stringList = new BuiltinFunction("string_list") { - public Attribute.Builder invoke( + public Descriptor invoke( SkylarkList defaultList, Boolean mandatory, Boolean nonEmpty, @@ -430,7 +430,7 @@ public final class SkylarkAttr { name = "int_list", doc = "Creates an attribute of type list of ints", objectType = SkylarkAttr.class, - returnType = Attribute.Builder.class, + returnType = Descriptor.class, optionalPositionals = { @Param( name = DEFAULT_ARG, @@ -449,7 +449,7 @@ public final class SkylarkAttr { ) private static BuiltinFunction intList = new BuiltinFunction("int_list") { - public Attribute.Builder invoke( + public Descriptor invoke( SkylarkList defaultList, Boolean mandatory, Boolean nonEmpty, @@ -472,7 +472,7 @@ public final class SkylarkAttr { "Creates an attribute of type list of labels. " + "See label for more information.", objectType = SkylarkAttr.class, - returnType = Attribute.Builder.class, + returnType = Descriptor.class, optionalNamedOnly = { @Param( name = DEFAULT_ARG, @@ -529,7 +529,7 @@ public final class SkylarkAttr { ) private static BuiltinFunction labelList = new BuiltinFunction("label_list") { - public Attribute.Builder invoke( + public Descriptor invoke( Object defaultList, Object allowFiles, Object allowRules, @@ -570,7 +570,7 @@ public final class SkylarkAttr { name = "bool", doc = "Creates an attribute of type bool. Its default value is False.", objectType = SkylarkAttr.class, - returnType = Attribute.Builder.class, + returnType = Descriptor.class, optionalNamedOnly = { @Param(name = DEFAULT_ARG, type = Boolean.class, defaultValue = "False", doc = DEFAULT_DOC), @Param(name = MANDATORY_ARG, type = Boolean.class, defaultValue = "False", doc = MANDATORY_DOC @@ -581,7 +581,7 @@ public final class SkylarkAttr { ) private static BuiltinFunction bool = new BuiltinFunction("bool") { - public Attribute.Builder invoke( + public Descriptor invoke( Boolean defaultO, Boolean mandatory, FuncallExpression ast, Environment env) throws EvalException { env.checkLoadingPhase("attr.bool", ast.getLocation()); @@ -600,7 +600,7 @@ public final class SkylarkAttr { + "The user provides a file name (string) and the rule must create an action that " + "generates the file.", objectType = SkylarkAttr.class, - returnType = Attribute.Builder.class, + returnType = Descriptor.class, optionalNamedOnly = { @Param( name = DEFAULT_ARG, @@ -617,7 +617,7 @@ public final class SkylarkAttr { ) private static BuiltinFunction output = new BuiltinFunction("output") { - public Attribute.Builder invoke( + public Descriptor invoke( Object defaultO, Boolean mandatory, FuncallExpression ast, Environment env) throws EvalException { env.checkLoadingPhase("attr.output", ast.getLocation()); @@ -635,7 +635,7 @@ public final class SkylarkAttr { "Creates an attribute of type list of outputs. Its default value is []. " + "See output above for more information.", objectType = SkylarkAttr.class, - returnType = Attribute.Builder.class, + returnType = Descriptor.class, optionalNamedOnly = { @Param( name = DEFAULT_ARG, @@ -654,7 +654,7 @@ public final class SkylarkAttr { ) private static BuiltinFunction outputList = new BuiltinFunction("output_list") { - public Attribute.Builder invoke( + public Descriptor invoke( SkylarkList defaultList, Boolean mandatory, Boolean nonEmpty, @@ -677,7 +677,7 @@ public final class SkylarkAttr { "Creates an attribute of type dictionary, mapping from string to string. " + "Its default value is dict().", objectType = SkylarkAttr.class, - returnType = Attribute.Builder.class, + returnType = Descriptor.class, optionalNamedOnly = { @Param(name = DEFAULT_ARG, type = Map.class, defaultValue = "{}", doc = DEFAULT_DOC), @Param(name = MANDATORY_ARG, type = Boolean.class, defaultValue = "False", doc = MANDATORY_DOC @@ -690,7 +690,7 @@ public final class SkylarkAttr { ) private static BuiltinFunction stringDict = new BuiltinFunction("string_dict") { - public Attribute.Builder invoke( + public Descriptor invoke( Map defaultO, Boolean mandatory, Boolean nonEmpty, @@ -713,7 +713,7 @@ public final class SkylarkAttr { "Creates an attribute of type dictionary, mapping from string to list of string. " + "Its default value is dict().", objectType = SkylarkAttr.class, - returnType = Attribute.Builder.class, + returnType = Descriptor.class, optionalNamedOnly = { @Param(name = DEFAULT_ARG, type = Map.class, defaultValue = "{}", doc = DEFAULT_DOC), @Param(name = MANDATORY_ARG, type = Boolean.class, defaultValue = "False", doc = MANDATORY_DOC @@ -726,7 +726,7 @@ public final class SkylarkAttr { ) private static BuiltinFunction stringListDict = new BuiltinFunction("string_list_dict") { - public Attribute.Builder invoke( + public Descriptor invoke( Map defaultO, Boolean mandatory, Boolean nonEmpty, @@ -748,7 +748,7 @@ public final class SkylarkAttr { doc = "Creates an attribute of type license. Its default value is NO_LICENSE.", // TODO(bazel-team): Implement proper license support for Skylark. objectType = SkylarkAttr.class, - returnType = Attribute.Builder.class, + returnType = Descriptor.class, optionalNamedOnly = { // TODO(bazel-team): ensure this is the correct default value @Param(name = DEFAULT_ARG, defaultValue = "None", noneable = true, doc = DEFAULT_DOC), @@ -760,7 +760,7 @@ public final class SkylarkAttr { ) private static BuiltinFunction license = new BuiltinFunction("license") { - public Attribute.Builder invoke( + public Descriptor invoke( Object defaultO, Boolean mandatory, FuncallExpression ast, Environment env) throws EvalException { env.checkLoadingPhase("attr.license", ast.getLocation()); @@ -772,6 +772,21 @@ public final class SkylarkAttr { } }; + /** + * A descriptor of an attribute defined in Skylark. + */ + public static final class Descriptor { + private final Attribute.Builder attributeBuilder; + + public Descriptor(Attribute.Builder attributeBuilder) { + this.attributeBuilder = attributeBuilder; + } + + public Attribute.Builder getAttributeBuilder() { + return attributeBuilder; + } + } + static { SkylarkSignatureProcessor.configureSkylarkFunctions(SkylarkAttr.class); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java index ed5892486a..63b6307525 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java @@ -84,6 +84,7 @@ import com.google.devtools.build.lib.syntax.SkylarkSignatureProcessor; import com.google.devtools.build.lib.syntax.SkylarkValue; import com.google.devtools.build.lib.syntax.Type; import com.google.devtools.build.lib.syntax.Type.ConversionException; +import com.google.devtools.build.lib.util.Pair; import java.util.HashMap; import java.util.Map; @@ -264,18 +265,22 @@ public class SkylarkRuleClassFunctions { // We'll set the name later, pass the empty string for now. RuleClass.Builder builder = new RuleClass.Builder("", type, true, parent); + ImmutableList.Builder> attributes = + ImmutableList.builder(); if (attrs != Runtime.NONE) { - for (Map.Entry attr : - castMap(attrs, String.class, Attribute.Builder.class, "attrs").entrySet()) { - Attribute.Builder attrBuilder = (Attribute.Builder) attr.getValue(); + for (Map.Entry attr : + castMap(attrs, String.class, SkylarkAttr.Descriptor.class, "attrs").entrySet()) { + SkylarkAttr.Descriptor attrDescriptor = attr.getValue(); String attrName = - attributeToNative(attr.getKey(), ast.getLocation(), attrBuilder.hasLateBoundValue()); - addAttribute(builder, attrBuilder.build(attrName)); + attributeToNative(attr.getKey(), ast.getLocation(), + attrDescriptor.getAttributeBuilder().hasLateBoundValue()); + attributes.add(Pair.of(attrName, attrDescriptor)); } } if (executable || test) { addAttribute( + ast.getLocation(), builder, attr("$is_executable", BOOLEAN) .value(true) @@ -307,16 +312,9 @@ public class SkylarkRuleClassFunctions { registerRequiredFragments(fragments, hostFragments, builder); builder.setConfiguredTargetFunction(implementation); builder.setRuleDefinitionEnvironment(funcallEnv); - return new RuleFunction(builder, type); + return new RuleFunction(builder, type, attributes.build(), ast.getLocation()); } - private void addAttribute(RuleClass.Builder builder, Attribute attribute) throws EvalException { - try { - builder.addOrOverrideAttribute(attribute); - } catch (IllegalArgumentException ex) { - throw new EvalException(location, ex); - } - } private void registerRequiredFragments( SkylarkList fragments, SkylarkList hostFragments, RuleClass.Builder builder) @@ -336,6 +334,16 @@ public class SkylarkRuleClassFunctions { } }; + private static void addAttribute( + Location location, RuleClass.Builder builder, Attribute attribute) throws EvalException { + try { + builder.addOrOverrideAttribute(attribute); + } catch (IllegalArgumentException ex) { + throw new EvalException(location, ex); + } + } + + @SkylarkSignature( name = "aspect", returnType = SkylarkAspect.class, @@ -367,17 +375,22 @@ public class SkylarkRuleClassFunctions { /** The implementation for the magic function "rule" that creates Skylark rule classes */ public static final class RuleFunction extends BaseFunction { - // Note that this means that we can reuse the same builder. - // This is fine since we don't modify the builder from here. - private final RuleClass.Builder builder; + private RuleClass.Builder builder; + + private RuleClass ruleClass; private final RuleClassType type; + private ImmutableList> attributes; + private final Location definitionLocation; private Label skylarkLabel; - private String ruleClassName; - public RuleFunction(Builder builder, RuleClassType type) { + public RuleFunction(Builder builder, RuleClassType type, + ImmutableList> attributes, + Location definitionLocation) { super("rule", FunctionSignature.KWARGS); this.builder = builder; this.type = type; + this.attributes = attributes; + this.definitionLocation = definitionLocation; } @Override @@ -387,15 +400,10 @@ public class SkylarkRuleClassFunctions { throws EvalException, InterruptedException, ConversionException { env.checkLoadingPhase(getName(), ast.getLocation()); try { - if (ruleClassName == null || skylarkLabel == null) { + if (ruleClass == null) { throw new EvalException(ast.getLocation(), "Invalid rule class hasn't been exported by a Skylark file"); } - if (type == RuleClassType.TEST != TargetUtils.isTestRuleName(ruleClassName)) { - throw new EvalException(ast.getLocation(), "Invalid rule class name '" + ruleClassName - + "', test rule class names must end with '_test' and other rule classes must not"); - } - RuleClass ruleClass = builder.build(ruleClassName); PackageContext pkgContext = (PackageContext) env.lookup(PackageFactory.PKG_CONTEXT); return RuleFactory.createAndAddRule( pkgContext, ruleClass, (Map) args[0], ast, env); @@ -407,18 +415,32 @@ public class SkylarkRuleClassFunctions { /** * Export a RuleFunction from a Skylark file with a given name. */ - void export(Label skylarkLabel, String ruleClassName) { + void export(Label skylarkLabel, String ruleClassName) throws EvalException { + Preconditions.checkState(ruleClass == null && builder != null); this.skylarkLabel = skylarkLabel; - this.ruleClassName = ruleClassName; + if (type == RuleClassType.TEST != TargetUtils.isTestRuleName(ruleClassName)) { + throw new EvalException(definitionLocation, "Invalid rule class name '" + ruleClassName + + "', test rule class names must end with '_test' and other rule classes must not"); + } + for (Pair attribute : attributes) { + addAttribute(definitionLocation, builder, + attribute.getSecond().getAttributeBuilder().build(attribute.getFirst())); + } + this.ruleClass = builder.build(ruleClassName); + + this.builder = null; + this.attributes = null; } @VisibleForTesting - public RuleClass.Builder getBuilder() { - return builder; + public RuleClass getRuleClass() { + Preconditions.checkState(ruleClass != null && builder == null); + return ruleClass; } } - public static void exportRuleFunctionsAndAspects(Environment env, Label skylarkLabel) { + public static void exportRuleFunctionsAndAspects(Environment env, Label skylarkLabel) + throws EvalException { for (String name : env.getGlobals().getDirectVariableNames()) { try { Object value = env.lookup(name); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java index 9baa935cd1..5eed6a6ec4 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java @@ -34,6 +34,7 @@ import com.google.devtools.build.lib.packages.RuleClassProvider; import com.google.devtools.build.lib.rules.SkylarkRuleClassFunctions; import com.google.devtools.build.lib.syntax.BuildFileAST; import com.google.devtools.build.lib.syntax.Environment.Extension; +import com.google.devtools.build.lib.syntax.EvalException; import com.google.devtools.build.lib.syntax.LoadStatement; import com.google.devtools.build.lib.syntax.Mutability; import com.google.devtools.build.lib.vfs.PathFragment; @@ -376,7 +377,11 @@ public class SkylarkImportLookupFunction implements SkyFunction { mutability, eventHandler, ast.getContentHashCode(), importMap) .setupOverride("native", packageFactory.getNativeModule()); ast.exec(extensionEnv, eventHandler); - SkylarkRuleClassFunctions.exportRuleFunctionsAndAspects(extensionEnv, extensionLabel); + try { + SkylarkRuleClassFunctions.exportRuleFunctionsAndAspects(extensionEnv, extensionLabel); + } catch (EvalException e) { + eventHandler.handle(Event.error(e.getLocation(), e.getMessage())); + } Event.replayEventsOn(env.getListener(), eventHandler.getEvents()); if (eventHandler.hasErrors()) { -- cgit v1.2.3