Implement cquery --output=graph #12248

gregestren · 2020-10-09T23:04:00Z

Thankfully, query already has most of the infrastructure necessary to make this
easy.

query implements graph output (in GraphOutputFormatter) over a
Digraph<Target>, which is a generic graph data structure with Target payloads.
All output logic then runs over this data structure. To opt query in, all we have
to do is create an equivalent Digraph<ConfiguredTarget>, which is a simple
transformation from the backing graph.

This change creates a new generic class for that common logic:
GraphOutputWriter. query's GraphOutputFormatter then becomes a simple wrapper
over that, and the new GraphOutputFormatterCallback is cquery's equivalent.

A few differences:

cquery output is always fully ordered (--order_output=full). We could match
this with query's controllable version, but I don't see a reason to make this
yet another bit to configured.
query output annotates edges with select() conditions. cquery doesn't do this
because select()s are resolved and removed from the graph after analysis. I
think we could annotate edges with the chosen condition if there was
demand, but that'd be a followup effort.

Fixes #10843 (for cquery, not aquery)

Thankfully, query already has most of the infrastructure necessary to make this easy. query implements graph output (in GraphOutputFormatter) over a Digraph<Target>, which is a generic graph data structure with Target payloads. All output logic then runs over this data structure. To opt query in, all we have to do is create an equivalent Digraph<ConfiguredTarget>, which is a simple transformation from the backing graph. This change creates a new generic class for that common logic: GraphOutputWriter. query's GraphOutputFormatter then becomes a simple wrapper over that, and the new GraphOutputFormatterCallback is cquery's equivalent. A few differences: - cquery output is always fully ordered (--order_output=full). We could match this with query's controllable version, but I don't see a reason to make this yet another bit to configured. - query output annotates edges with select() conditions. cquery doesn't do this because select()s are resolved and removed from the graph after analysis. I think we could annotate edges with the *chosen* condition if there was demand, but that'd be a followup effort. PiperOrigin-RevId: 336377123 Change-Id: Iea0802850d18f6b047f8f35a5aa51926b97289e5

gregestren · 2020-10-09T23:06:37Z

@meisterT re: potential aquery integration.

gregestren · 2020-10-09T23:17:23Z

TODO:

Verify streamed mode doesn't apply here (i.e. partialResults in the output formatters aren't actually partial results)
Add tests
Add docs

joeleba

Generally LGTM. Re: aquery integration: conceptually it makes sense, but in actuality aquery outputs are humongous and I'm not sure a visual representation of that is useful. Maybe with very specific scopes specified by the filters.

joeleba · 2020-10-12T12:34:39Z

src/main/java/com/google/devtools/build/lib/query2/common/CommonQueryOptions.java

+  ///////////////////////////////////////////////////////////
+
+  @Option(
+      name = "graph:node_limit",


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.

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.

src/main/java/com/google/devtools/build/lib/query2/query/output/GraphOutputWriter.java

gregestren · 2020-10-12T15:16:01Z

Generally LGTM. Re: aquery integration: conceptually it makes sense, but in actuality aquery outputs are humongous and I'm not sure a visual representation of that is useful. Maybe with very specific scopes specified by the filters.

I figured as much. Thanks for clarifying. I'm personally happy just that you're aware this is a thing now.

Thankfully, query already has most of the infrastructure necessary to make this easy. query implements graph output (in GraphOutputFormatter) over a Digraph<Target>, which is a generic graph data structure with Target payloads. All output logic then runs over this data structure. To opt query in, all we have to do is create an equivalent Digraph<ConfiguredTarget>, which is a simple transformation from the backing graph. This change creates a new generic class for that common logic: GraphOutputWriter. query's GraphOutputFormatter then becomes a simple wrapper over that, and the new GraphOutputFormatterCallback is cquery's equivalent. A few differences: - cquery output is always fully ordered (--order_output=full). We could match this with query's controllable version, but I don't see a reason to make this yet another bit to configured. - query output annotates edges with select() conditions. cquery doesn't do this because select()s are resolved and removed from the graph after analysis. I think we could annotate edges with the *chosen* condition if there was demand, but that'd be a followup effort. PiperOrigin-RevId: 336377123 Change-Id: Iea0802850d18f6b047f8f35a5aa51926b97289e5

gregestren · 2020-10-12T19:37:23Z

Added a long disclaimer to PostAnalysisQueryBuildTool in support of @haxorz 's suggestions to guarantee non-streaming mode.

juliexxia

Overall LGTM! Just minor nits/qs. Excited this is happening!

src/main/java/com/google/devtools/build/lib/buildtool/PostAnalysisQueryBuildTool.java

juliexxia · 2020-10-12T21:09:18Z

src/main/java/com/google/devtools/build/lib/query2/common/CommonQueryOptions.java

@@ -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                        //


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

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

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

juliexxia · 2020-10-12T21:11:34Z

src/main/java/com/google/devtools/build/lib/query2/cquery/GraphOutputFormatterCallback.java

+import java.io.OutputStream;
+import java.util.Comparator;
+
+/** cquery output formatter that prints the result as factored graph in AT&amp;T GraphViz format. */


nit: just AT&T?

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 < and >. Likewise, the ampersand (&) should be written &

juliexxia · 2020-10-12T21:14:00Z

src/main/java/com/google/devtools/build/lib/query2/cquery/GraphOutputFormatterCallback.java

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


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

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.

juliexxia · 2020-10-12T21:17:43Z

src/main/java/com/google/devtools/build/lib/query2/cquery/GraphOutputFormatterCallback.java

+    for (ConfiguredTarget configuredTarget : partialResult) {
+      Node<ConfiguredTarget> node = graph.createNode(configuredTarget);
+      for (ConfiguredTarget dep : depsRetriever.getDirectDeps(configuredTarget)) {
+        if (allNodes.contains(dep)) {


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

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).

src/main/java/com/google/devtools/build/lib/query2/query/output/GraphOutputWriter.java

juliexxia · 2020-10-12T21:30:46Z

src/main/java/com/google/devtools/build/lib/query2/query/output/GraphOutputWriter.java

+          @Override
+          public void beginVisit() {
+            super.beginVisit();
+            // TODO(bazel-team): (2009) make this the default in Digraph.


laughing at the date of this TODO

I definitely noticed that too. :p

src/main/java/com/google/devtools/build/lib/buildtool/PostAnalysisQueryBuildTool.java

src/main/java/com/google/devtools/build/lib/query2/cquery/GraphOutputFormatterCallback.java

src/main/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryEnvironment.java

joeleba · 2020-10-13T14:00:05Z

Generally LGTM. Re: aquery integration: conceptually it makes sense, but in actuality aquery outputs are humongous and I'm not sure a visual representation of that is useful. Maybe with very specific scopes specified by the filters.

I figured as much. Thanks for clarifying. I'm personally happy just that you're aware this is a thing now.

Related: https://www.youtube.com/watch?v=GDbaBOCDwrQ

gregestren · 2020-10-13T22:10:41Z

Generally LGTM. Re: aquery integration: conceptually it makes sense, but in actuality aquery outputs are humongous and I'm not sure a visual representation of that is useful. Maybe with very specific scopes specified by the filters.

I figured as much. Thanks for clarifying. I'm personally happy just that you're aware this is a thing now.

Related: https://www.youtube.com/watch?v=GDbaBOCDwrQ

That graph visualization is super-cool.

google-cla bot added the cla: yes label Oct 9, 2020

gregestren requested review from juliexxia and haxorz October 9, 2020 23:04

gregestren self-assigned this Oct 9, 2020

gregestren added the team-Configurability platforms, toolchains, cquery, select(), config transitions label Oct 9, 2020

gregestren mentioned this pull request Oct 9, 2020

Please implement output format 'graph' for cquery and aquery #10843

Closed

meisterT requested a review from joeleba October 12, 2020 12:09

joeleba reviewed Oct 12, 2020

View reviewed changes

gregestren added 2 commits October 12, 2020 15:20

Lint cleanups

939e4ca

juliexxia reviewed Oct 12, 2020

View reviewed changes

haxorz approved these changes Oct 12, 2020

View reviewed changes

Update with review comments.

64b15e1

gregestren requested review from aiuto, floriographygoth and jin as code owners October 12, 2020 23:59

juliexxia approved these changes Oct 19, 2020

View reviewed changes

bazel-io closed this in 02cbcd2 Oct 19, 2020

gregestren deleted the cquery_graph_output branch October 19, 2020 19:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement cquery --output=graph #12248

Implement cquery --output=graph #12248

gregestren commented Oct 9, 2020 •

edited

Loading

gregestren commented Oct 9, 2020

gregestren commented Oct 9, 2020 •

edited

Loading

joeleba left a comment

joeleba Oct 12, 2020

gregestren Oct 12, 2020

gregestren commented Oct 12, 2020

gregestren commented Oct 12, 2020

juliexxia left a comment

juliexxia Oct 12, 2020

gregestren Oct 13, 2020

juliexxia Oct 19, 2020

juliexxia Oct 12, 2020

gregestren Oct 13, 2020 •

edited

Loading

juliexxia Oct 12, 2020

gregestren Oct 13, 2020

juliexxia Oct 12, 2020

gregestren Oct 13, 2020 •

edited

Loading

juliexxia Oct 12, 2020

gregestren Oct 13, 2020

joeleba commented Oct 13, 2020

gregestren commented Oct 13, 2020

Implement cquery --output=graph #12248

Implement cquery --output=graph #12248

Conversation

gregestren commented Oct 9, 2020 • edited Loading

gregestren commented Oct 9, 2020

gregestren commented Oct 9, 2020 • edited Loading

joeleba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregestren commented Oct 12, 2020

gregestren commented Oct 12, 2020

juliexxia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregestren Oct 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregestren Oct 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeleba commented Oct 13, 2020

gregestren commented Oct 13, 2020

gregestren commented Oct 9, 2020 •

edited

Loading

gregestren commented Oct 9, 2020 •

edited

Loading

gregestren Oct 13, 2020 •

edited

Loading

gregestren Oct 13, 2020 •

edited

Loading