diff options
Diffstat (limited to 'src')
15 files changed, 610 insertions, 157 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java index c07d950969..2cced64191 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java @@ -560,11 +560,13 @@ public class BuildView { // Tests. This must come last, so that the exclusive tests are scheduled after everything else. scheduleTestsIfRequested(parallelTests, exclusiveTests, topLevelOptions, allTargetsToTest); - String error = loadingResult.hasLoadingError() || skyframeAnalysisResult.hasLoadingError() - ? "execution phase succeeded, but there were loading phase errors" - : skyframeAnalysisResult.hasAnalysisError() - ? "execution phase succeeded, but not all targets were analyzed" - : null; + String error = loadingResult.hasTargetPatternError() + ? "execution phase successful, but there were errors parsing the target pattern" + : loadingResult.hasLoadingError() || skyframeAnalysisResult.hasLoadingError() + ? "execution phase succeeded, but there were loading phase errors" + : skyframeAnalysisResult.hasAnalysisError() + ? "execution phase succeeded, but not all targets were analyzed" + : null; final WalkableGraph graph = skyframeAnalysisResult.getWalkableGraph(); final ActionGraph actionGraph = new ActionGraph() { diff --git a/src/main/java/com/google/devtools/build/lib/packages/Aspect.java b/src/main/java/com/google/devtools/build/lib/packages/Aspect.java index dfd7e5c35d..93f3f4c248 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Aspect.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Aspect.java @@ -55,7 +55,7 @@ public final class Aspect implements DependencyFilter.AttributeInfoProvider { SkylarkAspectClass skylarkAspectClass, AspectDefinition definition, AspectParameters parameters) { - return new Aspect(skylarkAspectClass, definition, parameters); + return new Aspect(skylarkAspectClass, definition, parameters); } /** diff --git a/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java b/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java index c9b8a3b3a8..4e3f08a777 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java +++ b/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java @@ -23,6 +23,7 @@ import com.google.common.collect.Multimap; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.packages.ConfigurationFragmentPolicy.MissingFragmentPolicy; +import com.google.devtools.build.lib.syntax.Type; import com.google.devtools.build.lib.util.Preconditions; import java.util.Collection; @@ -289,7 +290,9 @@ public final class AspectDefinition { * configuration is available, starting with ':') */ public Builder add(Attribute attribute) { - Preconditions.checkArgument(attribute.isImplicit() || attribute.isLateBound()); + Preconditions.checkArgument(attribute.isImplicit() || attribute.isLateBound() + || (attribute.getType() == Type.STRING && attribute.checkAllowedValues()), + "Invalid attribute '%s' (%s)", attribute.getName(), attribute.getType()); Preconditions.checkArgument(!attributes.containsKey(attribute.getName()), "An attribute with the name '%s' already exists.", attribute.getName()); attributes.put(attribute.getName(), attribute); diff --git a/src/main/java/com/google/devtools/build/lib/packages/AspectParameters.java b/src/main/java/com/google/devtools/build/lib/packages/AspectParameters.java index 0cf175e130..6862f106ce 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/AspectParameters.java +++ b/src/main/java/com/google/devtools/build/lib/packages/AspectParameters.java @@ -58,7 +58,7 @@ public final class AspectParameters { } /** - * Returns collection of values for specified key, or null if key is missing. + * Returns collection of values for specified key, or an empty collection if key is missing. */ public ImmutableCollection<String> getAttribute(String key) { return attributes.get(key); diff --git a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java index 501b46cb05..2b8a738806 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java @@ -19,6 +19,7 @@ import com.google.common.base.Function; import com.google.common.base.Predicate; import com.google.common.base.Predicates; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.Sets; @@ -61,6 +62,9 @@ public final class Attribute implements Comparable<Attribute> { public static final Predicate<RuleClass> NO_RULE = Predicates.alwaysFalse(); + /** + * Wraps the information necessary to construct an Aspect. + */ private abstract static class RuleAspect<C extends AspectClass> { protected final C aspectClass; protected final Function<Rule, AspectParameters> parametersExtractor; @@ -70,6 +74,14 @@ public final class Attribute implements Comparable<Attribute> { this.parametersExtractor = parametersExtractor; } + public String getName() { + return this.aspectClass.getName(); + } + + public ImmutableSet<String> getRequiredParameters() { + return ImmutableSet.<String>of(); + } + public abstract Aspect getAspect(Rule rule); } @@ -86,19 +98,40 @@ public final class Attribute implements Comparable<Attribute> { } private static class SkylarkRuleAspect extends RuleAspect<SkylarkAspectClass> { - private final AspectDefinition definition; + private final SkylarkAspect aspect; + + public SkylarkRuleAspect(SkylarkAspect aspect) { + super(aspect.getAspectClass(), aspect.getDefaultParametersExtractor()); + this.aspect = aspect; + } + + @Override + public ImmutableSet<String> getRequiredParameters() { + return aspect.getParamAttributes(); + } + + @Override + public Aspect getAspect(Rule rule) { + AspectParameters parameters = parametersExtractor.apply(rule); + return Aspect.forSkylark(aspectClass, aspect.getDefinition(parameters), parameters); + } + } - public SkylarkRuleAspect(SkylarkAspectClass aspectClass, AspectDefinition definition) { - super(aspectClass, NO_PARAMETERS); - this.definition = definition; + /** + * A RuleAspect that just wraps a pre-existing Aspect that doesn't vary with the Rule. + * For instance, this may come from a DeserializedSkylarkAspect. + */ + private static class PredefinedRuleAspect extends RuleAspect<AspectClass> { + private Aspect aspect; + + public PredefinedRuleAspect(Aspect aspect) { + super(aspect.getAspectClass(), null); + this.aspect = aspect; } @Override public Aspect getAspect(Rule rule) { - return Aspect.forSkylark( - aspectClass, - definition, - parametersExtractor.apply(rule)); + return aspect; } } @@ -351,14 +384,13 @@ public final class Attribute implements Comparable<Attribute> { } } - private static final Function<Rule, AspectParameters> NO_PARAMETERS = - new Function<Rule, AspectParameters>() { - @Override - public AspectParameters apply(Rule input) { - return AspectParameters.EMPTY; - } - }; - + public ImmutableMap<String, ImmutableSet<String>> getRequiredAspectParameters() { + ImmutableMap.Builder<String, ImmutableSet<String>> paramBuilder = ImmutableMap.builder(); + for (RuleAspect<?> aspect : aspects) { + paramBuilder.put(aspect.getName(), aspect.getRequiredParameters()); + } + return paramBuilder.build(); + } /** * Creates a new attribute builder. @@ -871,8 +903,13 @@ public final class Attribute implements Comparable<Attribute> { return this.aspect(aspect, noParameters); } - public Builder<TYPE> aspect(SkylarkAspectClass aspectClass, AspectDefinition definition) { - this.aspects.add(new SkylarkRuleAspect(aspectClass, definition)); + public Builder<TYPE> aspect(SkylarkAspect skylarkAspect) { + this.aspects.add(new SkylarkRuleAspect(skylarkAspect)); + return this; + } + + public Builder<TYPE> aspect(final Aspect aspect) { + this.aspects.add(new PredefinedRuleAspect(aspect)); return this; } @@ -1637,8 +1674,9 @@ public final class Attribute implements Comparable<Attribute> { /** * Returns a replica builder of this Attribute. */ - public Attribute.Builder<?> cloneBuilder() { - Builder<?> builder = new Builder<>(name, this.type); + public <TYPE> Attribute.Builder<TYPE> cloneBuilder(Type<TYPE> tp) { + Preconditions.checkArgument(tp == this.type); + Builder<TYPE> builder = new Builder<TYPE>(name, tp); builder.allowedFileTypesForLabels = allowedFileTypesForLabels; builder.allowedRuleClassesForLabels = allowedRuleClassesForLabels; builder.allowedRuleClassesForLabelsWarning = allowedRuleClassesForLabelsWarning; @@ -1655,4 +1693,8 @@ public final class Attribute implements Comparable<Attribute> { return builder; } + + public Attribute.Builder<?> cloneBuilder() { + return cloneBuilder(this.type); + } } diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java index 7fd42dcca3..b4aac3be6e 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java +++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java @@ -1343,6 +1343,7 @@ public final class RuleClass { throws LabelSyntaxException, InterruptedException { Rule rule = pkgBuilder.createRule(ruleLabel, this, location, attributeContainer); populateRuleAttributeValues(rule, pkgBuilder, attributeValues, eventHandler); + checkAspectAllowedValues(rule, eventHandler); rule.populateOutputFiles(eventHandler, pkgBuilder); if (ast != null) { populateAttributeLocations(rule, ast); @@ -1741,6 +1742,30 @@ public final class RuleClass { } } + private static void checkAspectAllowedValues( + Rule rule, EventHandler eventHandler) { + for (Attribute attrOfRule : rule.getAttributes()) { + for (Aspect aspect : attrOfRule.getAspects(rule)) { + for (Attribute attrOfAspect : aspect.getDefinition().getAttributes().values()) { + // By this point the AspectDefinition has been created and values assigned. + if (attrOfAspect.checkAllowedValues()) { + PredicateWithMessage<Object> allowedValues = attrOfAspect.getAllowedValues(); + Object value = attrOfAspect.getDefaultValue(rule); + if (!allowedValues.apply(value)) { + rule.reportError( + String.format( + "%s: invalid value in '%s' attribute: %s", + rule.getLabel(), + attrOfAspect.getName(), + allowedValues.getErrorReason(value)), + eventHandler); + } + } + } + } + } + } + @Override public String toString() { return name; diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkAspect.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkAspect.java new file mode 100644 index 0000000000..d679273a56 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkAspect.java @@ -0,0 +1,172 @@ +// Copyright 2016 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.packages; + +import com.google.common.base.Function; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.skylarkinterface.SkylarkModule; +import com.google.devtools.build.lib.skylarkinterface.SkylarkValue; +import com.google.devtools.build.lib.syntax.BaseFunction; +import com.google.devtools.build.lib.syntax.Environment; +import com.google.devtools.build.lib.syntax.Printer; +import com.google.devtools.build.lib.syntax.Type; +import com.google.devtools.build.lib.util.Pair; +import com.google.devtools.build.lib.util.Preconditions; + +import javax.annotation.Nullable; + +/** + * A Skylark value that is a result of an 'aspect(..)' function call. + */ +@SkylarkModule(name = "aspect", doc = "", documented = false) +public class SkylarkAspect implements SkylarkValue { + private final BaseFunction implementation; + private final ImmutableList<String> attributeAspects; + private final ImmutableList<Attribute> attributes; + private final ImmutableSet<String> paramAttributes; + private final ImmutableSet<String> fragments; + private final ImmutableSet<String> hostFragments; + private final Environment funcallEnv; + private SkylarkAspectClass aspectClass; + + public SkylarkAspect( + BaseFunction implementation, + ImmutableList<String> attributeAspects, + ImmutableList<Attribute> attributes, + ImmutableSet<String> paramAttributes, + ImmutableSet<String> fragments, + ImmutableSet<String> hostFragments, + Environment funcallEnv) { + this.implementation = implementation; + this.attributeAspects = attributeAspects; + this.attributes = attributes; + this.paramAttributes = paramAttributes; + this.fragments = fragments; + this.hostFragments = hostFragments; + this.funcallEnv = funcallEnv; + ImmutableList.Builder<Pair<String, Attribute>> builder = ImmutableList.builder(); + } + + public BaseFunction getImplementation() { + return implementation; + } + + public ImmutableList<String> getAttributeAspects() { + return attributeAspects; + } + + public Environment getFuncallEnv() { + return funcallEnv; + } + + public ImmutableList<Attribute> getAttributes() { + return attributes; + } + + @Override + public boolean isImmutable() { + return implementation.isImmutable(); + } + + @Override + public void write(Appendable buffer, char quotationMark) { + Printer.append(buffer, "Aspect:"); + implementation.write(buffer, quotationMark); + } + + public String getName() { + return getAspectClass().getName(); + } + + public SkylarkAspectClass getAspectClass() { + Preconditions.checkState(isExported()); + return aspectClass; + } + + public ImmutableSet<String> getParamAttributes() { + return paramAttributes; + } + + public void export(Label extensionLabel, String name) { + Preconditions.checkArgument(!isExported()); + this.aspectClass = new SkylarkAspectClass(extensionLabel, name); + } + + public AspectDefinition getDefinition(AspectParameters aspectParams) { + AspectDefinition.Builder builder = new AspectDefinition.Builder(getName()); + for (String attributeAspect : attributeAspects) { + builder.attributeAspect(attributeAspect, aspectClass); + } + for (Attribute attribute : attributes) { + Attribute attr = attribute; // Might be reassigned. + if (!aspectParams.getAttribute(attr.getName()).isEmpty()) { + String value = aspectParams.getOnlyValueOfAttribute(attr.getName()); + Preconditions.checkState(!Attribute.isImplicit(attr.getName())); + Preconditions.checkState(attr.getType() == Type.STRING); + Preconditions.checkArgument(aspectParams.getAttribute(attr.getName()).size() == 1, + String.format("Aspect %s parameter %s has %d values (must have exactly 1).", + getName(), + attr.getName(), + aspectParams.getAttribute(attr.getName()).size())); + attr = attr.cloneBuilder(Type.STRING).value(value).build(attr.getName()); + } + builder.add(attr); + } + builder.requiresConfigurationFragmentsBySkylarkModuleName(fragments); + builder.requiresHostConfigurationFragmentsBySkylarkModuleName(hostFragments); + return builder.build(); + } + + public boolean isExported() { + return aspectClass != null; + } + + public Function<Rule, AspectParameters> getDefaultParametersExtractor() { + return new Function<Rule, AspectParameters>() { + @Nullable + @Override + public AspectParameters apply(Rule rule) { + AttributeMap ruleAttrs = RawAttributeMapper.of(rule); + AspectParameters.Builder builder = new AspectParameters.Builder(); + for (Attribute aspectAttr : attributes) { + if (!Attribute.isImplicit(aspectAttr.getName())) { + String param = aspectAttr.getName(); + Attribute ruleAttr = ruleAttrs.getAttributeDefinition(param); + if (paramAttributes.contains(aspectAttr.getName())) { + // These are preconditions because if they are false, RuleFunction.call() should + // already have generated an error. + Preconditions.checkArgument(ruleAttr != null, + String.format("Cannot apply aspect %s to %s that does not define attribute '%s'.", + getName(), + rule.getTargetKind(), + param)); + Preconditions.checkArgument(ruleAttr.getType() == Type.STRING, + String.format("Cannot apply aspect %s to %s with non-string attribute '%s'.", + getName(), + rule.getTargetKind(), + param)); + } + if (ruleAttr != null && ruleAttr.getType() == aspectAttr.getType()) { + builder.addAttribute(param, (String) ruleAttrs.get(param, ruleAttr.getType())); + } + } + } + return builder.build(); + } + }; + } +} 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 8561dfd80e..85ab569f3e 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 @@ -22,7 +22,7 @@ import com.google.devtools.build.lib.packages.Attribute.AllowedValueSet; import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition; import com.google.devtools.build.lib.packages.Attribute.SkylarkLateBound; import com.google.devtools.build.lib.packages.BuildType; -import com.google.devtools.build.lib.rules.SkylarkRuleClassFunctions.SkylarkAspect; +import com.google.devtools.build.lib.packages.SkylarkAspect; import com.google.devtools.build.lib.skylarkinterface.SkylarkModule; import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature; import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature.Param; 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 c8c0c86494..706a86ab87 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 @@ -46,7 +46,6 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.events.Location; -import com.google.devtools.build.lib.packages.AspectDefinition; import com.google.devtools.build.lib.packages.Attribute; import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition; import com.google.devtools.build.lib.packages.Attribute.LateBoundLabel; @@ -57,6 +56,7 @@ import com.google.devtools.build.lib.packages.ImplicitOutputsFunction.SkylarkImp import com.google.devtools.build.lib.packages.Package.NameConflictException; import com.google.devtools.build.lib.packages.PackageFactory; import com.google.devtools.build.lib.packages.PackageFactory.PackageContext; +import com.google.devtools.build.lib.packages.PredicateWithMessage; import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.RuleClass; import com.google.devtools.build.lib.packages.RuleClass.Builder; @@ -64,14 +64,12 @@ import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType; import com.google.devtools.build.lib.packages.RuleFactory; import com.google.devtools.build.lib.packages.RuleFactory.BuildLangTypedAttributeValuesMap; import com.google.devtools.build.lib.packages.RuleFactory.InvalidRuleException; -import com.google.devtools.build.lib.packages.SkylarkAspectClass; +import com.google.devtools.build.lib.packages.SkylarkAspect; import com.google.devtools.build.lib.packages.TargetUtils; import com.google.devtools.build.lib.packages.TestSize; import com.google.devtools.build.lib.rules.SkylarkAttr.Descriptor; -import com.google.devtools.build.lib.skylarkinterface.SkylarkModule; import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature; import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature.Param; -import com.google.devtools.build.lib.skylarkinterface.SkylarkValue; import com.google.devtools.build.lib.syntax.BaseFunction; import com.google.devtools.build.lib.syntax.BuiltinFunction; import com.google.devtools.build.lib.syntax.ClassObject; @@ -82,7 +80,6 @@ import com.google.devtools.build.lib.syntax.EvalException; import com.google.devtools.build.lib.syntax.EvalUtils; import com.google.devtools.build.lib.syntax.FuncallExpression; import com.google.devtools.build.lib.syntax.FunctionSignature; -import com.google.devtools.build.lib.syntax.Printer; import com.google.devtools.build.lib.syntax.Runtime; import com.google.devtools.build.lib.syntax.SkylarkCallbackFunction; import com.google.devtools.build.lib.syntax.SkylarkDict; @@ -415,9 +412,12 @@ public class SkylarkRuleClassFunctions { + "It maps from an attribute name to an attribute object " + "(see <a href=\"attr.html\">attr</a> module). " + "Aspect attributes are available to implementation function as fields of ctx parameter. " - + "All aspect attributes must be private, so their names must start with <code>_</code>. " - + "All aspect attributes must be have default values, and be of type " - + "<code>label</code> or <code>label_list</code>" + + "Implicit attributes starting with <code>_</code> must have default values, and have " + + "type <code>label</code> or <code>label_list</code>." + + "Explicit attributes must have type <code>string</code>, and must use the " + + "<code>values</code> restriction. If explicit attributes are present, the aspect can " + + "only be used with rules that have attributes of the same name and type, with valid " + + "values. " ), @Param( name = "fragments", @@ -465,31 +465,55 @@ public class SkylarkRuleClassFunctions { } } - ImmutableList<Pair<String, SkylarkAttr.Descriptor>> attributes = + ImmutableList<Pair<String, SkylarkAttr.Descriptor>> descriptors = attrObjectToAttributesList(attrs, ast); - for (Pair<String, Descriptor> attribute : attributes) { - String nativeName = attribute.getFirst(); - if (!Attribute.isImplicit(nativeName)) { - throw new EvalException( - ast.getLocation(), - String.format( - "Aspect attribute '%s' must be implicit (its name should start with '_').", - nativeName)); + ImmutableList.Builder<Attribute> attributes = ImmutableList.builder(); + ImmutableSet.Builder<String> requiredParams = ImmutableSet.<String>builder(); + for (Pair<String, Descriptor> descriptor : descriptors) { + String nativeName = descriptor.getFirst(); + boolean hasDefault = descriptor.getSecond().getAttributeBuilder().isValueSet(); + Attribute attribute = descriptor.second.getAttributeBuilder().build(descriptor.first); + if (attribute.getType() == Type.STRING + && ((String) attribute.getDefaultValue(null)).isEmpty()) { + hasDefault = false; // isValueSet() is always true for attr.string. } - - String skylarkName = "_" + nativeName.substring(1); - - if (!attribute.getSecond().getAttributeBuilder().isValueSet()) { + if (!Attribute.isImplicit(nativeName)) { + if (!attribute.checkAllowedValues() || attribute.getType() != Type.STRING) { + throw new EvalException( + ast.getLocation(), + String.format( + "Aspect parameter attribute '%s' must have type 'string' and use the " + + "'values' restriction.", + nativeName)); + } + if (!hasDefault) { + requiredParams.add(nativeName); + } else { + PredicateWithMessage<Object> allowed = attribute.getAllowedValues(); + Object defaultVal = attribute.getDefaultValue(null); + if (!allowed.apply(defaultVal)) { + throw new EvalException( + ast.getLocation(), + String.format( + "Aspect parameter attribute '%s' has a bad default value: %s", + nativeName, + allowed.getErrorReason(defaultVal))); + } + } + } else if (!hasDefault) { // Implicit attribute + String skylarkName = "_" + nativeName.substring(1); throw new EvalException( ast.getLocation(), String.format("Aspect attribute '%s' has no default value.", skylarkName)); } + attributes.add(attribute); } return new SkylarkAspect( implementation, attrAspects.build(), - attributes, + attributes.build(), + requiredParams.build(), ImmutableSet.copyOf(fragments.getContents(String.class, "fragments")), ImmutableSet.copyOf(hostFragments.getContents(String.class, "host_fragments")), funcallEnv); @@ -528,6 +552,24 @@ public class SkylarkRuleClassFunctions { throw new EvalException(ast.getLocation(), "Invalid rule class hasn't been exported by a Skylark file"); } + + for (Attribute attribute : ruleClass.getAttributes()) { + // TODO(dslomov): If a Skylark parameter extractor is specified for this aspect, its + // attributes may not be required. + for (Map.Entry<String, ImmutableSet<String>> attrRequirements : + attribute.getRequiredAspectParameters().entrySet()) { + for (String required : attrRequirements.getValue()) { + if (!ruleClass.hasAttr(required, Type.STRING)) { + throw new EvalException(definitionLocation, String.format( + "Aspect %s requires rule %s to specify attribute '%s' with type string.", + attrRequirements.getKey(), + ruleClass.getName(), + required)); + } + } + } + } + PackageContext pkgContext = (PackageContext) env.lookup(PackageFactory.PKG_CONTEXT); BuildLangTypedAttributeValuesMap attributeValues = new BuildLangTypedAttributeValuesMap((Map<String, Object>) args[0]); @@ -555,7 +597,7 @@ public class SkylarkRuleClassFunctions { throw new EvalException(definitionLocation, "All aspects applied to rule dependencies must be top-level values"); } - attributeBuilder.aspect(skylarkAspect.getAspectClass(), skylarkAspect.getDefinition()); + attributeBuilder.aspect(skylarkAspect); } addAttribute(definitionLocation, builder, @@ -848,91 +890,4 @@ public class SkylarkRuleClassFunctions { static { SkylarkSignatureProcessor.configureSkylarkFunctions(SkylarkRuleClassFunctions.class); } - - /** - * A Skylark value that is a result of 'aspect(..)' function call. - */ - @SkylarkModule(name = "aspect", doc = "", documented = false) - public static final class SkylarkAspect implements SkylarkValue { - private final BaseFunction implementation; - private final ImmutableList<String> attributeAspects; - private final ImmutableList<Pair<String, Descriptor>> attributes; - private final ImmutableSet<String> fragments; - private final ImmutableSet<String> hostFragments; - private final Environment funcallEnv; - private SkylarkAspectClass aspectClass; - - public SkylarkAspect( - BaseFunction implementation, - ImmutableList<String> attributeAspects, - ImmutableList<Pair<String, Descriptor>> attributes, - ImmutableSet<String> fragments, - ImmutableSet<String> hostFragments, - Environment funcallEnv) { - this.implementation = implementation; - this.attributeAspects = attributeAspects; - this.attributes = attributes; - this.fragments = fragments; - this.hostFragments = hostFragments; - this.funcallEnv = funcallEnv; - } - - public BaseFunction getImplementation() { - return implementation; - } - - public ImmutableList<String> getAttributeAspects() { - return attributeAspects; - } - - public Environment getFuncallEnv() { - return funcallEnv; - } - - public ImmutableList<Pair<String, Descriptor>> getAttributes() { - return attributes; - } - - @Override - public boolean isImmutable() { - return implementation.isImmutable(); - } - - @Override - public void write(Appendable buffer, char quotationMark) { - Printer.append(buffer, "Aspect:"); - implementation.write(buffer, quotationMark); - } - - public String getName() { - return getAspectClass().getName(); - } - - public SkylarkAspectClass getAspectClass() { - Preconditions.checkState(isExported()); - return aspectClass; - } - - void export(Label extensionLabel, String name) { - Preconditions.checkArgument(!isExported()); - this.aspectClass = new SkylarkAspectClass(extensionLabel, name); - } - - public boolean isExported() { - return aspectClass != null; - } - - public AspectDefinition getDefinition() { - AspectDefinition.Builder builder = new AspectDefinition.Builder(getName()); - for (String attributeAspect : attributeAspects) { - builder.attributeAspect(attributeAspect, aspectClass); - } - for (Pair<String, Descriptor> attribute : attributes) { - builder.add(attribute.second.getAttributeBuilder().build(attribute.first)); - } - builder.requiresConfigurationFragmentsBySkylarkModuleName(fragments); - builder.requiresHostConfigurationFragmentsBySkylarkModuleName(hostFragments); - return builder.build(); - } - } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java index d201d880cd..9bf225f1de 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java @@ -41,10 +41,10 @@ import com.google.devtools.build.lib.packages.NoSuchThingException; import com.google.devtools.build.lib.packages.Package; import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.RuleClassProvider; +import com.google.devtools.build.lib.packages.SkylarkAspect; import com.google.devtools.build.lib.packages.SkylarkAspectClass; import com.google.devtools.build.lib.packages.Target; import com.google.devtools.build.lib.packages.TargetUtils; -import com.google.devtools.build.lib.rules.SkylarkRuleClassFunctions.SkylarkAspect; import com.google.devtools.build.lib.skyframe.AspectValue.AspectKey; import com.google.devtools.build.lib.skyframe.ConfiguredTargetFunction.ConfiguredValueCreationException; import com.google.devtools.build.lib.skyframe.ConfiguredTargetFunction.DependencyEvaluationException; @@ -144,7 +144,7 @@ public final class AspectFunction implements SkyFunction { aspectFactory = new SkylarkAspectFactory(skylarkAspect); aspect = Aspect.forSkylark( skylarkAspect.getAspectClass(), - skylarkAspect.getDefinition(), + skylarkAspect.getDefinition(key.getParameters()), key.getParameters()); } else { throw new IllegalStateException(); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java index d82a9026cc..b0dfa3c3e6 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java @@ -178,9 +178,12 @@ final class SkyframeTargetPatternEvaluator implements TargetPatternEvaluator { } } - if (!keepGoing && result.hasError()) { + if (result.hasError()) { Preconditions.checkState(errorMessage != null, "unexpected errors: %s", result.errorMap()); - throw new TargetParsingException(errorMessage); + finalTargetSetEvaluator.setError(); + if (!keepGoing) { + throw new TargetParsingException(errorMessage); + } } WalkableGraph walkableGraph = Preconditions.checkNotNull(result.getWalkableGraph(), result); return finalTargetSetEvaluator.build(walkableGraph); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java index 5500bc8b99..33b544bb34 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java @@ -23,7 +23,7 @@ import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.analysis.SkylarkProviderValidationUtil; import com.google.devtools.build.lib.events.Location; import com.google.devtools.build.lib.packages.AspectParameters; -import com.google.devtools.build.lib.rules.SkylarkRuleClassFunctions.SkylarkAspect; +import com.google.devtools.build.lib.packages.SkylarkAspect; import com.google.devtools.build.lib.rules.SkylarkRuleContext; import com.google.devtools.build.lib.rules.SkylarkRuleContext.Kind; import com.google.devtools.build.lib.syntax.ClassObject.SkylarkClassObject; diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java index ed93b8e659..785e7407f4 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java @@ -19,7 +19,7 @@ import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.packages.AspectParameters; -import com.google.devtools.build.lib.rules.SkylarkRuleClassFunctions.SkylarkAspect; +import com.google.devtools.build.lib.packages.SkylarkAspect; import com.google.devtools.build.lib.skyframe.AspectFunction.AspectCreationException; import com.google.devtools.build.lib.skyframe.AspectValue.SkylarkAspectLoadingKey; import com.google.devtools.build.lib.skyframe.SkylarkImportLookupFunction.SkylarkImportFailedException; @@ -66,12 +66,16 @@ public class ToplevelSkylarkAspectFunction implements SkyFunction { try { skylarkAspect = AspectFunction.loadSkylarkAspect( env, labelLookupMap.get(extensionFile), skylarkValueName); + if (skylarkAspect == null) { + return null; + } + if (!skylarkAspect.getParamAttributes().isEmpty()) { + throw new AspectCreationException("Cannot instantiate parameterized aspect " + + skylarkAspect.getName() + " at the top level.", labelLookupMap.get(extensionFile)); + } } catch (AspectCreationException e) { throw new LoadSkylarkAspectFunctionException(e); } - if (skylarkAspect == null) { - return null; - } SkyKey aspectKey = AspectValue.key( aspectLoadingKey.getTargetLabel(), diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java index 4d0514873f..35514449e1 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java @@ -532,6 +532,215 @@ public class SkylarkAspectsTest extends AnalysisTestCase { } @Test + public void testAspectParametersUncovered() throws Exception { + scratch.file( + "test/aspect.bzl", + "def _impl(target, ctx):", + " return struct()", + "def _rule_impl(ctx):", + " return struct()", + "MyAspectUncovered = aspect(", + " implementation=_impl,", + " attrs = { 'my_attr' : attr.string(values=['aaa']) },", + ")", + "my_rule = rule(", + " implementation=_rule_impl,", + " attrs = { 'deps' : attr.label_list(aspects=[MyAspectUncovered]) },", + ")"); + scratch.file("test/BUILD", + "load('//test:aspect.bzl', 'my_rule')", + "my_rule(name = 'xxx')"); + + reporter.removeHandler(failFastHandler); + try { + AnalysisResult result = update(ImmutableList.<String>of(), "//test:xxx"); + assertThat(keepGoing()).isTrue(); + assertThat(result.hasError()).isTrue(); + } catch (Exception e) { + // expect to fail. + } + assertContainsEvent(//"ERROR /workspace/test/aspect.bzl:9:11: " + "Aspect //test:aspect.bzl%MyAspectUncovered requires rule my_rule to specify attribute " + + "'my_attr' with type string."); + } + + @Test + public void testAspectParametersTypeMismatch() throws Exception { + scratch.file( + "test/aspect.bzl", + "def _impl(target, ctx):", + " return struct()", + "def _rule_impl(ctx):", + " return struct()", + "MyAspectMismatch = aspect(", + " implementation=_impl,", + " attrs = { 'my_attr' : attr.string(values=['aaa']) },", + ")", + "my_rule = rule(", + " implementation=_rule_impl,", + " attrs = { 'deps' : attr.label_list(aspects=[MyAspectMismatch]),", + " 'my_attr' : attr.int() },", + ")"); + scratch.file("test/BUILD", + "load('//test:aspect.bzl', 'my_rule')", + "my_rule(name = 'xxx', my_attr = 4)"); + + reporter.removeHandler(failFastHandler); + try { + AnalysisResult result = update(ImmutableList.<String>of(), "//test:xxx"); + assertThat(keepGoing()).isTrue(); + assertThat(result.hasError()).isTrue(); + } catch (Exception e) { + // expect to fail. + } + assertContainsEvent( + "Aspect //test:aspect.bzl%MyAspectMismatch requires rule my_rule to specify attribute " + + "'my_attr' with type string."); + } + + @Test + public void testAspectParametersBadDefault() throws Exception { + scratch.file( + "test/aspect.bzl", + "def _impl(target, ctx):", + " return struct()", + "def _rule_impl(ctx):", + " return struct()", + "MyAspectBadDefault = aspect(", + " implementation=_impl,", + " attrs = { 'my_attr' : attr.string(values=['a'], default='b') },", + ")", + "my_rule = rule(", + " implementation=_rule_impl,", + " attrs = { 'deps' : attr.label_list(aspects=[MyAspectBadDefault]) },", + ")"); + scratch.file("test/BUILD", + "load('//test:aspect.bzl', 'my_rule')", + "my_rule(name = 'xxx')"); + + reporter.removeHandler(failFastHandler); + try { + AnalysisResult result = update(ImmutableList.<String>of(), "//test:xxx"); + assertThat(keepGoing()).isTrue(); + assertThat(result.hasError()).isTrue(); + } catch (Exception e) { + // expect to fail. + } + assertContainsEvent("ERROR /workspace/test/aspect.bzl:5:22: " + + "Aspect parameter attribute 'my_attr' has a bad default value: has to be one of 'a' " + + "instead of 'b'"); + } + + @Test + public void testAspectParametersBadValue() throws Exception { + scratch.file( + "test/aspect.bzl", + "def _impl(target, ctx):", + " return struct()", + "def _rule_impl(ctx):", + " return struct()", + "MyAspectBadValue = aspect(", + " implementation=_impl,", + " attrs = { 'my_attr' : attr.string(values=['a']) },", + ")", + "my_rule = rule(", + " implementation=_rule_impl,", + " attrs = { 'deps' : attr.label_list(aspects=[MyAspectBadValue]),", + " 'my_attr' : attr.string() },", + ")"); + scratch.file("test/BUILD", + "load('//test:aspect.bzl', 'my_rule')", + "my_rule(name = 'xxx', my_attr='b')"); + + reporter.removeHandler(failFastHandler); + try { + AnalysisResult result = update(ImmutableList.<String>of(), "//test:xxx"); + assertThat(keepGoing()).isTrue(); + assertThat(result.hasError()).isTrue(); + } catch (Exception e) { + // expect to fail. + } + assertContainsEvent("ERROR /workspace/test/BUILD:2:1: //test:xxx: invalid value in 'my_attr' " + + "attribute: has to be one of 'a' instead of 'b'"); + } + + @Test + public void testAspectParameters() throws Exception { + scratch.file( + "test/aspect.bzl", + "def _impl(target, ctx):", + " return struct()", + "def _rule_impl(ctx):", + " return struct()", + "MyAspect = aspect(", + " implementation=_impl,", + " attrs = { 'my_attr' : attr.string(values=['aaa']) },", + ")", + "my_rule = rule(", + " implementation=_rule_impl,", + " attrs = { 'deps' : attr.label_list(aspects=[MyAspect]),", + " 'my_attr' : attr.string() },", + ")"); + scratch.file("test/BUILD", + "load('//test:aspect.bzl', 'my_rule')", + "my_rule(name = 'xxx', my_attr = 'aaa')"); + + AnalysisResult result = update(ImmutableList.<String>of(), "//test:xxx"); + assertThat(result.hasError()).isFalse(); + } + + @Test + public void testAspectParametersOptional() throws Exception { + scratch.file( + "test/aspect.bzl", + "def _impl(target, ctx):", + " return struct()", + "def _rule_impl(ctx):", + " return struct()", + "MyAspectOptParam = aspect(", + " implementation=_impl,", + " attrs = { 'my_attr' : attr.string(values=['aaa'], default='aaa') },", + ")", + "my_rule = rule(", + " implementation=_rule_impl,", + " attrs = { 'deps' : attr.label_list(aspects=[MyAspectOptParam]) },", + ")"); + scratch.file("test/BUILD", + "load('//test:aspect.bzl', 'my_rule')", + "my_rule(name = 'xxx')"); + + AnalysisResult result = update(ImmutableList.<String>of(), "//test:xxx"); + assertThat(result.hasError()).isFalse(); + } + + @Test + public void testAspectParametersOptionalOverride() throws Exception { + scratch.file( + "test/aspect.bzl", + "def _impl(target, ctx):", + " if (ctx.attr.my_attr == 'a'):", + " fail('Rule is not overriding default, still has value ' + ctx.attr.my_attr)", + " return struct()", + "def _rule_impl(ctx):", + " return struct()", + "MyAspectOptOverride = aspect(", + " implementation=_impl,", + " attrs = { 'my_attr' : attr.string(values=['a', 'b'], default='a') },", + ")", + "my_rule = rule(", + " implementation=_rule_impl,", + " attrs = { 'deps' : attr.label_list(aspects=[MyAspectOptOverride]),", + " 'my_attr' : attr.string() },", + ")"); + scratch.file("test/BUILD", + "load('//test:aspect.bzl', 'my_rule')", + "my_rule(name = 'xxx', my_attr = 'b')"); + + AnalysisResult result = update(ImmutableList.<String>of(), "//test:xxx"); + assertThat(result.hasError()).isFalse(); + } + + @Test public void multipleExecutablesInTarget() throws Exception { scratch.file("foo/extension.bzl", "def _aspect_impl(target, ctx):", diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java index 81636b1aeb..50bd25ac56 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java @@ -32,17 +32,15 @@ import com.google.devtools.build.lib.packages.ImplicitOutputsFunction; import com.google.devtools.build.lib.packages.PredicateWithMessage; import com.google.devtools.build.lib.packages.RuleClass; import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType; +import com.google.devtools.build.lib.packages.SkylarkAspect; import com.google.devtools.build.lib.rules.SkylarkAttr; -import com.google.devtools.build.lib.rules.SkylarkAttr.Descriptor; import com.google.devtools.build.lib.rules.SkylarkFileType; import com.google.devtools.build.lib.rules.SkylarkRuleClassFunctions; import com.google.devtools.build.lib.rules.SkylarkRuleClassFunctions.RuleFunction; -import com.google.devtools.build.lib.rules.SkylarkRuleClassFunctions.SkylarkAspect; import com.google.devtools.build.lib.skylark.util.SkylarkTestCase; import com.google.devtools.build.lib.syntax.EvalException; import com.google.devtools.build.lib.syntax.Type; import com.google.devtools.build.lib.util.FileTypeSet; -import com.google.devtools.build.lib.util.Pair; import org.junit.Assert; import org.junit.Before; @@ -237,24 +235,64 @@ public class SkylarkRuleClassFunctionsTest extends SkylarkTestCase { " attrs = { '_extra_deps' : attr.label(default = Label('//foo/bar:baz')) }", ")"); SkylarkAspect aspect = (SkylarkAspect) ev.lookup("my_aspect"); - Pair<String, Descriptor> pair = Iterables.getOnlyElement(aspect.getAttributes()); - assertThat(pair.first).isEqualTo("$extra_deps"); - assertThat(pair.second.getAttributeBuilder().build("$extra_deps").getDefaultValue(null)) + Attribute attribute = Iterables.getOnlyElement(aspect.getAttributes()); + assertThat(attribute.getName()).isEqualTo("$extra_deps"); + assertThat(attribute.getDefaultValue(null)) .isEqualTo(Label.parseAbsolute("//foo/bar:baz", false)); } @Test - public void testAspectNonImplicitAttribute() throws Exception { + public void testAspectParameter() throws Exception { + evalAndExport( + "def _impl(target, ctx):", + " pass", + "my_aspect = aspect(_impl,", + " attrs = { 'param' : attr.string(values=['a', 'b']) }", + ")"); + SkylarkAspect aspect = (SkylarkAspect) ev.lookup("my_aspect"); + Attribute attribute = Iterables.getOnlyElement(aspect.getAttributes()); + assertThat(attribute.getName()).isEqualTo("param"); + } + + @Test + public void testAspectParameterRequiresValues() throws Exception { checkErrorContains( - "Aspect attribute 'extra_deps' must be implicit (its name should start with '_')", + "Aspect parameter attribute 'param' must have type 'string' and use the 'values' " + + "restriction.", "def _impl(target, ctx):", " pass", "my_aspect = aspect(_impl,", - " attrs = { 'extra_deps' : attr.label(default = Label('//foo/bar:baz')) }", + " attrs = { 'param' : attr.string(default = 'c') }", ")"); } @Test + public void testAspectParameterBadType() throws Exception { + checkErrorContains( + "Aspect parameter attribute 'param' must have type 'string' and use the 'values' " + + "restriction.", + "def _impl(target, ctx):", + " pass", + "my_aspect = aspect(_impl,", + " attrs = { 'param' : attr.label(default = Label('//foo/bar:baz')) }", + ")"); + } + + @Test + public void testAspectParameterAndExtraDeps() throws Exception { + evalAndExport( + "def _impl(target, ctx):", + " pass", + "my_aspect = aspect(_impl,", + " attrs = { 'param' : attr.string(values=['a', 'b']),", + " '_extra' : attr.label(default = Label('//foo/bar:baz')) }", + ")"); + SkylarkAspect aspect = (SkylarkAspect) ev.lookup("my_aspect"); + assertThat(aspect.getAttributes()).hasSize(2); + assertThat(aspect.getParamAttributes()).containsExactly("param"); + } + + @Test public void testAspectNoDefaultValueAttribute() throws Exception { checkErrorContains( "Aspect attribute '_extra_deps' has no default value", |