cquery: Include toolchains as ruleInputs #22
Conversation
These were already returned in a text output for a deps query - this adds them also as ruleInputs.
|
cc @gregestren |
|
I'm open to this but want to run this by someone else with an interest in cquery's proto outputs. I'm not sure of their Github handle so I'll send this link to them directly. |
|
Sounds great - please let me know if there's anything to do here :) It'd be easy to gate this behaviour behind a flag or something if that'd be simpler for folks. |
|
@gregestren any news here? Thanks! |
|
No new updates here yet, I'm sorry. I am still trying to get bandwidth on this but haven't followed up yet. |
WalkthroughThe changes introduce explicit handling and reporting of implicit toolchain dependencies in the query output of a build system. The core update modifies how target protocol buffers are generated, ensuring that implicit toolchain dependencies are included as rule inputs. This is achieved by altering method signatures and internal logic in the proto output formatter classes, and updating related method calls to provide the necessary dependency information. Additionally, a new integration test is added to verify that toolchain dependencies are correctly reported in configured queries. Changes
Sequence Diagram(s)sequenceDiagram
participant QueryEngine
participant ProtoOutputFormatterCallback
participant ProtoOutputFormatter
participant RuleConfiguredTarget
QueryEngine->>ProtoOutputFormatterCallback: processOutput(ConfiguredTarget)
alt ConfiguredTarget is RuleConfiguredTarget
ProtoOutputFormatterCallback->>RuleConfiguredTarget: getImplicitDeps()
ProtoOutputFormatterCallback->>ProtoOutputFormatter: toTargetProtoBuffer(target, implicitDeps)
else
ProtoOutputFormatterCallback->>ProtoOutputFormatter: toTargetProtoBuffer(target, empty set)
end
ProtoOutputFormatter->>QueryEngine: Build.Target proto (with all deps)
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @arvi18, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request addresses issue bazelbuild#19058 by including toolchains as ruleInputs in cquery results. Previously, these toolchains were only available in the text output of a deps query. The changes modify the ProtoOutputFormatterCallback and ProtoOutputFormatter classes to include toolchain dependencies as rule inputs, which are identified as implicit dependencies of RuleConfiguredTargets. A new test case is added to verify the functionality.
Highlights
- Toolchain Inclusion: Toolchains are now included as ruleInputs in cquery output, fixing issue cquery doesn't include artificial toolchain deps in ruleInputs section, but does in configuredRuleInputs bazelbuild/bazel#19058.
- Implicit Dependency Handling: The code now explicitly handles implicit dependencies (toolchains) that are not captured by attribute traversal.
- Code Modification: Modified
ProtoOutputFormatterCallbackto add implicit dependencies fromRuleConfiguredTargetas rule inputs. - Code Modification: Modified
ProtoOutputFormatterto include implicit dependencies when converting aTargetto aBuild.Targetprotobuf. - Test Addition: Added a new integration test
test_toolchain_rule_inputsto verify the correct inclusion of toolchains as rule inputs.
Changelog
Click here to see the changelog
- src/main/java/com/google/devtools/build/lib/query2/cquery/ProtoOutputFormatterCallback.java
- Imported
ImmutableSortedSetto manage implicit dependencies. - Imported
RuleConfiguredTargetto identify rule-based configured targets. - Added logic to extract implicit dependencies (toolchains) from
RuleConfiguredTargetand include them as rule inputs. - Modified
processOutputto add implicit dependencies as inputs.
- Imported
- src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java
- Imported
ImmutableSortedSet. - Modified
toTargetProtoBuffermethods to accept and handleImmutableSortedSet<Label>for extra implicit dependencies. - Modified
toTargetProtoBufferto include implicit dependencies when converting a Target to a Build.Target protobuf. - Modified
processOutputto pass an emptyImmutableSortedSettotoTargetProtoBuffer.
- Imported
- src/main/java/com/google/devtools/build/lib/query2/query/output/StreamedJSONProtoOutputFormatter.java
- Imported
ImmutableSortedSet. - Modified
processOutputto pass an emptyImmutableSortedSettotoTargetProtoBuffer.
- Imported
- src/main/java/com/google/devtools/build/lib/query2/query/output/StreamedProtoOutputFormatter.java
- Imported
ImmutableSortedSet. - Modified
createPostFactoStreamCallbackto pass an emptyImmutableSortedSettotoTargetProtoBuffer.
- Imported
- src/test/shell/integration/configured_query_test.sh
- Added a new integration test
test_toolchain_rule_inputsto verify the correct inclusion of toolchains as rule inputs.
- Added a new integration test
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A toolchain's hidden,
Implicit, now brought to light,
Query's answer clear.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request introduces changes to include toolchains as ruleInputs in cquery, addressing issue bazelbuild#19058. The changes involve modifying the ProtoOutputFormatterCallback and ProtoOutputFormatter classes to handle toolchain dependencies correctly. Additionally, a new test case is added to verify the functionality.
Summary of Findings
- Missing null check: In
ProtoOutputFormatterCallback.java, theconfiguredTargetKeyis not checked for null before callinggetConfiguration(configurationKey). This could lead to a NullPointerException ifconfiguredTargetKeyis null. - Inconsistent handling of implicit dependencies: The code in
ProtoOutputFormatterCallback.javaadds implicit dependencies as rule inputs. It's important to ensure that this logic is consistent with how other dependencies are handled and that it doesn't introduce any unintended side effects. - Potential performance impact: The addition of toolchain dependencies as rule inputs could potentially increase the size of the output and impact performance, especially for large projects. It's important to monitor the performance impact of these changes.
Merge Readiness
The pull request introduces important functionality by including toolchains as ruleInputs in cquery. However, there are a few issues that need to be addressed before merging. Specifically, the missing null check in ProtoOutputFormatterCallback.java should be added to prevent potential NullPointerExceptions. Additionally, the potential performance impact of these changes should be monitored. I am unable to approve this pull request, and recommend that it not be merged until the identified issues are addressed and re-reviewed. Other reviewers should also review and approve this code before merging.
| ImmutableSortedSet<Label> ruleInputs = ImmutableSortedSet.<Label>naturalOrder() | ||
| .addAll(rule.getSortedLabels(dependencyFilter)) | ||
| .addAll(extraImplicitDeps) | ||
| .build(); |
There was a problem hiding this comment.
Consider adding a comment explaining why ImmutableSortedSet is being used here, and what the performance implications are.
// Include explicit elements for all direct inputs and outputs of a rule; this goes beyond
// what is available from the attributes above, since it may also (depending on options)
// include implicit outputs, exec-configuration outputs, and default values.
// Using ImmutableSortedSet to ensure deterministic order of rule inputs.
ImmutableSortedSet<Label> ruleInputs = ImmutableSortedSet.<Label>naturalOrder()
.addAll(rule.getSortedLabels(dependencyFilter))
.addAll(extraImplicitDeps)
.build();![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java (1)
179-180: Consider adding JavaDoc to clarify the purpose ofextraImplicitDeps.The method signature change is correct, but the
extraImplicitDepsparameter would benefit from documentation explaining that it's intended for implicit toolchain dependencies./** Converts a logical {@link Target} object into a {@link Build.Target} protobuffer. */ + * @param target The target to convert + * @param extraImplicitDeps Additional implicit dependencies (like toolchains) to include as rule inputs + * @param extraDataForAttrHash Additional data to include in synthetic attribute hash calculation */ private Build.Target toTargetProtoBuffer(Target target, ImmutableSortedSet<Label> extraImplicitDeps, Object extraDataForAttrHash) throws InterruptedException {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/com/google/devtools/build/lib/query2/cquery/ProtoOutputFormatterCallback.java(3 hunks)src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java(4 hunks)src/main/java/com/google/devtools/build/lib/query2/query/output/StreamedJSONProtoOutputFormatter.java(2 hunks)src/main/java/com/google/devtools/build/lib/query2/query/output/StreamedProtoOutputFormatter.java(2 hunks)src/test/shell/integration/configured_query_test.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
src/test/shell/integration/configured_query_test.sh
[warning] 1525-1525: Expanding an array without an index only gives the first element.
(SC2128)
🔇 Additional comments (11)
src/main/java/com/google/devtools/build/lib/query2/query/output/StreamedProtoOutputFormatter.java (1)
16-16: Method signature update well-implementedThe import of
ImmutableSortedSetand the updated call totoTargetProtoBufferwith an empty set are correctly implemented to accommodate the new parameter for implicit dependencies. This change aligns properly with the method signature update in the parent class.Also applies to: 42-42
src/main/java/com/google/devtools/build/lib/query2/query/output/StreamedJSONProtoOutputFormatter.java (1)
16-16: Method signature update correctly appliedThe necessary import has been added and the method call updated properly to accommodate the new parameter for implicit dependencies, maintaining consistency with the parent class changes.
Also applies to: 48-48
src/main/java/com/google/devtools/build/lib/query2/cquery/ProtoOutputFormatterCallback.java (3)
20-20: Required imports properly addedThe imports for
ImmutableSortedSetandRuleConfiguredTargetare correctly added to support the new functionality for collecting toolchain dependencies.Also applies to: 31-31
249-260: Well-documented implementation for collecting toolchain dependenciesThis implementation correctly identifies and collects implicit toolchain dependencies that wouldn't otherwise be discovered through attribute traversal. The code is clear, well-commented, and follows the appropriate pattern for collecting dependencies into a sorted set.
261-261: Proper integration with target proto buffer generationThe collected implicit dependencies are correctly passed to the
toTargetProtoBuffermethod, ensuring toolchain dependencies are included in the rule inputs for the proto output.src/test/shell/integration/configured_query_test.sh (1)
1524-1603: Comprehensive test for toolchain rule inputsThis test thoroughly verifies the new functionality by:
- Setting up a custom toolchain type and implementation
- Creating a rule that consumes the toolchain
- Running a configured query with the extra toolchain
- Verifying that the toolchain appears in the rule inputs
The test covers the end-to-end functionality well and serves as good documentation for how toolchain dependencies should now appear in query results.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 1525-1525: Expanding an array without an index only gives the first element.
(SC2128)
src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java (5)
29-29: Import addition is appropriate.Adding
ImmutableSortedSetis necessary to support the new parameter intoTargetProtoBuffermethod.
174-176: Method signature change looks good with proper delegation.The method now accepts an additional parameter
extraImplicitDepsto include toolchain dependencies. The delegation to the private method with an empty default forextraDataForAttrHashmaintains a clean API separation.
247-252: Correctly implements toolchain inclusion as rule inputs.The implementation properly merges the original rule dependencies with the extra implicit dependencies using a sorted set builder. This ensures deterministic ordering of the inputs in the proto output.
I'd recommend adding a brief comment explaining that
extraImplicitDepsrepresents toolchain dependencies:// Include explicit elements for all direct inputs and outputs of a rule; this goes beyond // what is available from the attributes above, since it may also (depending on options) // include implicit outputs, exec-configuration outputs, and default values. + // Also includes toolchain dependencies and other implicit deps not attached to any attribute ImmutableSortedSet<Label> ruleInputs = ImmutableSortedSet.<Label>naturalOrder() .addAll(rule.getSortedLabels(dependencyFilter)) .addAll(extraImplicitDeps) .build();
573-573: Correctly updates call site with empty set.This change correctly adapts the existing usage to the new method signature by passing an empty set of extra implicit dependencies, maintaining compatibility with existing code.
1-583:✅ Verification successful
Verify integration test coverage.
The changes look good overall. To ensure these changes work as expected, make sure there's an integration test that verifies toolchain dependencies are correctly included in the proto output.
🏁 Script executed:
#!/bin/bash # Description: Check for the presence of an integration test for toolchain dependencies in query output # Look for test cases related to toolchain rule inputs rg -A 10 "test.*toolchain.*rule.*input" --glob "*.sh" src/test/Length of output: 898
Integration test coverage verified
An existing integration test (
test_toolchain_rule_inputs) in
src/test/shell/integration/configured_query_test.shcovers toolchain rule inputs in the query output. No further action required.
These were already returned in a text output for a deps query - this adds them also as ruleInputs.
Fixes bazelbuild#19058
Summary by CodeRabbit
New Features
Tests