Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement cquery --output=graph #12248

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,23 @@ public class CommonQueryOptions extends OptionsBase {
+ " with top-level options.")
public List<String> universeScope;

@Option(
name = "line_terminator_null",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.QUERY,
effectTags = {OptionEffectTag.TERMINAL_OUTPUT},
help = "Whether each format is terminated with \\0 instead of newline.")
public boolean lineTerminatorNull;

/** Ugly workaround since line terminator option default has to be constant expression. */
public String getLineTerminator() {
if (lineTerminatorNull) {
return "\0";
}

return System.lineSeparator();
}

@Option(
name = "infer_universe_scope",
defaultValue = "false",
Expand Down Expand Up @@ -253,4 +270,30 @@ public AspectResolutionModeConverter() {
+ "precise mode is not completely precise: the decision whether to compute an aspect "
+ "is decided in the analysis phase, which is not run during 'bazel query'.")
public AspectResolver.Mode aspectDeps;

///////////////////////////////////////////////////////////
// GRAPH OUTPUT FORMATTER OPTIONS //
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the proto: options above also graph output formatter options?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not to my knowledge? As far as I can see those only apply to --output=proto

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I didn't quite read this as --output=graph output formatter options. makes sense.

///////////////////////////////////////////////////////////

@Option(
name = "graph:node_limit",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I would interpret node_limit as a limit to the number of nodes in the graph, but it's not the case here. Maybe node_string_limit? It's consistent with the variable name on L287.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This carries over query's --graph:node_limit flag into a common location. However clear or not the flag is, I'd prefer to keep the existing API for the context of this change.

defaultValue = "512",
documentationCategory = OptionDocumentationCategory.QUERY,
effectTags = {OptionEffectTag.TERMINAL_OUTPUT},
help =
"The maximum length of the label string for a graph node in the output. Longer labels"
+ " will be truncated; -1 means no truncation. This option is only applicable to"
+ " --output=graph.")
public int graphNodeStringLimit;

@Option(
name = "graph:factored",
defaultValue = "true",
documentationCategory = OptionDocumentationCategory.QUERY,
effectTags = {OptionEffectTag.TERMINAL_OUTPUT},
help =
"If true, then the graph will be emitted 'factored', i.e. topologically-equivalent nodes "
+ "will be merged together and their labels concatenated. This option is only "
+ "applicable to --output=graph.")
public boolean graphFactored;
}
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,13 @@ private static ImmutableMap<String, BuildConfiguration> getTransitiveConfigurati
OutputType.JSON),
new BuildOutputFormatterCallback(
eventHandler, cqueryOptions, out, skyframeExecutor, accessor),
new GraphOutputFormatterCallback(
gregestren marked this conversation as resolved.
Show resolved Hide resolved
eventHandler,
cqueryOptions,
out,
skyframeExecutor,
accessor,
ct -> getFwdDeps(ImmutableList.of(ct))),
new StarlarkOutputFormatterCallback(
eventHandler, cqueryOptions, out, skyframeExecutor, accessor));
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
// Copyright 2020 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.query2.cquery;

import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.graph.Digraph;
import com.google.devtools.build.lib.graph.Node;
import com.google.devtools.build.lib.query2.engine.QueryEnvironment.TargetAccessor;
import com.google.devtools.build.lib.query2.query.output.GraphOutputWriter;
import com.google.devtools.build.lib.query2.query.output.GraphOutputWriter.NodeReader;
import com.google.devtools.build.lib.skyframe.SkyframeExecutor;
import java.io.OutputStream;
import java.util.Comparator;

/** cquery output formatter that prints the result as factored graph in AT&amp;T GraphViz format. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: just AT&T?

Copy link
Contributor Author

@gregestren gregestren Oct 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This inherits the same comment as in the query version, which I believe follows Javadoc's "Comments are written in HTML" guidance: https://docs.oracle.com/javase/1.5.0/docs/tooldocs/windows/javadoc.html#blockandinlinetags

Key snippet:

entities for the less-than (<) and greater-than (>) symbols should be written &lt; and &gt;. Likewise, the ampersand (&) should be written &amp;

class GraphOutputFormatterCallback extends CqueryThreadsafeCallback {
@Override
public String getName() {
return "graph";
}

/** Interface for finding a configured target's direct dependencies. */
@FunctionalInterface
public interface DepsRetriever {
Iterable<ConfiguredTarget> getDirectDeps(ConfiguredTarget taget) throws InterruptedException;
}

private final DepsRetriever depsRetriever;

private final GraphOutputWriter.NodeReader<ConfiguredTarget> nodeReader =
new NodeReader<ConfiguredTarget>() {

private final Comparator<ConfiguredTarget> configuredTargetOrdering =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my own edification - reason to initialize this outside of the comparator method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My original reasoning is that the comparator method can be called multiple times whereas the actual comparator logic only needs to be defined once. So it theoretically saves unnecessary extra instantiation.

In practice I don't think that'd make a huge difference (and I'd hope the JDK could optimize that). So I don't have strong feelings on the subject.

(ct1, ct2) -> {
// Order graph output first by target label, then by configuration hash.
Label label1 = ct1.getLabel();
Label label2 = ct2.getLabel();
return label1.equals(label2)
? ct1.getConfigurationChecksum().compareTo(ct2.getConfigurationChecksum())
: label1.compareTo(label2);
};

@Override
public String getLabel(Node<ConfiguredTarget> node) {
// Node payloads are ConfiguredTargets. Output node labels are target labels + config
// hashes.
ConfiguredTarget ct = node.getLabel();
return String.format(
"%s (%s)",
ct.getLabel(),
shortId(skyframeExecutor.getConfiguration(eventHandler, ct.getConfigurationKey())));
gregestren marked this conversation as resolved.
Show resolved Hide resolved
}

@Override
public Comparator<ConfiguredTarget> comparator() {
return configuredTargetOrdering;
}
};

GraphOutputFormatterCallback(
ExtendedEventHandler eventHandler,
CqueryOptions options,
OutputStream out,
SkyframeExecutor skyframeExecutor,
TargetAccessor<ConfiguredTarget> accessor,
DepsRetriever depsRetriever) {
super(eventHandler, options, out, skyframeExecutor, accessor);
this.depsRetriever = depsRetriever;
}

@Override
public void processOutput(Iterable<ConfiguredTarget> partialResult) throws InterruptedException {
// Transform the cquery-backed graph into a Digraph to make it suitable for GraphOutputWriter.
// Note that this involves an extra iteration over the entire query result subgraph. We could
// conceptually merge transformation and output writing into the same iteration if needed.
Digraph<ConfiguredTarget> graph = new Digraph<>();
ImmutableSet<ConfiguredTarget> allNodes = ImmutableSet.copyOf(partialResult);
for (ConfiguredTarget configuredTarget : partialResult) {
Node<ConfiguredTarget> node = graph.createNode(configuredTarget);
for (ConfiguredTarget dep : depsRetriever.getDirectDeps(configuredTarget)) {
if (allNodes.contains(dep)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming this is for a situation like --noimplicit_deps or the like?

Copy link
Contributor Author

@gregestren gregestren Oct 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reasoning here is that the query output may only contain a subset of all deps.

So if A depends on B, C, and D and some magical query expression filtered the results down to just A and C, depsRetriever.getDirectDeps returns all of A's deps (B, C, and D). We only want to include the ones that are part of the query result (any target in partialResult, which in this case is A and C).

Node<ConfiguredTarget> depNode = graph.createNode(dep);
graph.addEdge(node, depNode);
}
}
}

GraphOutputWriter<ConfiguredTarget> graphWriter =
new GraphOutputWriter<>(
nodeReader,
options.getLineTerminator(),
/*sortLabels=*/ true,
options.graphNodeStringLimit,
// select() conditions don't matter for cquery because cquery operates post-analysis
// phase, when select()s have been resolved and removed from the graph.
/*maxConditionalEdges=*/ 0,
options.graphFactored);
graphWriter.write(graph, /*conditionalEdges=*/ null, outputStream);
}
}
Loading