Introduce flags to allow filtering of targets by rule name #7
Conversation
This commit introduces two new flags: `--build_rule_filters` & `--test_rule_filters`. They behave in a very similar manner to existing filter flags, e.g. tag, timeout, size, lang, etc., and will allow users to filter out or in targets for building/testing that use a specific rule.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis change introduces rule name-based filtering for build and test targets across several components. New command-line options allow users to specify rule name filters for both build and test operations. Supporting methods and fields were added to core classes to parse, store, and apply these filters. The filtering logic is implemented analogously to existing tag-based filters, and the relevant predicates are applied during target pattern resolution and test filtering. Comprehensive tests were added or updated to validate the new rule filter functionality in both production and test code. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LoadingOptions
participant SkyframeExecutor
participant TargetPatternPhaseValue
participant TargetPatternPhaseFunction
participant TargetUtils
User->>LoadingOptions: Specify --build_rule_filters / --test_rule_filters
LoadingOptions->>SkyframeExecutor: buildRuleFilterList / testRuleFilterList
SkyframeExecutor->>TargetPatternPhaseValue: key(..., buildRuleFilterList)
TargetPatternPhaseValue->>TargetPatternPhaseFunction: getBuildRuleFilter()
TargetPatternPhaseFunction->>TargetUtils: ruleFilter(buildRuleFilterList)
TargetUtils-->>TargetPatternPhaseFunction: Predicate<Target>
TargetPatternPhaseFunction->>TargetPatternPhaseFunction: Filter ResolvedTargets with predicate
TargetPatternPhaseFunction-->>User: Filtered targets
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.31.1)src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java✨ 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 (
|
![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
🔭 Outside diff range comments (1)
src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java (1)
290-301:⚠️ Potential issueMissing buildRuleFilter in hashCode method
The buildRuleFilter field is included in the equals method but not in the hashCode method. This inconsistency can lead to bugs when the object is used in hash-based collections.
Add the buildRuleFilter field to the hashCode method:
@Override public int hashCode() { return Objects.hash( targetPatterns, offset, compileOneDependency, buildTestsOnly, determineTests, + buildTargetFilter, + buildRuleFilter, buildManualTests, expandTestSuites, testFilter); }
🧹 Nitpick comments (1)
src/test/java/com/google/devtools/build/lib/packages/TargetUtilsTest.java (1)
130-132: Fix comment to reference rules instead of tagsThe comment refers to "tag" but this test is about rule filtering, not tag filtering.
- // Applying same tag as positive and negative filter produces an empty + // Applying same rule name as positive and negative filter produces an empty // result because the negative filter is applied first and positive filter will // not match anything.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java(1 hunks)src/main/java/com/google/devtools/build/lib/packages/TestTargetUtils.java(1 hunks)src/main/java/com/google/devtools/build/lib/pkgcache/LoadingOptions.java(1 hunks)src/main/java/com/google/devtools/build/lib/pkgcache/TestFilter.java(6 hunks)src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java(1 hunks)src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java(1 hunks)src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java(8 hunks)src/test/java/com/google/devtools/build/lib/packages/TargetUtilsTest.java(1 hunks)src/test/java/com/google/devtools/build/lib/packages/TestTargetUtilsTest.java(1 hunks)src/test/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseKeyTest.java(12 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java (1)
src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java (1)
TargetUtils(44-455)
src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java (1)
src/main/java/com/google/devtools/build/lib/packages/TestTargetUtils.java (1)
TestTargetUtils(27-151)
🔇 Additional comments (20)
src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java (1)
370-373: Clean implementation of rule filteringThe addition of rule filtering alongside the existing tag filtering is well-implemented and follows the established pattern. This change correctly applies the rule filter predicate from
TargetUtils.ruleFilter()to the builder before constructing the finalResolvedTargets<Target>result.src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java (1)
3004-3005: Build rule filters correctly integratedThe change correctly adds support for rule-based filtering in the target pattern loading process, passing the newly introduced
buildRuleFilterListas an argument to theTargetPatternPhaseValue.keymethod. This follows the same pattern as the existing tag filters, maintaining consistency.src/main/java/com/google/devtools/build/lib/packages/TestTargetUtils.java (1)
94-104: Implementation looks good!The
testMatchesRuleFiltersmethod follows a clean and consistent pattern similar to the existing tag filtering methods. The logic is straightforward and correctly implements the rule filtering requirements.src/test/java/com/google/devtools/build/lib/packages/TargetUtilsTest.java (1)
99-139: Well-structured test for rule filtering functionalityThe test implementation thoroughly verifies the rule filtering functionality with various combinations of positive and negative rule name filters. It properly tests all the key scenarios including empty filters, single rule filters, negative filters, and combinations.
src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java (1)
384-405: Implementation looks good!The
ruleFiltermethod is well-implemented, following the same pattern as the existingtagFiltermethod. It correctly reuses thesortTagsBySenseutility method to separate positive and negative filters and returns a predicate that properly checks rule class names against the filter lists.src/main/java/com/google/devtools/build/lib/pkgcache/LoadingOptions.java (2)
131-145: Command-line option well definedThe
build_rule_filtersoption follows the established pattern of other filter options. The help text clearly explains its purpose and behavior.
146-159: Command-line option well definedThe
test_rule_filtersoption is well-defined and consistent with other test filter options. The help text clearly explains its purpose and behavior.src/test/java/com/google/devtools/build/lib/packages/TestTargetUtilsTest.java (2)
117-142: Comprehensive test coverage for language filteringGood addition of test methods to verify the functionality of language filtering. The test properly creates mock objects and validates both positive and negative language filters.
144-169: Well-structured test for rule-based filteringThis new test method correctly validates the rule filtering feature, ensuring that both positive and negative rule filters work as expected. The test setup follows the same pattern as the language filter test, which ensures consistency.
src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java (4)
158-159: Good addition of buildRuleFilter parameterThe parameter is correctly added to the key method signature and passed to the constructor.
Also applies to: 169-170
186-187: Proper update to keyWithoutFiltersThe keyWithoutFilters method is correctly updated to include an empty list for the new buildRuleFilter parameter.
199-200: Proper implementation of buildRuleFilterThe field is correctly added, initialized in the constructor, and a getter method is provided.
Also applies to: 211-212, 221-222, 259-261
317-318: Field included in equals methodThe buildRuleFilter field is correctly incorporated into the equals method.
src/main/java/com/google/devtools/build/lib/pkgcache/TestFilter.java (4)
49-51: Good addition of testRuleFilterListThe new field is properly added to the class, parameters, and constructor.
Also applies to: 57-58, 65-66, 71-72
85-87: Proper handling of rule filtersThe implementation correctly checks if the rule filter list is not empty and applies the filter.
99-100: Fields properly included in hashCode, equals, and toStringThe testRuleFilterList field is consistently incorporated into all object methods.
Also applies to: 113-115, 124-125
172-194: Well-implemented testRuleFilter methodThe implementation follows the same pattern as the testLangFilter method, properly separating required and excluded rules. The filtering logic is correct and consistent with the existing code style.
src/test/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseKeyTest.java (3)
56-57: Consistent update of test casesAll test cases are consistently updated to include the new buildRuleFilter parameter.
Also applies to: 66-67, 76-77, 86-87, 96-97, 106-107, 116-117, 126-127, 136-137, 145-146
156-157: Helper method correctly updatedThe helper method is properly updated to include the new parameter and pass it to the TargetPatternPhaseValue.key method.
Also applies to: 172-173
180-181: Simplified helper method updatedThe simplified helper method is correctly modified to include an empty list for the buildRuleFilter parameter.
Introduce two new flags to allow for filtering of targets based on rule name:
--build_rule_filters--test_rule_filtersSummary by CodeRabbit
New Features
Bug Fixes
Tests