From d2582637866f28272addd9935a513aaa488c7ec7 Mon Sep 17 00:00:00 2001 From: brandjon Date: Fri, 29 Jan 2021 16:10:58 -0800 Subject: [PATCH] Don't call Starlark implicit outputs functions twice Previously, a Starlark rule's implicit outputs function would be called twice: once during loading to help populate the complete set of outputs, and then again at the start of analysis while constructing the `ctx.outputs` object. This double Starlark evaluation was not only wasteful, but caused StarlarkRuleContext's constructor to potentially throw EvalException and InterruptedException, complicating refactoring of that code. This change makes it so we remember the result of the first call in Rule and access it later. Initial versions of this CL attempted to store the implicit output function results in a new field on Rule. However, this added ~0.5% RAM overhead to a large `build --nobuild` invocation, and ~1.5% on a `query 'deps(...)'` of the same target. (This increase was not attributable simply to the addition of the field itself, but to the map entries it contained.) The new strategy is to take an existing field that held all the outputs (implicit and explicit), and expand it to also include the string keys of Starlark implicit outputs. Not only does this avoid adding new redundant fields, it allows us to contract an existing one. The tradeoff is that the accessors must do some computational work to copy only the relevant data from the field. The stored map is compressed as a flat list for further space savings. Along the way, I refactored the code that populates this collection of outputs. The diff is larger than I would've liked, but essentially the bodies of populateExplicitOutputFiles() and populateImplicitOutputFiles() got lifted into lambdas in the caller (with a little inlining of helpers), to better update state during the computation. There may be further refactoring that could be done, e.g. changing the mode of one failure to do a rule error instead of a LabelSyntaxException, or combining validation logic into one place, but I'm trying to avoid any more semantic changes than are absolutely necessary. On the benchmark alluded to above, we save 0.93% RAM on a query invocation, and 0.40% on a build --nobuild. Cleanup work toward #11437. PiperOrigin-RevId: 354631199 --- .../build/lib/analysis/RuleContext.java | 2 + .../starlark/StarlarkRuleClassFunctions.java | 4 + .../StarlarkRuleConfiguredTargetUtil.java | 8 +- .../starlark/StarlarkRuleContext.java | 73 ++-- .../devtools/build/lib/packages/Rule.java | 376 ++++++++++++------ .../lib/skyframe/StarlarkAspectFactory.java | 7 +- .../lib/starlark/StarlarkIntegrationTest.java | 2 +- 7 files changed, 294 insertions(+), 178 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java index 27913ecfa63650..9dfef7e1ac7164 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java @@ -1606,6 +1606,8 @@ public Artifact getImplicitOutputArtifact(String path) { * content-based path for this artifact (see {@link * BuildConfiguration#useContentBasedOutputPaths()}). */ + // TODO(bazel-team): Consider removing contentBasedPath stuff, which is unused as of 18 months + // after its introduction in cl/252148134. private Artifact getImplicitOutputArtifact(String path, boolean contentBasedPath) { return getPackageRelativeArtifact(path, getBinOrGenfilesDirectory(), contentBasedPath); } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java index c101a298468c05..fe9626ac875f98 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java @@ -334,6 +334,10 @@ public StarlarkCallable rule( if (implicitOutputs != Starlark.NONE) { if (implicitOutputs instanceof StarlarkFunction) { + // TODO(brandjon): Embedding bazelContext in a callback is not thread safe! Instead + // construct a new BazelStarlarkContext with copies of the relevant fields that are safe to + // share. Maybe create a method on BazelStarlarkContext for safely constructing a child + // context. StarlarkCallbackHelper callback = new StarlarkCallbackHelper( (StarlarkFunction) implicitOutputs, thread.getSemantics(), bazelContext); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleConfiguredTargetUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleConfiguredTargetUtil.java index 7c67f3d4b2717e..a579c46f4cfd77 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleConfiguredTargetUtil.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleConfiguredTargetUtil.java @@ -84,14 +84,8 @@ public static ConfiguredTarget buildRule( String toolsRepository) throws InterruptedException, RuleErrorException, ActionConflictException { String expectFailure = ruleContext.attributes().get("expect_failure", Type.STRING); - StarlarkRuleContext starlarkRuleContext = null; + StarlarkRuleContext starlarkRuleContext = new StarlarkRuleContext(ruleContext, null); try { - // The StarlarkRuleContext constructor may throw EvalException due - // to StarlarkImplicitOutputsFunction, which shouldn't be called at - // analysis time anyway since all such targets should be created during loading. - // TODO(adonovan): clean it up. - starlarkRuleContext = new StarlarkRuleContext(ruleContext, null); - RuleClass ruleClass = ruleContext.getRule().getRuleClassObject(); if (ruleClass.getRuleClassType().equals(RuleClass.Builder.RuleClassType.WORKSPACE)) { ruleContext.ruleError( diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java index 84c0070036f26e..8cc74a480be809 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java @@ -21,6 +21,7 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; @@ -61,11 +62,10 @@ import com.google.devtools.build.lib.packages.BuildSetting; import com.google.devtools.build.lib.packages.BuildType; import com.google.devtools.build.lib.packages.ImplicitOutputsFunction; -import com.google.devtools.build.lib.packages.ImplicitOutputsFunction.StarlarkImplicitOutputsFunction; import com.google.devtools.build.lib.packages.OutputFile; import com.google.devtools.build.lib.packages.Package; import com.google.devtools.build.lib.packages.Provider; -import com.google.devtools.build.lib.packages.RawAttributeMapper; +import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException; import com.google.devtools.build.lib.packages.StructImpl; import com.google.devtools.build.lib.packages.StructProvider; @@ -137,10 +137,9 @@ public final class StarlarkRuleContext implements StarlarkRuleContextApinull if it is * for a rule. - * @throws InterruptedException */ public StarlarkRuleContext(RuleContext ruleContext, @Nullable AspectDescriptor aspectDescriptor) - throws EvalException, InterruptedException, RuleErrorException { + throws RuleErrorException { // Init ruleContext first, we need it to obtain the StarlarkSemantics used by // StarlarkActionFactory (and possibly others). this.ruleContext = Preconditions.checkNotNull(ruleContext); @@ -150,40 +149,37 @@ public StarlarkRuleContext(RuleContext ruleContext, @Nullable AspectDescriptor a this.hostFragments = new FragmentCollection(ruleContext, HostTransition.INSTANCE); this.aspectDescriptor = aspectDescriptor; + Rule rule = ruleContext.getRule(); + if (aspectDescriptor == null) { this.isForAspect = false; - Collection attributes = ruleContext.getRule().getAttributes(); - Outputs outputs = new Outputs(this); - - ImplicitOutputsFunction implicitOutputsFunction = - ruleContext.getRule().getImplicitOutputsFunction(); - - if (implicitOutputsFunction instanceof StarlarkImplicitOutputsFunction) { - StarlarkImplicitOutputsFunction func = - (StarlarkImplicitOutputsFunction) implicitOutputsFunction; - for (Map.Entry entry : - func.calculateOutputs( - ruleContext.getAnalysisEnvironment().getEventHandler(), - RawAttributeMapper.of(ruleContext.getRule())) - .entrySet()) { - outputs.addOutput( - entry.getKey(), - ruleContext.getImplicitOutputArtifact(entry.getValue())); - } - } + Collection attributes = rule.getAttributes(); + // Populate ctx.outputs. + Outputs outputs = new Outputs(this); + // These getters do some computational work to return a view, so ensure we only do it once. + ImmutableListMultimap explicitOutMap = rule.getExplicitOutputFileMap(); + ImmutableMap implicitOutMap = rule.getStarlarkImplicitOutputFileMap(); + // Add the explicit outputs -- values of attributes of type OUTPUT or OUTPUT_LIST. + // We must iterate over the attribute definitions, and not just the entries in the + // explicitOutMap, because the latter omits empty output attributes, which must still + // generate None or [] fields in the struct. for (Attribute a : attributes) { + // Skip non-output attrs. String attrName = a.getName(); Type type = a.getType(); if (type.getLabelClass() != LabelClass.OUTPUT) { continue; } + + // Grab all associated outputs. ImmutableList.Builder artifactsBuilder = ImmutableList.builder(); - for (OutputFile outputFile : ruleContext.getRule().getOutputFileMap().get(attrName)) { + for (OutputFile outputFile : explicitOutMap.get(attrName)) { artifactsBuilder.add(ruleContext.createOutputArtifact(outputFile)); } StarlarkList artifacts = StarlarkList.immutableCopyOf(artifactsBuilder.build()); + // For singular output attributes, unwrap sole element or else use None for arity mismatch. if (type == BuildType.OUTPUT) { if (artifacts.size() == 1) { outputs.addOutput(attrName, Iterables.getOnlyElement(artifacts)); @@ -193,15 +189,23 @@ public StarlarkRuleContext(RuleContext ruleContext, @Nullable AspectDescriptor a } else if (type == BuildType.OUTPUT_LIST) { outputs.addOutput(attrName, artifacts); } else { - throw new IllegalArgumentException( - "Type of " + attrName + "(" + type + ") is not output type "); + throw new AssertionError( + String.format("Attribute %s has unexpected output type %s", attrName, type)); } } + // Add the implicit outputs. In the case where the rule has a native-defined implicit outputs + // function, nothing is added. Note that Rule ensures that Starlark-defined implicit output + // keys don't conflict with output attribute names. + // TODO(bazel-team): Also see about requiring the key to be a valid Starlark identifier. + for (Map.Entry e : implicitOutMap.entrySet()) { + outputs.addOutput(e.getKey(), ruleContext.createOutputArtifact(e.getValue())); + } this.outputsObject = outputs; + // Populate ctx.attr. StarlarkAttributesCollection.Builder builder = StarlarkAttributesCollection.builder(this); - for (Attribute attribute : ruleContext.getRule().getAttributes()) { + for (Attribute attribute : attributes) { Object value = ruleContext.attributes().get(attribute.getName(), attribute.getType()); builder.addAttribute(attribute, value); } @@ -212,13 +216,13 @@ public StarlarkRuleContext(RuleContext ruleContext, @Nullable AspectDescriptor a } else { // ASPECT this.isForAspect = true; this.outputsObject = null; - ImmutableCollection attributes = ruleContext.getMainAspect().getDefinition().getAttributes().values(); + StarlarkAttributesCollection.Builder aspectBuilder = StarlarkAttributesCollection.builder(this); for (Attribute attribute : attributes) { - Object defaultValue = attribute.getDefaultValue(ruleContext.getRule()); + Object defaultValue = attribute.getDefaultValue(rule); if (defaultValue instanceof ComputedDefault) { defaultValue = ((ComputedDefault) defaultValue).getDefault(ruleContext.attributes()); } @@ -229,7 +233,7 @@ public StarlarkRuleContext(RuleContext ruleContext, @Nullable AspectDescriptor a this.splitAttributes = null; StarlarkAttributesCollection.Builder ruleBuilder = StarlarkAttributesCollection.builder(this); - for (Attribute attribute : ruleContext.getRule().getAttributes()) { + for (Attribute attribute : rule.getAttributes()) { Object value = ruleContext.attributes().get(attribute.getName(), attribute.getType()); ruleBuilder.addAttribute(attribute, value); } @@ -239,7 +243,7 @@ public StarlarkRuleContext(RuleContext ruleContext, @Nullable AspectDescriptor a continue; } for (Attribute attribute : aspect.getDefinition().getAttributes().values()) { - Object defaultValue = attribute.getDefaultValue(ruleContext.getRule()); + Object defaultValue = attribute.getDefaultValue(rule); if (defaultValue instanceof ComputedDefault) { defaultValue = ((ComputedDefault) defaultValue).getDefault(ruleContext.attributes()); } @@ -276,13 +280,14 @@ public Outputs(StarlarkRuleContext context) { this.context = context; } - private void addOutput(String key, Object value) - throws EvalException { + private void addOutput(String key, Object value) throws RuleErrorException { Preconditions.checkState(!context.isImmutable(), "Cannot add outputs to immutable Outputs object"); + // TODO(bazel-team): We should reject outputs whose key is not an identifier. Today this is + // allowed, and the resulting ctx.outputs value can be retrieved using getattr(). if (outputs.containsKey(key) || (context.isExecutable() && EXECUTABLE_OUTPUT_NAME.equals(key))) { - throw Starlark.errorf("Multiple outputs with the same key: %s", key); + context.getRuleContext().throwWithRuleError("Multiple outputs with the same key: " + key); } outputs.put(key, value); } diff --git a/src/main/java/com/google/devtools/build/lib/packages/Rule.java b/src/main/java/com/google/devtools/build/lib/packages/Rule.java index 0c6a2c3d274188..f7aa3819f5e2c9 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Rule.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Rule.java @@ -20,22 +20,26 @@ import com.google.common.collect.HashMultimap; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableListMultimap; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSortedSet; import com.google.common.collect.LinkedHashMultimap; -import com.google.common.collect.ListMultimap; import com.google.common.collect.Lists; import com.google.common.collect.Multimap; import com.google.common.collect.SetMultimap; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.LabelSyntaxException; +import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; +import com.google.devtools.build.lib.packages.ImplicitOutputsFunction.StarlarkImplicitOutputsFunction; import com.google.devtools.build.lib.packages.License.DistributionType; import com.google.devtools.build.lib.util.BinaryPredicate; import java.util.Collection; +import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; +import java.util.Map; import java.util.Set; import javax.annotation.Nullable; import net.starlark.java.eval.EvalException; @@ -84,9 +88,35 @@ public class Rule implements Target, DependencyFilter.AttributeInfoProvider { private final ImplicitOutputsFunction implicitOutputsFunction; - // Initialized in the call to populateOutputFiles. - private List outputFiles; - private ListMultimap outputFileMap; + /** + * A compact representation of a multimap from "output keys" to output files. + * + *

An output key is an identifier used to access the output in {@code ctx.outputs}, or the + * empty string in the case of an output that's not exposed there. For explicit outputs, the + * output key is the name of the attribute under which that output appears. For Starlark-defined + * implicit outputs, the output key is determined by the dict returned from the Starlark function. + * Native-defined implicit outputs are not named in this manner, and so are invisible to {@code + * ctx.outputs} and use the empty string key. (It'd be pathological for the empty string to be + * used as a key in the other two cases, but this class makes no attempt to prohibit that.) + * + *

Rather than naively store an ImmutableListMultimap, we save space by compressing it as an + * ImmutableList, where each key is followed by all the values having that key. We distinguish + * keys (Strings) from values (OutputFiles) by the fact that they have different types. The + * accessor methods traverse the list and create a more user-friendly view. + * + *

To distinguish implicit outputs from explicit outputs, we store all the implicit outputs in + * the list first, and record how many implicit output keys there are in a separate field. + * + *

The order of the implicit outputs is the same as returned by the implicit output function. + * This allows a native rule implementation and native implicit outputs function to agree on the + * index of a given kind of output. The order of explicit outputs preserves the attribute + * iteration order and the order of values in a list attribute; the latter is important so that + * {@code ctx.outputs.some_list} has a well-defined order. + */ + // Both of these fields are initialized by populateOutputFiles(). + private ImmutableList flattenedOutputFileMap; + + private int numImplicitOutputKeys; Rule( Package pkg, @@ -245,30 +275,98 @@ public Attribute getAttributeDefinition(String attrName) { } /** - * Returns an (unmodifiable, ordered) collection containing all the declared output files of this - * rule. + * Constructs and returns an immutable list containing all the declared output files of this rule. * - *

All implicit output files (declared in the {@link RuleClass}) are - * listed first, followed by any explicit files (declared via the 'outs' attribute). Additionally - * both implicit and explicit outputs will retain the relative order in which they were declared. + *

There are two kinds of outputs. Explicit outputs are declared in attributes of type OUTPUT + * or OUTPUT_LABEL. Implicit outputs are determined by custom rule logic in an "implicit outputs + * function" (either defined natively or in Starlark), and are named following a template pattern + * based on the target's attributes. * - *

This ordering is useful because it is propagated through to the list of targets returned by - * getOuts() and allows targets to access their implicit outputs easily via - * {@code getOuts().get(N)} (providing that N is less than the number of implicit outputs). + *

All implicit output files (declared in the {@link RuleClass}) are listed first, followed by + * any explicit files (declared via output attributes). Additionally, both implicit and explicit + * outputs will retain the relative order in which they were declared. + */ + public ImmutableList getOutputFiles() { + // Discard the String keys, taking only the OutputFile values. + ImmutableList.Builder result = ImmutableList.builder(); + for (Object o : flattenedOutputFileMap) { + if (o instanceof OutputFile) { + result.add((OutputFile) o); + } + } + return result.build(); + } + + /** + * Constructs and returns an immutable list of all the implicit output files of this rule, in the + * order they were declared. + */ + public ImmutableList getImplicitOutputFiles() { + ImmutableList.Builder result = ImmutableList.builder(); + int seenKeys = 0; + for (Object o : flattenedOutputFileMap) { + if (o instanceof String) { + if (++seenKeys > numImplicitOutputKeys) { + break; + } + } else { + result.add((OutputFile) o); + } + } + return result.build(); + } + + /** + * Constructs and returns an immutable multimap of the explicit outputs, from attribute name to + * associated value. * - *

The fact that the relative order of the explicit outputs is also retained is less obviously - * useful but is still well defined. + *

Keys are listed in the same order as attributes. Order of attribute values (outputs in an + * output list) is preserved. + * + *

Since this is a multimap, attributes that have no associated outputs are omitted from the + * result. */ - public Collection getOutputFiles() { - return outputFiles; + public ImmutableListMultimap getExplicitOutputFileMap() { + ImmutableListMultimap.Builder result = ImmutableListMultimap.builder(); + int seenKeys = 0; + String key = null; + for (Object o : flattenedOutputFileMap) { + if (o instanceof String) { + seenKeys++; + key = (String) o; + } else if (seenKeys > numImplicitOutputKeys) { + result.put(key, (OutputFile) o); + } + } + return result.build(); } /** - * Returns an (unmodifiable, ordered) map containing the list of output files for every - * output type attribute. + * Returns a map of the Starlark-defined implicit outputs, from dict key to output file. + * + *

If there is no implicit outputs function, or it is a native one, an empty map is returned. + * + *

This is not a multimap because Starlark-defined implicit output functions return exactly one + * output per key. */ - public ListMultimap getOutputFileMap() { - return outputFileMap; + public ImmutableMap getStarlarkImplicitOutputFileMap() { + if (!(implicitOutputsFunction instanceof StarlarkImplicitOutputsFunction)) { + return ImmutableMap.of(); + } + ImmutableMap.Builder result = ImmutableMap.builder(); + int seenKeys = 0; + String key = null; + for (Object o : flattenedOutputFileMap) { + if (o instanceof String) { + if (++seenKeys > numImplicitOutputKeys) { + break; + } + key = (String) o; + } else { + result.put(key, (OutputFile) o); + } + } + return result.build(); } @Override @@ -553,151 +651,171 @@ void checkValidityPredicate(EventHandler eventHandler) { } /** - * Collects the output files (both implicit and explicit). All the implicit output files are added - * first, followed by any explicit files. Additionally both implicit and explicit output files - * will retain the relative order in which they were declared. + * Collects the output files (both implicit and explicit). Must be called before the output + * accessors methods can be used, and must be called only once. */ void populateOutputFiles(EventHandler eventHandler, Package.Builder pkgBuilder) throws LabelSyntaxException, InterruptedException { - populateOutputFilesInternal(eventHandler, pkgBuilder, /*performChecks=*/ true); + populateOutputFilesInternal( + eventHandler, pkgBuilder.getPackageIdentifier(), /*checkLabels=*/ true); } void populateOutputFilesUnchecked(EventHandler eventHandler, Package.Builder pkgBuilder) throws InterruptedException { try { - populateOutputFilesInternal(eventHandler, pkgBuilder, /*performChecks=*/ false); + populateOutputFilesInternal( + eventHandler, pkgBuilder.getPackageIdentifier(), /*checkLabels=*/ false); } catch (LabelSyntaxException e) { throw new IllegalStateException(e); } } + @FunctionalInterface + private interface ExplicitOutputHandler { + public void accept(Attribute attribute, Label outputLabel) throws LabelSyntaxException; + } + + @FunctionalInterface + private interface ImplicitOutputHandler { + public void accept(String outputKey, String outputName); + } + private void populateOutputFilesInternal( - EventHandler eventHandler, Package.Builder pkgBuilder, boolean performChecks) + EventHandler eventHandler, PackageIdentifier pkgId, boolean checkLabels) throws LabelSyntaxException, InterruptedException { - Preconditions.checkState(outputFiles == null); - // Order is important here: implicit before explicit - ImmutableList.Builder outputFilesBuilder = ImmutableList.builder(); - ImmutableListMultimap.Builder outputFileMapBuilder = + Preconditions.checkState(flattenedOutputFileMap == null); + + // We associate each output with its String key (or empty string if there's no key) as we go, + // and compress it down to a flat list afterwards. We use ImmutableListMultimap because it's + // more efficient than LinkedListMultimap and provides ordering guarantees among keys (whereas + // ArrayListMultimap doesn't). + ImmutableListMultimap.Builder outputFileMap = ImmutableListMultimap.builder(); - populateImplicitOutputFiles(eventHandler, pkgBuilder, outputFilesBuilder, performChecks); - populateExplicitOutputFiles( - eventHandler, outputFilesBuilder, outputFileMapBuilder, performChecks); - outputFiles = outputFilesBuilder.build(); - outputFileMap = outputFileMapBuilder.build(); - } - - // Explicit output files are user-specified attributes of type OUTPUT. - private void populateExplicitOutputFiles( - EventHandler eventHandler, - ImmutableList.Builder outputFilesBuilder, - ImmutableListMultimap.Builder outputFileMapBuilder, - boolean performChecks) - throws LabelSyntaxException { + // Detects collisions where the same output key is used for both an explicit and implicit entry. + HashSet implicitOutputKeys = new HashSet<>(); + + // We need the implicits to appear before the explicits in the final data structure, so we + // process them first. We check for duplicates while handling the explicits. + // + // Each of these cases has two subcases, so we factor their bodies out into lambdas. + + ImplicitOutputHandler implicitOutputHandler = + // outputKey: associated dict key if Starlark-defined, empty string otherwise + // outputName: package-relative path fragment + (outputKey, outputName) -> { + Label label; + if (checkLabels) { // controls label syntax validation only + try { + label = Label.create(pkgId, outputName); + } catch (LabelSyntaxException e) { + reportError( + String.format( + "illegal output file name '%s' in rule %s due to: %s", + outputName, getLabel(), e.getMessage()), + eventHandler); + return; + } + } else { + label = Label.createUnvalidated(pkgId, outputName); + } + validateOutputLabel(label, eventHandler); + + OutputFile file = new OutputFile(label, this); + outputFileMap.put(outputKey, file); + implicitOutputKeys.add(outputKey); + }; + + // Populate the implicit outputs. + try { + RawAttributeMapper attributeMap = RawAttributeMapper.of(this); + // TODO(bazel-team): Reconsider the ImplicitOutputsFunction abstraction. It doesn't seem to be + // a good fit if it forces us to downcast in situations like this. It also causes + // getImplicitOutputs() to declare that it throws EvalException (which then has to be + // explicitly disclaimed by the subclass SafeImplicitOutputsFunction). + if (implicitOutputsFunction instanceof StarlarkImplicitOutputsFunction) { + for (Map.Entry e : + ((StarlarkImplicitOutputsFunction) implicitOutputsFunction) + .calculateOutputs(eventHandler, attributeMap) + .entrySet()) { + implicitOutputHandler.accept(e.getKey(), e.getValue()); + } + } else { + for (String out : implicitOutputsFunction.getImplicitOutputs(eventHandler, attributeMap)) { + implicitOutputHandler.accept(/*outputKey=*/ "", out); + } + } + } catch (EvalException e) { + reportError( + String.format("In rule %s: %s", getLabel(), e.getMessageWithStack()), eventHandler); + } + + ExplicitOutputHandler explicitOutputHandler = + (attribute, outputLabel) -> { + String attrName = attribute.getName(); + if (implicitOutputKeys.contains(attrName)) { + reportError( + String.format( + "Implicit output key '%s' collides with output attribute name", attrName), + eventHandler); + } + if (checkLabels) { + if (!outputLabel.getPackageIdentifier().equals(pkg.getPackageIdentifier())) { + throw new IllegalStateException( + String.format( + "Label for attribute %s should refer to '%s' but instead refers to '%s'" + + " (label '%s')", + attribute, + pkg.getName(), + outputLabel.getPackageFragment(), + outputLabel.getName())); + } + if (outputLabel.getName().equals(".")) { + throw new LabelSyntaxException("output file name can't be equal '.'"); + } + } + validateOutputLabel(outputLabel, eventHandler); + + OutputFile outputFile = new OutputFile(outputLabel, this); + outputFileMap.put(attrName, outputFile); + }; + + // Populate the explicit outputs. NonconfigurableAttributeMapper nonConfigurableAttributes = NonconfigurableAttributeMapper.of(this); for (Attribute attribute : ruleClass.getAttributes()) { String name = attribute.getName(); Type type = attribute.getType(); if (type == BuildType.OUTPUT) { - Label outputLabel = nonConfigurableAttributes.get(name, BuildType.OUTPUT); - if (outputLabel != null) { - addLabelOutput( - attribute, - outputLabel, - eventHandler, - outputFilesBuilder, - outputFileMapBuilder, - performChecks); + Label label = nonConfigurableAttributes.get(name, BuildType.OUTPUT); + if (label != null) { + explicitOutputHandler.accept(attribute, label); } } else if (type == BuildType.OUTPUT_LIST) { for (Label label : nonConfigurableAttributes.get(name, BuildType.OUTPUT_LIST)) { - addLabelOutput( - attribute, - label, - eventHandler, - outputFilesBuilder, - outputFileMapBuilder, - performChecks); + explicitOutputHandler.accept(attribute, label); } } } - } - /** - * Implicit output files come from rule-specific patterns, and are a function of the rule's - * "name", "srcs", and other attributes. - */ - private void populateImplicitOutputFiles( - EventHandler eventHandler, - Package.Builder pkgBuilder, - ImmutableList.Builder outputFilesBuilder, - boolean performChecks) - throws InterruptedException { - try { - RawAttributeMapper attributeMap = RawAttributeMapper.of(this); - for (String out : implicitOutputsFunction.getImplicitOutputs(eventHandler, attributeMap)) { - Label label; - if (performChecks) { - try { - label = pkgBuilder.createLabel(out); - } catch (LabelSyntaxException e) { - reportError( - "illegal output file name '" - + out - + "' in rule " - + getLabel() - + " due to: " - + e.getMessage(), - eventHandler); - continue; - } - } else { - label = Label.createUnvalidated(pkgBuilder.getPackageIdentifier(), out); - } - addOutputFile(label, eventHandler, outputFilesBuilder); + // Flatten the result into the final list. + ImmutableList.Builder builder = ImmutableList.builder(); + for (Map.Entry> e : outputFileMap.build().asMap().entrySet()) { + builder.add(e.getKey()); + for (OutputFile out : e.getValue()) { + builder.add(out); } - } catch (EvalException e) { - reportError( - String.format("In rule %s: %s", getLabel(), e.getMessageWithStack()), eventHandler); } + flattenedOutputFileMap = builder.build(); + numImplicitOutputKeys = implicitOutputKeys.size(); } - private void addLabelOutput( - Attribute attribute, - Label label, - EventHandler eventHandler, - ImmutableList.Builder outputFilesBuilder, - ImmutableListMultimap.Builder outputFileMapBuilder, - boolean performChecks) - throws LabelSyntaxException { - if (performChecks) { - if (!label.getPackageIdentifier().equals(pkg.getPackageIdentifier())) { - throw new IllegalStateException("Label for attribute " + attribute - + " should refer to '" + pkg.getName() - + "' but instead refers to '" + label.getPackageFragment() - + "' (label '" + label.getName() + "')"); - } - if (label.getName().equals(".")) { - throw new LabelSyntaxException("output file name can't be equal '.'"); - } - } - OutputFile outputFile = addOutputFile(label, eventHandler, outputFilesBuilder); - outputFileMapBuilder.put(attribute.getName(), outputFile); - } - - private OutputFile addOutputFile( - Label label, - EventHandler eventHandler, - ImmutableList.Builder outputFilesBuilder) { + private void validateOutputLabel(Label label, EventHandler eventHandler) { if (label.getName().equals(getName())) { // TODO(bazel-team): for now (23 Apr 2008) this is just a warning. After // June 1st we should make it an error. reportWarning("target '" + getName() + "' is both a rule and a file; please choose " + "another name for the rule", eventHandler); } - OutputFile outputFile = new OutputFile(label, this); - outputFilesBuilder.add(outputFile); - return outputFile; } void reportError(String message, EventHandler eventHandler) { @@ -709,11 +827,7 @@ private void reportWarning(String message, EventHandler eventHandler) { eventHandler.handle(Event.warn(location, message)); } - /** - * Returns a string of the form "cc_binary rule //foo:foo" - * - * @return a string of the form "cc_binary rule //foo:foo" - */ + /** Returns a string of the form "cc_binary rule //foo:foo" */ @Override public String toString() { return getRuleClass() + " rule " + getLabel(); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkAspectFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkAspectFactory.java index bb750e717e496e..0a01746c068d3e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkAspectFactory.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkAspectFactory.java @@ -50,14 +50,11 @@ public ConfiguredAspect create( throws InterruptedException, ActionConflictException { StarlarkRuleContext starlarkRuleContext = null; // TODO(adonovan): simplify use of try/finally here. + AspectDescriptor aspectDescriptor = + new AspectDescriptor(starlarkAspect.getAspectClass(), parameters); try { - AspectDescriptor aspectDescriptor = - new AspectDescriptor(starlarkAspect.getAspectClass(), parameters); try { starlarkRuleContext = new StarlarkRuleContext(ruleContext, aspectDescriptor); - } catch (EvalException e) { - ruleContext.ruleError(e.getMessageWithStack()); - return null; } catch (RuleErrorException e) { ruleContext.ruleError(e.getMessage()); return null; diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java index a90d0a313cb4d0..f27bf04a1f5c32 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java @@ -1346,7 +1346,7 @@ public void testRuleClassImplicitAndExplicitOutputNamesCollide() throws Exceptio checkError( "test/starlark", "cr", - "Multiple outputs with the same key: o", + "Implicit output key 'o' collides with output attribute name", "load('//test/starlark:extension.bzl', 'custom_rule')", "", "custom_rule(name = 'cr', o = [':bar.txt'])");