Stop "query --output=proto" crashing on label-dict aspect attributes.
It's unfortunately complicated to properly support these attributes (we need
to define the proto spec more finely). This change just reports a proper error
vs. crashing Blaze. See the new comments in ProtoOutputFormatter for details.
PiperOrigin-RevId: 299903363
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
index 4a90723..9ed0d21 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -30,6 +30,7 @@
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable;
import com.google.devtools.build.lib.events.Location;
+import com.google.devtools.build.lib.events.Reporter;
import com.google.devtools.build.lib.events.StoredEventHandler;
import com.google.devtools.build.lib.packages.Globber.BadGlobException;
import com.google.devtools.build.lib.packages.PackageValidator.InvalidPackageException;
@@ -136,6 +137,7 @@
protected Iterable<EnvironmentExtension> environmentExtensions = ImmutableList.of();
protected PackageValidator packageValidator = PackageValidator.NOOP_VALIDATOR;
protected boolean doChecksForTesting = true;
+ protected Reporter reporter;
public BuilderForTesting setEnvironmentExtensions(
Iterable<EnvironmentExtension> environmentExtensions) {
@@ -153,6 +155,11 @@
return this;
}
+ public BuilderForTesting setReporter(Reporter reporter) {
+ this.reporter = reporter;
+ return this;
+ }
+
public abstract PackageFactory build(RuleClassProvider ruleClassProvider, FileSystem fs);
}
diff --git a/src/main/java/com/google/devtools/build/lib/query2/engine/OutputFormatterCallback.java b/src/main/java/com/google/devtools/build/lib/query2/engine/OutputFormatterCallback.java
index 50708d6..c66eefe 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/engine/OutputFormatterCallback.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/engine/OutputFormatterCallback.java
@@ -67,11 +67,11 @@
* Use an {@code OutputFormatterCallback} with an already computed set of targets. Note that this
* does not work in stream mode, as the {@code targets} would already be computed.
*
- * <p>The intended usage of this method is to use {@code StreamedFormatter} formaters in non
+ * <p>The intended usage of this method is to use {@code StreamedFormatter} formatters in non
* streaming contexts.
*/
- public static <T> void processAllTargets(OutputFormatterCallback<T> callback,
- Iterable<T> targets) throws IOException, InterruptedException {
+ public static <T> void processAllTargets(OutputFormatterCallback<T> callback, Iterable<T> targets)
+ throws IOException, InterruptedException {
boolean failFast = true;
try {
callback.start();
diff --git a/src/main/java/com/google/devtools/build/lib/query2/query/output/AbstractUnorderedFormatter.java b/src/main/java/com/google/devtools/build/lib/query2/query/output/AbstractUnorderedFormatter.java
index 6a6917a..336849b 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/query/output/AbstractUnorderedFormatter.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/query/output/AbstractUnorderedFormatter.java
@@ -15,6 +15,7 @@
package com.google.devtools.build.lib.query2.query.output;
import com.google.common.collect.Iterables;
+import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.graph.Digraph;
import com.google.devtools.build.lib.graph.Node;
import com.google.devtools.build.lib.packages.Target;
@@ -30,11 +31,19 @@
@Override
public void setOptions(CommonQueryOptions options, AspectResolver aspectResolver) {}
+ /** Sets a handler for reporting status output / errors. */
+ public void setEventHandler(EventHandler eventHandler) {}
+
@Override
public void output(
- QueryOptions options, Digraph<Target> result, OutputStream out, AspectResolver aspectResolver)
+ QueryOptions options,
+ Digraph<Target> result,
+ OutputStream out,
+ AspectResolver aspectResolver,
+ EventHandler eventHandler)
throws IOException, InterruptedException {
setOptions(options, aspectResolver);
+ setEventHandler(eventHandler);
OutputFormatterCallback.processAllTargets(
createPostFactoStreamCallback(out, options), getOrderedTargets(result, options));
}
diff --git a/src/main/java/com/google/devtools/build/lib/query2/query/output/GraphOutputFormatter.java b/src/main/java/com/google/devtools/build/lib/query2/query/output/GraphOutputFormatter.java
index 7e9f377..3d71a0f 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/query/output/GraphOutputFormatter.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/query/output/GraphOutputFormatter.java
@@ -19,6 +19,7 @@
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.collect.CollectionUtils;
import com.google.devtools.build.lib.collect.EquivalenceRelation;
+import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.graph.Digraph;
import com.google.devtools.build.lib.graph.DotOutputVisitor;
import com.google.devtools.build.lib.graph.LabelSerializer;
@@ -58,7 +59,8 @@
QueryOptions options,
Digraph<Target> result,
OutputStream out,
- AspectResolver aspectProvider) {
+ AspectResolver aspectProvider,
+ EventHandler eventHandler) {
this.graphNodeStringLimit = options.graphNodeStringLimit;
this.graphConditionalEdgesLimit = options.graphConditionalEdgesLimit;
diff --git a/src/main/java/com/google/devtools/build/lib/query2/query/output/MaxrankOutputFormatter.java b/src/main/java/com/google/devtools/build/lib/query2/query/output/MaxrankOutputFormatter.java
index 83dba1a..777cc3d 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/query/output/MaxrankOutputFormatter.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/query/output/MaxrankOutputFormatter.java
@@ -16,6 +16,7 @@
import static java.util.Comparator.comparingInt;
+import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.graph.Digraph;
import com.google.devtools.build.lib.graph.Node;
import com.google.devtools.build.lib.packages.Target;
@@ -51,7 +52,11 @@
@Override
public void output(
- QueryOptions options, Digraph<Target> result, OutputStream out, AspectResolver aspectResolver)
+ QueryOptions options,
+ Digraph<Target> result,
+ OutputStream out,
+ AspectResolver aspectResolver,
+ EventHandler eventHandler)
throws IOException {
// In order to handle cycles correctly, we need work on the strong
// component graph, as cycles should be treated a "clump" of nodes all on
diff --git a/src/main/java/com/google/devtools/build/lib/query2/query/output/MinrankOutputFormatter.java b/src/main/java/com/google/devtools/build/lib/query2/query/output/MinrankOutputFormatter.java
index efc5cb6..8e020cd 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/query/output/MinrankOutputFormatter.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/query/output/MinrankOutputFormatter.java
@@ -15,6 +15,7 @@
package com.google.devtools.build.lib.query2.query.output;
import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.graph.Digraph;
import com.google.devtools.build.lib.graph.Node;
import com.google.devtools.build.lib.packages.Target;
@@ -63,7 +64,11 @@
@Override
public void output(
- QueryOptions options, Digraph<Target> result, OutputStream out, AspectResolver aspectResolver)
+ QueryOptions options,
+ Digraph<Target> result,
+ OutputStream out,
+ AspectResolver aspectResolver,
+ EventHandler eventHandler)
throws IOException {
PrintStream printStream = new PrintStream(out);
// getRoots() isn't defined for cyclic graphs, so in order to handle
diff --git a/src/main/java/com/google/devtools/build/lib/query2/query/output/OutputFormatter.java b/src/main/java/com/google/devtools/build/lib/query2/query/output/OutputFormatter.java
index 77770fc..a3e8314 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/query/output/OutputFormatter.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/query/output/OutputFormatter.java
@@ -13,6 +13,7 @@
// limitations under the License.
package com.google.devtools.build.lib.query2.query.output;
+import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.graph.Digraph;
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.query2.engine.QueryEnvironment;
@@ -54,6 +55,10 @@
* by the QueryEnvironment), and print it to "out".
*/
public abstract void output(
- QueryOptions options, Digraph<Target> result, OutputStream out, AspectResolver aspectProvider)
+ QueryOptions options,
+ Digraph<Target> result,
+ OutputStream out,
+ AspectResolver aspectProvider,
+ EventHandler eventHandler)
throws IOException, InterruptedException;
}
diff --git a/src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java b/src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java
index 23db2a6..e0b580b 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java
@@ -24,11 +24,14 @@
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.ImmutableMultimap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Maps;
import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.graph.Digraph;
import com.google.devtools.build.lib.graph.Node;
import com.google.devtools.build.lib.packages.AggregatingAttributeMapper;
@@ -98,6 +101,8 @@
private boolean includeLocations = true;
private boolean includeRuleInputsAndOutputs = true;
+ private EventHandler eventHandler;
+
@Override
public String getName() {
return "proto";
@@ -116,6 +121,11 @@
this.includeRuleInputsAndOutputs = options.protoIncludeRuleInputsAndOutputs;
}
+ @Override
+ public void setEventHandler(EventHandler eventHandler) {
+ this.eventHandler = eventHandler;
+ }
+
private static Predicate<String> newAttributePredicate(List<String> outputAttributes) {
if (outputAttributes.equals(ImmutableList.of("all"))) {
return Predicates.alwaysTrue();
@@ -198,7 +208,7 @@
if (!includeAspectAttribute(attribute, labels)) {
continue;
}
- Object attributeValue = getAspectAttributeValue(attribute, labels);
+ Object attributeValue = getAspectAttributeValue(target, attribute, labels);
Build.Attribute serializedAttribute =
AttributeFormatter.getAttributeProto(
attribute,
@@ -369,11 +379,29 @@
&& ruleAttributePredicate.apply(attr.getName());
}
- private static Object getAspectAttributeValue(Attribute attribute, Collection<Label> labels) {
+ private Object getAspectAttributeValue(
+ Target target, Attribute attribute, Collection<Label> labels) {
Type<?> attributeType = attribute.getType();
if (attributeType.equals(BuildType.LABEL)) {
Preconditions.checkState(labels.size() == 1, "attribute=%s, labels=%s", attribute, labels);
return Iterables.getOnlyElement(labels);
+ } else if (attributeType.equals(BuildType.LABEL_KEYED_STRING_DICT)) {
+ // Ideally we'd support LABEL_KEYED_STRING_DICT by getting the value directly from the aspect
+ // definition vs. trying to reverse-construct it from the flattened labels as this method
+ // does. Unfortunately any proper support surfaces a latent bug between --output=proto and
+ // aspect attributes: "{@code labels} isn't the set of labels for a single attribute value but
+ // for all values of all attributes with the same name. We can have multiple attributes with
+ // the same name because multiple aspects may attach to a rule, and nothing is stopping them
+ // from defining the same attribute names. That means the "Attribute" proto message doesn't
+ // really represent a single attribute, in spite of its documented purpose. This all calls for
+ // an API design upgrade to properly consider these relationships. Details at b/149982967.
+ eventHandler.handle(
+ Event.error(
+ String.format(
+ "Target \"%s\", aspect attribute \"%s\": type \"%s\" not yet supported with"
+ + " --output=proto. See b/149982967",
+ target.getLabel(), attribute.getName(), BuildType.LABEL_KEYED_STRING_DICT)));
+ return ImmutableMap.of();
} else {
Preconditions.checkState(
attributeType.equals(BuildType.LABEL_LIST),
diff --git a/src/main/java/com/google/devtools/build/lib/query2/query/output/QueryOutputUtils.java b/src/main/java/com/google/devtools/build/lib/query2/query/output/QueryOutputUtils.java
index 89bf13f..5e6506f 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/query/output/QueryOutputUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/query/output/QueryOutputUtils.java
@@ -13,6 +13,7 @@
// limitations under the License.
package com.google.devtools.build.lib.query2.query.output;
+import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.graph.Digraph;
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.profiler.Profiler;
@@ -37,9 +38,14 @@
&& formatter instanceof StreamedFormatter;
}
- public static void output(QueryOptions queryOptions, QueryEvalResult result,
- Set<Target> targetsResult, OutputFormatter formatter, OutputStream outputStream,
- AspectResolver aspectResolver)
+ public static void output(
+ QueryOptions queryOptions,
+ QueryEvalResult result,
+ Set<Target> targetsResult,
+ OutputFormatter formatter,
+ OutputStream outputStream,
+ AspectResolver aspectResolver,
+ EventHandler eventHandler)
throws IOException, InterruptedException {
/*
* This is not really streaming, but we are using the streaming interface for writing into the
@@ -63,7 +69,7 @@
}
try (SilentCloseable closeable = Profiler.instance().profile("formatter.output")) {
- formatter.output(queryOptions, subgraph, outputStream, aspectResolver);
+ formatter.output(queryOptions, subgraph, outputStream, aspectResolver, eventHandler);
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java b/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java
index fbbc6bb..252e1e2 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java
@@ -404,7 +404,8 @@
result,
formatter,
outputStream,
- queryOptions.aspectDeps.createResolver(packageProvider, getEventHandler(ruleContext)));
+ queryOptions.aspectDeps.createResolver(packageProvider, getEventHandler(ruleContext)),
+ getEventHandler(ruleContext));
outputStream.close();
} catch (ClosedByInterruptException e) {
throw new InterruptedException(e.getMessage());
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryCommand.java
index c0ced79..66964a7 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryCommand.java
@@ -158,7 +158,8 @@
targets,
formatter,
out,
- queryOptions.aspectDeps.createResolver(env.getPackageManager(), env.getReporter()));
+ queryOptions.aspectDeps.createResolver(env.getPackageManager(), env.getReporter()),
+ env.getReporter());
} catch (ClosedByInterruptException | InterruptedException e) {
env.getReporter().handle(Event.error("query interrupted"));
return Either.ofLeft(BlazeCommandResult.exitCode(ExitCode.INTERRUPTED));
diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/QueryIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/buildtool/QueryIntegrationTest.java
index f3840da..8d4952a 100644
--- a/src/test/java/com/google/devtools/build/lib/buildtool/QueryIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/buildtool/QueryIntegrationTest.java
@@ -303,13 +303,15 @@
ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
+ Reporter reporter = new Reporter(new EventBus(), events.collector());
QueryOutputUtils.output(
- queryOptions, result,
- callback.getResult(), formatter,
+ queryOptions,
+ result,
+ callback.getResult(),
+ formatter,
outputStream,
- queryOptions.aspectDeps.createResolver(
- env.getPackageManager(),
- new Reporter(new EventBus(), events.collector())));
+ queryOptions.aspectDeps.createResolver(env.getPackageManager(), reporter),
+ reporter);
return outputStream.toByteArray();
}
diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/PackageLoadingTestCase.java b/src/test/java/com/google/devtools/build/lib/packages/util/PackageLoadingTestCase.java
index 7fea1b1..56e9db9 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/util/PackageLoadingTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/util/PackageLoadingTestCase.java
@@ -100,6 +100,7 @@
loadingMock
.getPackageFactoryBuilderForTesting(directories)
.setEnvironmentExtensions(getEnvironmentExtensions())
+ .setReporter(reporter)
.build(ruleClassProvider, fileSystem);
skyframeExecutor = createSkyframeExecutor();
setUpSkyframe();
