Cquery can report transitions #15038

illicitonion · 2022-03-14T16:17:53Z

When the existing --transitions flag is set to a non-None value,
cquery will report the configuration its dependencies are configured in.
This allows for pruning away values from ruleInputs which are not
required in the current configuration.

Example output from running:
bazel cquery --transitions=lite --output=jsonproto 'deps(...)':

[...]
        "ruleInput": ["@bazel_tools//src/conditions:host_windows", "@bazel_tools//src/tools/launcher:launcher", "@bazel_tools//tools/launcher:launcher_windows"],
        "configuredRuleInput": [{
          "label": "@bazel_tools//src/tools/launcher:launcher",
          "configurationChecksum": "01ec5513a9fc41b2a15570123817f3c2200ad9aeb21b1181d588a4b4f91d5693",
          "configurationId": 3
        }]
[...]

Previously on a non-Windows platform it was not possible to determine
that the two windows-specific ruleInputs are not actually required - now
we can see from the configuredRuleInput section that the
windows-specific dependencies have been pruned by selection, and we can
see that a transition has re-configured the ruleInput into the exec
configuration.

For dependencies which were not subject to transition (e.g. because
they're in a non-transition attribute), or which had no configuration
(e.g. because they're a source file), we add the label as a
ConfiguredRuleInput without any configuration information. This
indicates that the dependency has not been pruned, but that the caller
should determine the correct configuration from context (probably be
determining whether it's a source file, and if so, considering it
un-configured, otherwise propagating the contextual ConfiguredTarget's
configuration).

Fixes #14610
Fixes #14617

Implementation-wise, this took roughly the following shape:

Extract TransitionResolver from being inline in TransitionsOutputFormatterCallback
Extract KnownTargetsDependencyResolver from TransitionsOutputFormatterCallback.FormatterDependencyResolver
Conditionally call these from ProtoOutputFormatterCallback depending
on the --transitions flag.

When the existing `--transitions` flag is set to a non-None value, cquery will report the configuration its dependencies are configured in. This allows for pruning away values from ruleInputs which are not required in the current configuration. Example output from running: `bazel cquery --transitions=lite --output=jsonproto 'deps(...)'`: ``` [...] "ruleInput": ["@bazel_tools//src/conditions:host_windows", "@bazel_tools//src/tools/launcher:launcher", "@bazel_tools//tools/launcher:launcher_windows"], "configuredRuleInput": [{ "label": "@bazel_tools//src/tools/launcher:launcher", "configurationChecksum": "01ec5513a9fc41b2a15570123817f3c2200ad9aeb21b1181d588a4b4f91d5693", "configurationId": 3 }] [...] ``` Previously on a non-Windows platform it was not possible to determine that the two windows-specific ruleInputs are not actually required - now we can see from the configuredRuleInput section that the windows-specific dependencies have been pruned by selection, and we can see that a transition has re-configured the ruleInput into the exec configuration. For dependencies which were not subject to transition (e.g. because they're in a non-transition attribute), or which had no configuration (e.g. because they're a source file), we add the label as a ConfiguredRuleInput _without_ any configuration information. This indicates that the dependency has not been pruned, but that the caller should determine the correct configuration from context (probably be determining whether it's a source file, and if so, considering it un-configured, otherwise propagating the contextual ConfiguredTarget's configuration). Fixes bazelbuild#14610 Fixes bazelbuild#14617 Implementation-wise, this took roughly the following shape: 1. Extract TransitionResolver from being inline in TransitionsOutputFormatterCallback 2. Extract KnownTargetsDependencyResolver from TransitionsOutputFormatterCallback.FormatterDependencyResolver 3. Conditionally call these from ProtoOutputFormatterCallback depending on the --transitions flag.

illicitonion · 2022-03-14T16:18:13Z

/cc @gregestren @sdtwigg @aranguyen

gregestren · 2022-03-21T17:19:30Z

FYI we have different people assigned to the associated issues. I think the right followup from our side is to sync our understandings, then provide feedback here. Please stay tuned.

aranguyen · 2022-03-28T14:05:35Z

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

+import java.util.function.Function;
+import java.util.stream.Collectors;
+
+public class KnownTargetsDependencyResolver extends DependencyResolver {


Could we add documentation to explain what this class is and what it does?

this seems to be the same with FormatterDependencyResolver with an additional filter? Is it not possible to add the filter to the existing code?

This is exactly a FormatterDependencyResolver - no new filter, it was just extracted to be a top-level class. Because FormatterDependencyResolver uses state from its outer class, extracting it out to be explicit about which state it depends on felt useful. The problem is that we need to use it from ProtoOutputFormatterCallback as well as TransitionsOutputFormatterCallback - these are separately invoked by different options to cquery, so in order to use this logic from both it either needs extracting or duplicating.

aranguyen · 2022-03-28T16:27:02Z

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

+            }
+          }
+        } catch (DependencyResolver.Failure | InconsistentAspectOrderException e) {
+          // This is an abuse of InterruptedException.


What does this mean?

This was copied and pasted from TransitionsOutputFormatterCallback. I think what it's saying is: processOutput only has two checked exceptions it's allowed to throw, IOException and InterruptedException, so rather than changing the processOutput API (a larger change), or throwing a RuntimeException (generally frowned upon in Java, but 🤷 seems better than this), hide the error in an InterruptedException.

I tried adding the exception to the interface, but ended up with a cyclic dependency because it ended up needing the com.google.devtools.build.lib.cmdline.BatchCallback interface (very high-level) to depend on the analysis package (very low level).

I could use a RuntimeException, or introduce a new Exception type, but I assume whoever originally wrote this code decided it was not worthwhile.

This code has an even sketchier hack, we could do something similar if we needed?

ok thanks for the explanation. Looking at the previous commit, it seems to be part of a major change to reporting of Starlark errors and the call stack, so I would leave it.

aranguyen · 2022-03-28T16:27:13Z

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

+
+  @AutoValue
+  @Immutable
+  public abstract static class ResolvedTransition {


Similar question to KnownTargetsDepencyResolver/FormatterDependencyResolver, was not possible to add the filter functionality without extracting this piece of logic from TransitionsOutputFormatterCallback. Admittedly, this does look a bit cleaner

Because this code is now used from ProtoOutputFormatterCallback we need to expose this logic standalone somewhere. The change here is really less that it's filtering, and more that it's taking the computed data and setting a proto with its fields rather than just printing it to stdout. So we need this data structure so that one caller can consume the structured information, whereas the other can just print.

illicitonion · 2022-03-30T12:32:40Z

Thanks for the review - I added a bunch of javadoc to KnownTargetsDependencyResolver and TransitionResolver which hopefully clears some of this up - PTAL!

illicitonion · 2022-04-20T13:51:43Z

@aranguyen would you be able to give this another pass? Thanks!

aranguyen · 2022-04-20T14:46:02Z

@illicitonion I completely missed this, very sorry about that. I will have this reviewed again by eod today.

aranguyen

thanks for adding the documentation, that was really helpful. I added a few more comments and sorry again for overlooking your responses last time.

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

src/test/java/com/google/devtools/build/lib/query2/cquery/ProtoOutputFormatterCallbackTest.java

aranguyen · 2022-04-21T02:44:53Z

src/test/java/com/google/devtools/build/lib/query2/cquery/ProtoOutputFormatterCallbackTest.java

@@ -29,11 +29,13 @@
 import com.google.devtools.build.lib.events.NullEventHandler;


Could you please make sure this test target build after the change? I did an import changelist and it did not build successfully. Even though the checks in this PR seem to have passed, the test for ProtoOutputFormatterCallbackTest was not included looking at the artifacts (only TransitionsOutputFormatterTest was tested). I'll do a bit more debugging tomorrow morning.

It worked for me in open source:

//src/test/java/com/google/devtools/build/lib/query2/cquery:ProtoOutputFormatterCallbackTest PASSED in 32.9s

I introduced a compile break to it, and it stopped compiling, so in a Bazel world this looks fine... I'm not sure what would be different internally, sorry!

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

sgowroji · 2022-04-21T04:53:43Z

Hello @illicitonion, Could you please reply on the review comments. Thanks!

illicitonion · 2022-04-21T11:04:13Z

@aranguyen All addressed - thanks for the review! Let me know if I can be helpful with the test compiling issue at all

aranguyen · 2022-04-21T13:55:47Z

@illicitonion thanks! I'll work on the import and will update.

aranguyen · 2022-04-21T19:36:01Z

@illicitonion FYI, I had to do a small modification to your code for the internal tests to pass (instead of having ProtoOutputFormatter.toTargetProtoBuffer to return Build.Target.Builder, I'm having it returning Build.Target. The import now awaiting internal review from @sdtwigg

brentleyjones · 2022-05-04T15:14:16Z

@bazel-io flag

ckolli5 · 2022-05-09T03:58:35Z

@bazel-io fork 5.2.0

Bazel 6 got the ability to report configuration transitions: bazelbuild/bazel#15038 This allows us to stop over-estimating affected targets when dependencies exist in platforms other than the current one.

gregestren · 2024-02-21T16:05:42Z

For dependencies which were not subject to transition (e.g. because
they're in a non-transition attribute), or which had no configuration
(e.g. because they're a source file), we add the label as a
ConfiguredRuleInput without any configuration information.

Hey @illicitonion . We're exploring some cquery --output=proto cleanups and rediscovered this point.

Was this choice important? What would you think if we universally include the configuration info? While redundant, it does make it easier to directly identify all metadata for any input.

illicitonion · 2024-02-21T16:06:30Z

I would love to universally include configuration info. Yes please!

gregestren · 2024-02-21T16:10:07Z

Do you know why that wasn't done in the first place?

illicitonion · 2024-02-21T16:11:56Z

I believe the data wasn't readily available - the existing transition following mechanism only applied to transitioned targets, and there wasn't convenient access to the configuration of non-transitioned targets. If it's convenient (or useful) to get that information, it'd be great to do so - the ambiguity ("is this a source file with no configuration, or just untransitioned and in the current configuration") is annoying, just not annoying enough for me to get around to fixing myself :)

gregestren · 2024-02-21T16:16:56Z

I see. I think we can handle the source file / non-source difference. I'll share a PR with whatever I can do.

…input` This is an iteration of bazelbuild/bazel#15038. That PR added the concept of `configured_rule_input`, where `--output=proto` rules include both the labels and configurations of their dependencies. That used `CqueryTransitionResolver` to find the dependencies. `CqueryTransitionResolver` does this by directly querying the Skyframe graph. However, cquery's view of the graph doesn't directly match the Skyframe graph. For example, cquery doesn't explicitly list aspects: if CT `//foo` depends on aspect A which has an implicit dep on CT `//bar`, the Skyframe graph shows `//foo -> A -> //bar`. Since cquery can't directly show `A` (see [More accurate cquery](https://docs.google.com/document/d/1x-RqFFEcGIzVgtYHIq6s6Qt4vYtJWz3nuHIuqeI89dU/edit#heading=h.5mcn15i0e1ch)), cquery instead shows `//foo -> //bar`. For any cquery use we want to use cquery's interpretation of the graph as a consistent source of truth. This change refactors `--output=proto` to do that. This means, for example, that `cquery --output=proto` will now have `configured_rule_input` entries for deps like `//bar`. Before this change there's no way to tell where the dependency on `//bar` comes from. Also correctly includes aliases (doesn't dereference them as either outputs or `configured_rule_input` entries of other outputs). Also updates the API contract for `configured_rule_input`. Before, "same configuration" deps had their configuration fields omitted. Now they're always included, except for source files which have no configuration. PiperOrigin-RevId: 621245620

…input` This is an iteration of #15038. That PR added the concept of `configured_rule_input`, where `--output=proto` rules include both the labels and configurations of their dependencies. That used `CqueryTransitionResolver` to find the dependencies. `CqueryTransitionResolver` does this by directly querying the Skyframe graph. However, cquery's view of the graph doesn't directly match the Skyframe graph. For example, cquery doesn't explicitly list aspects: if CT `//foo` depends on aspect A which has an implicit dep on CT `//bar`, the Skyframe graph shows `//foo -> A -> //bar`. Since cquery can't directly show `A` (see [More accurate cquery](https://docs.google.com/document/d/1x-RqFFEcGIzVgtYHIq6s6Qt4vYtJWz3nuHIuqeI89dU/edit#heading=h.5mcn15i0e1ch)), cquery instead shows `//foo -> //bar`. For any cquery use we want to use cquery's interpretation of the graph as a consistent source of truth. This change refactors `--output=proto` to do that. This means, for example, that `cquery --output=proto` will now have `configured_rule_input` entries for deps like `//bar`. Before this change there's no way to tell where the dependency on `//bar` comes from. Also correctly includes aliases (doesn't dereference them as either outputs or `configured_rule_input` entries of other outputs). Also updates the API contract for `configured_rule_input`. Before, "same configuration" deps had their configuration fields omitted. Now they're always included, except for source files which have no configuration. PiperOrigin-RevId: 621245620 Change-Id: Ia19a2544a82652f196c03c3ca439ef620927b5f9

…input` This is an iteration of bazelbuild/bazel#15038. That PR added the concept of `configured_rule_input`, where `--output=proto` rules include both the labels and configurations of their dependencies. That used `CqueryTransitionResolver` to find the dependencies. `CqueryTransitionResolver` does this by directly querying the Skyframe graph. However, cquery's view of the graph doesn't directly match the Skyframe graph. For example, cquery doesn't explicitly list aspects: if CT `//foo` depends on aspect A which has an implicit dep on CT `//bar`, the Skyframe graph shows `//foo -> A -> //bar`. Since cquery can't directly show `A` (see [More accurate cquery](https://docs.google.com/document/d/1x-RqFFEcGIzVgtYHIq6s6Qt4vYtJWz3nuHIuqeI89dU/edit#heading=h.5mcn15i0e1ch)), cquery instead shows `//foo -> //bar`. For any cquery use we want to use cquery's interpretation of the graph as a consistent source of truth. This change refactors `--output=proto` to do that. This means, for example, that `cquery --output=proto` will now have `configured_rule_input` entries for deps like `//bar`. Before this change there's no way to tell where the dependency on `//bar` comes from. Also correctly includes aliases (doesn't dereference them as either outputs or `configured_rule_input` entries of other outputs). Also updates the API contract for `configured_rule_input`. Before, "same configuration" deps had their configuration fields omitted. Now they're always included, except for source files which have no configuration. (cherry picked from commit 654d988)

…input` This is an iteration of bazelbuild#15038. That PR added the concept of `configured_rule_input`, where `--output=proto` rules include both the labels and configurations of their dependencies. That used `CqueryTransitionResolver` to find the dependencies. `CqueryTransitionResolver` does this by directly querying the Skyframe graph. However, cquery's view of the graph doesn't directly match the Skyframe graph. For example, cquery doesn't explicitly list aspects: if CT `//foo` depends on aspect A which has an implicit dep on CT `//bar`, the Skyframe graph shows `//foo -> A -> //bar`. Since cquery can't directly show `A` (see [More accurate cquery](https://docs.google.com/document/d/1x-RqFFEcGIzVgtYHIq6s6Qt4vYtJWz3nuHIuqeI89dU/edit#heading=h.5mcn15i0e1ch)), cquery instead shows `//foo -> //bar`. For any cquery use we want to use cquery's interpretation of the graph as a consistent source of truth. This change refactors `--output=proto` to do that. This means, for example, that `cquery --output=proto` will now have `configured_rule_input` entries for deps like `//bar`. Before this change there's no way to tell where the dependency on `//bar` comes from. Also correctly includes aliases (doesn't dereference them as either outputs or `configured_rule_input` entries of other outputs). Also updates the API contract for `configured_rule_input`. Before, "same configuration" deps had their configuration fields omitted. Now they're always included, except for source files which have no configuration. PiperOrigin-RevId: 621245620 Change-Id: Ia19a2544a82652f196c03c3ca439ef620927b5f9

oquenchil assigned gregestren Mar 16, 2022

sgowroji added the team-Configurability platforms, toolchains, cquery, select(), config transitions label Mar 19, 2022

sdtwigg added P3 We're not considering working on this, but happy to review a PR. (No assignee) and removed P3 We're not considering working on this, but happy to review a PR. (No assignee) labels Mar 25, 2022

gregestren assigned aranguyen and sdtwigg and unassigned gregestren Mar 25, 2022

aranguyen reviewed Mar 28, 2022

View reviewed changes

Add javadoc

cfb4e5f

aranguyen reviewed Apr 21, 2022

View reviewed changes

sgowroji added the awaiting-user-response Awaiting a response from the author label Apr 21, 2022

illicitonion added 3 commits April 21, 2022 11:29

Add copyright headers

880a47c

Immutable all the things

835a9b8

Simplifications

1f88d48

aranguyen removed the awaiting-user-response Awaiting a response from the author label Apr 22, 2022

bazel-io closed this in 9994c32 May 2, 2022

bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label May 4, 2022

bazel-io mentioned this pull request May 9, 2022

[5.2.0] Cquery can report transitions #15425

Closed

bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label May 9, 2022

ckolli5 mentioned this pull request May 12, 2022

Cquery can report transitions #15475

Closed

illicitonion deleted the cquery-configuration-transitions branch March 24, 2023 14:48

illicitonion mentioned this pull request Mar 24, 2023

Filter unmatching configuration-specific dependencies bazel-contrib/target-determinator#55

Merged

gregestren mentioned this pull request Jul 27, 2023

cquery doesn't include artificial toolchain deps in ruleInputs section, but does in configuredRuleInputs #19058

Open

illicitonion mentioned this pull request Aug 28, 2023

Possible circular configurations? bazel-contrib/target-determinator#72

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cquery can report transitions #15038

Cquery can report transitions #15038

illicitonion commented Mar 14, 2022

illicitonion commented Mar 14, 2022

gregestren commented Mar 21, 2022

aranguyen Mar 28, 2022

aranguyen Mar 28, 2022

illicitonion Mar 30, 2022

aranguyen Mar 28, 2022

illicitonion Mar 30, 2022

aranguyen Apr 20, 2022

aranguyen Mar 28, 2022

illicitonion Mar 30, 2022

illicitonion commented Mar 30, 2022

illicitonion commented Apr 20, 2022

aranguyen commented Apr 20, 2022

aranguyen left a comment

aranguyen Apr 21, 2022

illicitonion Apr 21, 2022

sgowroji commented Apr 21, 2022

illicitonion commented Apr 21, 2022

aranguyen commented Apr 21, 2022

aranguyen commented Apr 21, 2022

brentleyjones commented May 4, 2022

ckolli5 commented May 9, 2022

gregestren commented Feb 21, 2024

illicitonion commented Feb 21, 2024

gregestren commented Feb 21, 2024

illicitonion commented Feb 21, 2024

gregestren commented Feb 21, 2024

		@@ -29,11 +29,13 @@
		import com.google.devtools.build.lib.events.NullEventHandler;

Cquery can report transitions #15038

Cquery can report transitions #15038

Conversation

illicitonion commented Mar 14, 2022

illicitonion commented Mar 14, 2022

gregestren commented Mar 21, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

illicitonion commented Mar 30, 2022

illicitonion commented Apr 20, 2022

aranguyen commented Apr 20, 2022

aranguyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgowroji commented Apr 21, 2022

illicitonion commented Apr 21, 2022

aranguyen commented Apr 21, 2022

aranguyen commented Apr 21, 2022

brentleyjones commented May 4, 2022

ckolli5 commented May 9, 2022

gregestren commented Feb 21, 2024

illicitonion commented Feb 21, 2024

gregestren commented Feb 21, 2024

illicitonion commented Feb 21, 2024

gregestren commented Feb 21, 2024