Roll forward of https://github.com/bazelbuild/bazel/commit/78fdb7d62d
The original change broke some call paths that didn't have an event reporter to
provide. It's also a bit fragile in relying on AbstractUnorderedFormatter.setEventHandler to provide the reporter - there are different call sites and if we forget to call that method from one of them we'd get a null pointer exception.
This version simply makes the reporting optional: if the reporter is available the output formatter reports to it. Else it skips it.
Ideally we'd do a deeper audit of all call sites and guarantee the existence of a reporter in all expected places. That's a lot of refactoring, and in some disparate pieces of code that serve varying purposes. So it's unclear what assumptions are correct in what place.
This approach, while weaker, at least avoids the crash and provides a clear reason in *some* use cases. We could presumably expand such qualifying use cases incrementally as needed. In the meantime those use cases will simply return misleading data. :(
PiperOrigin-RevId: 300832332
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..b00ad6a 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;
@@ -24,17 +25,27 @@
import com.google.devtools.build.lib.query2.query.output.QueryOptions.OrderOutput;
import java.io.IOException;
import java.io.OutputStream;
+import javax.annotation.Nullable;
abstract class AbstractUnorderedFormatter extends OutputFormatter implements StreamedFormatter {
@Override
public void setOptions(CommonQueryOptions options, AspectResolver aspectResolver) {}
+ /** Optionally sets a handler for reporting status output / errors. */
+ @Override
+ public void setEventHandler(@Nullable EventHandler eventHandler) {}
+
@Override
public void output(
- QueryOptions options, Digraph<Target> result, OutputStream out, AspectResolver aspectResolver)
+ QueryOptions options,
+ Digraph<Target> result,
+ OutputStream out,
+ AspectResolver aspectResolver,
+ @Nullable EventHandler eventHandler)
throws IOException, InterruptedException {
setOptions(options, aspectResolver);
+ setEventHandler(eventHandler);
OutputFormatterCallback.processAllTargets(
createPostFactoStreamCallback(out, options), getOrderedTargets(result, options));
}
@@ -46,4 +57,4 @@
: result.getTopologicalOrder(new FormatUtils.TargetOrdering());
return Iterables.transform(orderedResult, Node::getLabel);
}
-}
\ No newline at end of file
+}
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..da5c252 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;
@@ -22,6 +23,7 @@
import java.io.IOException;
import java.io.OutputStream;
import java.io.Serializable;
+import javax.annotation.Nullable;
/**
* Interface for classes which order, format and print the result of a Blaze
@@ -54,6 +56,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,
+ @Nullable 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 0a78721..5b4387a 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;
@@ -100,6 +103,8 @@
private boolean includeRuleInputsAndOutputs = true;
private boolean includeSyntheticAttributeHash = false;
+ @Nullable private EventHandler eventHandler;
+
@Override
public String getName() {
return "proto";
@@ -119,6 +124,11 @@
this.includeSyntheticAttributeHash = options.protoIncludeSyntheticAttributeHash;
}
+ @Override
+ public void setEventHandler(@Nullable EventHandler eventHandler) {
+ this.eventHandler = eventHandler;
+ }
+
private static Predicate<String> newAttributePredicate(List<String> outputAttributes) {
if (outputAttributes.equals(ImmutableList.of("all"))) {
return Predicates.alwaysTrue();
@@ -201,7 +211,7 @@
if (!includeAspectAttribute(attribute, labels)) {
continue;
}
- Object attributeValue = getAspectAttributeValue(attribute, labels);
+ Object attributeValue = getAspectAttributeValue(target, attribute, labels);
Build.Attribute serializedAttribute =
AttributeFormatter.getAttributeProto(
attribute,
@@ -380,11 +390,35 @@
&& 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.
+ if (eventHandler != null) {
+ eventHandler.handle(
+ Event.error(
+ String.format(
+ "Target \"%s\", aspect attribute \"%s\": type \"%s\" not yet supported with"
+ + " --output=proto.",
+ target.getLabel(), attribute.getName(), BuildType.LABEL_KEYED_STRING_DICT)));
+ }
+ // This return value is misleading when the above error isn't get triggered: it implies an
+ // empty result with no signal that that result isn't accurate.
+ // TODO(bazel-team): either make the result accurate or trigger an error universally. Letting
+ // OutputFormatter.output() throw a QueryException is a promising approach.
+ 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..e638a0d 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;
@@ -25,6 +26,7 @@
import java.io.IOException;
import java.io.OutputStream;
import java.util.Set;
+import javax.annotation.Nullable;
/** Static utility methods for outputting a query. */
public class QueryOutputUtils {
@@ -37,9 +39,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,
+ @Nullable EventHandler eventHandler)
throws IOException, InterruptedException {
/*
* This is not really streaming, but we are using the streaming interface for writing into the
@@ -49,6 +56,7 @@
if (shouldStreamResults(queryOptions, formatter)) {
StreamedFormatter streamedFormatter = (StreamedFormatter) formatter;
streamedFormatter.setOptions(queryOptions, aspectResolver);
+ streamedFormatter.setEventHandler(eventHandler);
OutputFormatterCallback.processAllTargets(
streamedFormatter.createPostFactoStreamCallback(outputStream, queryOptions),
targetsResult);
@@ -63,7 +71,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/query2/query/output/StreamedFormatter.java b/src/main/java/com/google/devtools/build/lib/query2/query/output/StreamedFormatter.java
index bbe9697..44f951f 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/query/output/StreamedFormatter.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/query/output/StreamedFormatter.java
@@ -14,6 +14,7 @@
package com.google.devtools.build.lib.query2.query.output;
+import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.query2.common.CommonQueryOptions;
import com.google.devtools.build.lib.query2.engine.OutputFormatterCallback;
@@ -21,6 +22,7 @@
import com.google.devtools.build.lib.query2.engine.ThreadSafeOutputFormatterCallback;
import com.google.devtools.build.lib.query2.query.aspectresolvers.AspectResolver;
import java.io.OutputStream;
+import javax.annotation.Nullable;
/**
* Unordered streamed output formatter (wrt. dependency ordering).
@@ -37,6 +39,9 @@
/** Specifies options to be used by subsequent calls to {@link #createStreamCallback}. */
void setOptions(CommonQueryOptions options, AspectResolver aspectResolver);
+ /** Sets an optional handler for reporting status output / errors. */
+ void setEventHandler(@Nullable EventHandler eventHandler);
+
/**
* Returns a {@link ThreadSafeOutputFormatterCallback} whose
* {@link OutputFormatterCallback#process} outputs formatted {@link Target}s to the given
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..19f3151 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
@@ -104,6 +104,7 @@
streamedFormatter.setOptions(
queryOptions,
queryOptions.aspectDeps.createResolver(env.getPackageManager(), env.getReporter()));
+ streamedFormatter.setEventHandler(env.getReporter());
callback = streamedFormatter.createStreamCallback(out, queryOptions, queryEnv);
} else {
callback = QueryUtil.newOrderedAggregateAllOutputFormatterCallback(queryEnv);
@@ -158,7 +159,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();
}
