search

bazelbuild / bazel

a fast, scalable, multi-language and extensible build system
https://bazel.build
Apache License 2.0
23.23k stars 4.07k forks source link

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

Open illicitonion opened 1 year ago

illicitonion commented 1 year ago

Description of the bug:

See https://github.com/illicitonion/repro-cquery-toolchains-in-ruleinputs

Related to https://docs.google.com/document/d/1x-RqFFEcGIzVgtYHIq6s6Qt4vYtJWz3nuHIuqeI89dU/edit

Currently deps cqueries artificially flattens toolchains into nodes for their dependencies to make toolchains appear in deps queries. However, this flattening isn't performed for the rule_inputs field of the output.

Interestingly, they do appear in the configured_rule_inputs field.

Ideally we'd have a marker node for toolchains in rule_inputs, so we can chase edges of the graph from a big query like bazel cquery 'deps(//...)'.

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Clone https://github.com/illicitonion/repro-cquery-toolchains-in-ruleinputs and run:

% bazel cquery --output=jsonproto '//dir:rule' 2>/dev/null

expect to see //dir:my_toolchain in ruleInputs but instead see nothing.

Which operating system are you running Bazel on?

macOS

What is the output of bazel info release?

release 6.3.0

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

Is this a regression? If yes, please try to identify the Bazel commit where the bug was introduced.

No response

Have you found anything relevant by searching the web?

No response

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

No response

illicitonion commented 1 year ago

/cc @gregestren

gregestren commented 1 year ago

Note: configuredRuleInput came from https://github.com/bazelbuild/bazel/pull/15038.

I think the core issue here is that cquery's proto format is mostly a clone of query's, back from when cquery itself was first forked off query. The original focus on cquery was to improve the standard text output format, with less focus on refactoring the proto.

To the extend rule_input is inherited from query, I'd expect the logic populating it not to consider toolchains (which don't kick in until the cquery phase).

I don't think https://docs.google.com/document/d/1x-RqFFEcGIzVgtYHIq6s6Qt4vYtJWz3nuHIuqeI89dU/edit#heading=h.5mcn15i0e1ch is 100% related - although it's definitely related in the sense that we should have a standardized way to handle and emit toolchain & aspect dependencies. And yes that doc does have a section for --output=proto.

We have another feature request bug to embed configurations properly into the proto output. I'm not sure this is the exact same scope but maybe we can find how this might fit in as part of that.

illicitonion commented 1 year ago

I don't know this code very well - how much hassle do you think it would be to copy over the flattened targets-reached-via-toolchains into rule_inputs like they're handled for including in the text output? We already force those toolchains to get evaluated in order for them to show up as deps, so I'd hope it wouldn't be too wild to also include them in a proto output attribute...?

gregestren commented 1 year ago

I would imagine it's manageable. And a tight change like that to make it consistent with text output sounds reasonable to me (larger API design issues aside).

I imagine the change would be in https://github.com/bazelbuild/bazel/blob/f309e0fc7fefa5d597db0b2dec7762e5523b69e7/src/main/java/com/google/devtools/build/lib/query2/cquery/ProtoOutputFormatterCallback.java#L60 possibly using similar logic to https://github.com/bazelbuild/bazel/blob/f309e0fc7fefa5d597db0b2dec7762e5523b69e7/src/main/java/com/google/devtools/build/lib/query2/PostAnalysisQueryEnvironment.java#L505 which is how aspects make it into text output.

I'd think that logic is already included in proto since https://github.com/bazelbuild/bazel/blob/f309e0fc7fefa5d597db0b2dec7762e5523b69e7/src/main/java/com/google/devtools/build/lib/query2/PostAnalysisQueryEnvironment.java#L465 is a generic accessor into the graph that all output formatters should rely on. But obviously that's not happening.

github-actions[bot] commented 1 month ago

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs. If you think this issue is still relevant and should stay open, please post any comment here and the issue will no longer be marked as stale.