Automated rollback of commit 78fdb7d62da15f7048cc3c170ca4fc03ff8f9bb2.
*** Reason for rollback ***
Throwing a NPE in users blaze integration tests
*** Original change description ***
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: 300096191
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 9ed0d21..4a90723 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,7 +30,6 @@
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;
@@ -137,7 +136,6 @@
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) {
@@ -155,11 +153,6 @@
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 c66eefe..50708d6 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} formatters in non
+ * <p>The intended usage of this method is to use {@code StreamedFormatter} formaters 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 336849b..6a6917a 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,7 +15,6 @@
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;
@@ -31,19 +30,11 @@
@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,
- EventHandler eventHandler)
+ QueryOptions options, Digraph<Target> result, OutputStream out, AspectResolver aspectResolver)
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 3d71a0f..7e9f377 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,7 +19,6 @@
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;
@@ -59,8 +58,7 @@
QueryOptions options,
Digraph<Target> result,
OutputStream out,
- AspectResolver aspectProvider,
- EventHandler eventHandler) {
+ AspectResolver aspectProvider) {
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 777cc3d..83dba1a 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,7 +16,6 @@
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;
@@ -52,11 +51,7 @@
@Override
public void output(
- QueryOptions options,
- Digraph<Target> result,
- OutputStream out,
- AspectResolver aspectResolver,
- EventHandler eventHandler)
+ QueryOptions options, Digraph<Target> result, OutputStream out, AspectResolver aspectResolver)
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 8e020cd..efc5cb6 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,7 +15,6 @@
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;
@@ -64,11 +63,7 @@
@Override
public void output(
- QueryOptions options,
- Digraph<Target> result,
- OutputStream out,
- AspectResolver aspectResolver,
- EventHandler eventHandler)
+ QueryOptions options, Digraph<Target> result, OutputStream out, AspectResolver aspectResolver)
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 a3e8314..77770fc 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,7 +13,6 @@
// 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;
@@ -55,10 +54,6 @@
* by the QueryEnvironment), and print it to "out".
*/
public abstract void output(
- QueryOptions options,
- Digraph<Target> result,
- OutputStream out,
- AspectResolver aspectProvider,
- EventHandler eventHandler)
+ QueryOptions options, Digraph<Target> result, OutputStream out, AspectResolver aspectProvider)
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 e0b580b..23db2a6 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,14 +24,11 @@
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;
@@ -101,8 +98,6 @@
private boolean includeLocations = true;
private boolean includeRuleInputsAndOutputs = true;
- private EventHandler eventHandler;
-
@Override
public String getName() {
return "proto";
@@ -121,11 +116,6 @@
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();
@@ -208,7 +198,7 @@
if (!includeAspectAttribute(attribute, labels)) {
continue;
}
- Object attributeValue = getAspectAttributeValue(target, attribute, labels);
+ Object attributeValue = getAspectAttributeValue(attribute, labels);
Build.Attribute serializedAttribute =
AttributeFormatter.getAttributeProto(
attribute,
@@ -379,29 +369,11 @@
&& ruleAttributePredicate.apply(attr.getName());
}
- private Object getAspectAttributeValue(
- Target target, Attribute attribute, Collection<Label> labels) {
+ private static Object getAspectAttributeValue(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 5e6506f..89bf13f 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,7 +13,6 @@
// 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;
@@ -38,14 +37,9 @@
&& formatter instanceof StreamedFormatter;
}
- public static void output(
- QueryOptions queryOptions,
- QueryEvalResult result,
- Set<Target> targetsResult,
- OutputFormatter formatter,
- OutputStream outputStream,
- AspectResolver aspectResolver,
- EventHandler eventHandler)
+ public static void output(QueryOptions queryOptions, QueryEvalResult result,
+ Set<Target> targetsResult, OutputFormatter formatter, OutputStream outputStream,
+ AspectResolver aspectResolver)
throws IOException, InterruptedException {
/*
* This is not really streaming, but we are using the streaming interface for writing into the
@@ -69,7 +63,7 @@
}
try (SilentCloseable closeable = Profiler.instance().profile("formatter.output")) {
- formatter.output(queryOptions, subgraph, outputStream, aspectResolver, eventHandler);
+ formatter.output(queryOptions, subgraph, outputStream, aspectResolver);
}
}
}
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 252e1e2..fbbc6bb 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,8 +404,7 @@
result,
formatter,
outputStream,
- queryOptions.aspectDeps.createResolver(packageProvider, getEventHandler(ruleContext)),
- getEventHandler(ruleContext));
+ queryOptions.aspectDeps.createResolver(packageProvider, 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 66964a7..c0ced79 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,8 +158,7 @@
targets,
formatter,
out,
- queryOptions.aspectDeps.createResolver(env.getPackageManager(), env.getReporter()),
- env.getReporter());
+ queryOptions.aspectDeps.createResolver(env.getPackageManager(), 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 8d4952a..f3840da 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,15 +303,13 @@
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(), reporter),
- reporter);
+ queryOptions.aspectDeps.createResolver(
+ env.getPackageManager(),
+ new Reporter(new EventBus(), events.collector())));
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 56e9db9..7fea1b1 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,7 +100,6 @@
loadingMock
.getPackageFactoryBuilderForTesting(directories)
.setEnvironmentExtensions(getEnvironmentExtensions())
- .setReporter(reporter)
.build(ruleClassProvider, fileSystem);
skyframeExecutor = createSkyframeExecutor();
setUpSkyframe();
