bazelbuild / bazelbuild/bazel

cquery: more precise results

Open
#16,310 23 comments 0 reactions 1 assignee Claimed by @gregestren View on GitHub
cquery_aspects P2 query bugs team-Configurability type: feature request
Dominant language
Java
Stars
25.8k
Forks
4.6k
Avg merge
2d 18h
Merged PRs (30d)
75

Description

### Description of the feature request:

cquery only *implicitly* shows deps from aspects and toolchain resolution.

For example, assume `//tt:parent` depends on `//tt:child` and that dependency adds "my_aspect" which has an implicit dep on `//tt:aspect_dep`. Querying this produces:

```
$ bazel cquery 'somepath(//tt:parent, //tt:aspect_dep)'
//tt:parent (e022623)
//tt:aspect_dep (e022623)
```

This result includes `my_aspect`'s dep, but misleadingly makes it look like `//tt:parent`'s direct dep.

With the proposed change you'd get:

```
$ bazel cquery 'somepath(//tt:parent, //tt:aspect_dep)'
//tt:parent (e022623)
//tt:defs.bzl%_my_aspect of //tt:child (e022623)
//tt:aspect_dep (e022623)
```

Toolchains are a similar story.

---

The current behavior is no accident. We explicitly added the current implicit support to at least guarantee cquery doesn't under-eport dependencies. The problem with the full solution is it's a much more invasive change into the Java code that powers cquery and aquery.

I prototyped code to do this, which is why I'm writing this issue now. But it'll take some careful planning and tolerance for reliability issues (crashes in unexpected places) to fully land this extension and know we've properly captured every call sequence.

### What underlying problem are you trying to solve with this feature?

1. Help users understand where their dependencies come from (i.e. "*why does `foo` depend on `bar`? I don't see any reference to `bar` in `foo`'s deps*")
2. Help devs accurately diagnose build structure
3. Improve cquery accuracy. In particular, `rpaths` works horribly with the current logic (it shows forward deps caused by aspects even though `rpaths` should only show backward deps). This would allow `rpaths` and possibly other query functions to work correctly

### Which operating system are you running Bazel on?

n/a

### What is the output of `bazel info release`?

n/a

### If `bazel info release` returns `development version` or `(@non-git)`, tell us how you built Bazel.

_No response_

### What's the output of `git remote get-url origin; git rev-parse master; git rev-parse HEAD` ?

_No response_

### Have you found anything relevant by searching the web?

- https://github.com/bazelbuild/bazel/issues/13866
- https://github.com/bazelbuild/bazel/issues/14799

_No response_

### Any other information, logs, or outputs that you want to share?

I think this update has to roll out over multiple PRs and behind a feature flag. It's *really* hard to audit every call point into the affected code: the full combination of ` x x `. Current code assumes every node is a `ConfiguredTarget`, and makes assumptions accordingly. The essence of this fix is to generalize that to `Aspect` and `Toolchain`.

There's also some API challenge:

`//tt:defs.bzl%_my_aspect of //tt:child` is not a label. So `$ bazel cquery 'somepath(//tt:parent, //tt:defs.bzl%_my_aspect of //tt:child)'` triggers a syntax error.

We also need to consider appropriate syntax for `cquery`'s various output formats.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.