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
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.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces new flags --build_rule_filters and --test_rule_filters to allow filtering targets by rule name. The changes seem well-structured and include new tests to validate the functionality. However, there are a few areas that could be improved for clarity and correctness.
Summary of Findings
- Missing Null Check: In
TargetUtils.ruleFilter, the code doesn't explicitly handle the case whereruleFilterListis null. Adding a null check would improve the robustness of the function. - Inconsistent Naming: The naming of variables and methods related to rule filtering could be more consistent. For example, using
ruleNamein one place andruleClassin another for the same concept can be confusing. - Potential Performance Issue: In
TestFilter.testRuleFilter, usingHashSetmight not be the most efficient choice if the number of filters is very large. Consider using a more efficient data structure if performance becomes an issue.
Merge Readiness
The pull request introduces valuable functionality with the new rule filtering flags. However, addressing the identified issues, particularly the missing null check and potential performance concern, would enhance the code's robustness and maintainability. I am unable to approve this pull request, and recommend that the author address the high severity issue before merging. Other reviewers should also review and approve this code before merging.
| * Returns a predicate to be used for test rule name filtering, i.e., that only accepts tests that match | ||
| * a required rule name and not an excluded rule name. | ||
| */ | ||
| public static Predicate<Target> ruleFilter(List<String> ruleFilterList) { |
There was a problem hiding this comment.
Consider adding a null check for ruleFilterList to prevent potential NullPointerException if the list is not initialized or passed as null.
if (ruleFilterList == null) {
return input -> true; // Or handle the null case appropriately
}| return TestTargetUtils.testMatchesRuleFilters( | ||
| ((Rule) input).getRuleClass(), requiredRules, excludedRules); |
There was a problem hiding this comment.
For consistency, consider using ruleName instead of ruleClass here, since getRuleClass() returns the rule name. This would align with the naming in TestTargetUtils.testMatchesRuleFilters.
return TestTargetUtils.testMatchesRuleFilters(
((Rule) input).getRuleClass(), requiredRules, excludedRules);| * the specified rule names. | ||
| */ | ||
| private static Predicate<Target> testRuleFilter(List<String> ruleFilterList) { | ||
| final Set<String> requiredRules = new HashSet<>(); |
There was a problem hiding this comment.
If the number of rule filters is expected to be large, consider using a more efficient data structure than HashSet for requiredRules and excludedRules, such as ImmutableSet or a Bloom filter, to improve performance.
| final Set<String> requiredRules = new HashSet<>(); | |
| final Set<String> requiredRules = new HashSet<>(); |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java (1)
2998-3008: 💡 Verification agent❓ Verification inconclusive
Pass new
buildRuleFilterListinto the target‐pattern key
The insertion ofImmutableList.copyOf(options.buildRuleFilterList)into the call to
TargetPatternPhaseValue.key(...)correctly wires up the rule‐name filters alongside the existing tag filters. Please verify that the correspondingTargetPatternPhaseKey(and itsequals/hashCode) has been updated to include this new parameter, and thatTargetPatternPhaseKeyTestcovers it to avoid cache key mismatches.You can confirm coverage by searching for
buildRuleFilterin the key tests:
🏁 Script executed:
rg -A5 "buildRuleFilterList" -e "TargetPatternPhaseKeyTest"Length of output: 117
Add
buildRuleFilterListtoTargetPatternPhaseKeyequals/hashCode and tests
I ranrg -A5 "buildRuleFilterList" -e "TargetPatternPhaseKeyTest"and didn’t find any existing coverage. Please ensure that:
- The
TargetPatternPhaseKeyclass (src/main/java/.../skyframe/TargetPatternPhaseKey.java)
– Declares a field forbuildRuleFilterList
– Includes it in the constructor,equals(), andhashCode()- You add unit tests in
TargetPatternPhaseKeyTest(src/test/java/.../skyframe/TargetPatternPhaseKeyTest.java) that verify two keys with differingbuildRuleFilterListvalues are not equal.src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java (1)
291-301:⚠️ Potential issueMissing buildRuleFilter in hashCode calculation.
The hashCode method doesn't include the buildRuleFilter field, which should be added to ensure consistency with the equals method.
@Override public int hashCode() { return Objects.hash( targetPatterns, offset, compileOneDependency, buildTestsOnly, determineTests, + buildTargetFilter, + buildRuleFilter, buildManualTests, expandTestSuites, testFilter); }
🧹 Nitpick comments (4)
src/main/java/com/google/devtools/build/lib/pkgcache/LoadingOptions.java (1)
142-142: Fix grammatical error in help text.There's a grammatical error in the help text: "those are be governed" should be either "those are governed" or "those will be governed".
- + "does not affect the set of tests executed with the 'test' command; those are be " + + "does not affect the set of tests executed with the 'test' command; those are "src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java (1)
276-288: Missing buildRuleFilter in toString method.The toString method doesn't include the buildRuleFilter field, which would be helpful for debugging.
@Override public String toString() { StringBuilder result = new StringBuilder(); result.append(targetPatterns); if (!offset.isEmpty()) { result.append(" OFFSET=").append(offset); } result.append(compileOneDependency ? " COMPILE_ONE_DEPENDENCY" : ""); result.append(buildTestsOnly ? " BUILD_TESTS_ONLY" : ""); result.append(determineTests ? " DETERMINE_TESTS" : ""); + if (!buildRuleFilter.isEmpty()) { + result.append(" BUILD_RULE_FILTER=").append(buildRuleFilter); + } result.append(expandTestSuites ? " EXPAND_TEST_SUITES" : ""); result.append(testFilter != null ? " " + testFilter : ""); return result.toString(); }src/main/java/com/google/devtools/build/lib/pkgcache/TestFilter.java (2)
172-194: Consider reducing code duplication in filter methods.The implementation of
testRuleFilteris very similar totestLangFilter. Consider refactoring to extract a common method for creating filter predicates with inclusion/exclusion logic.You could create a helper method like:
private static Predicate<Target> createFilterPredicate( List<String> filterList, java.util.function.Function<Target, String> valueExtractor) { final Set<String> requiredValues = new HashSet<>(); final Set<String> excludedValues = new HashSet<>(); for (String filterValue : filterList) { if (filterValue.startsWith("-")) { filterValue = filterValue.substring(1); excludedValues.add(filterValue); } else { requiredValues.add(filterValue); } } return rule -> { String value = valueExtractor.apply(rule); return (requiredValues.isEmpty() || requiredValues.contains(value)) && !excludedValues.contains(value); }; }Then use it in both methods:
private static Predicate<Target> testLangFilter(List<String> langFilterList) { return createFilterPredicate(langFilterList, TargetUtils::getRuleLanguage); } private static Predicate<Target> testRuleFilter(List<String> ruleFilterList) { return createFilterPredicate(ruleFilterList, Target::getRuleClass); }
172-194: Add JavaDoc to the testRuleFilter method.Add JavaDoc to the
testRuleFiltermethod to be consistent with other methods in the class./** * Returns a predicate to be used for test rule filtering, i.e., that only accepts tests of * the specified rule names. + * + * @param ruleFilterList list of rule names to filter by (names with a "-" prefix are excluded) + * @return predicate that filters targets by rule name */ private static Predicate<Target> testRuleFilter(List<String> ruleFilterList) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 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/packages/TargetUtils.java (1)
src/main/java/com/google/devtools/build/lib/packages/TestTargetUtils.java (1)
TestTargetUtils(27-151)
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)
🔇 Additional comments (27)
src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java (1)
370-372: Adding rule-based filtering for targetsThe code now filters targets by both tags and rule names. This change complements the existing tag filtering by adding the ability to filter targets based on their rule class names.
src/main/java/com/google/devtools/build/lib/packages/TestTargetUtils.java (1)
94-104: Well-implemented rule name matching logicThe implementation is concise and follows the same pattern as the existing tag filtering methods. The logic correctly handles both required and excluded rule names, returning true when:
- The rule name is in the required list (or required list is empty)
- AND the rule name is not in the excluded list
src/test/java/com/google/devtools/build/lib/packages/TargetUtilsTest.java (1)
99-139: Comprehensive test coverage for rule filteringThis test provides thorough coverage of the rule filtering functionality, testing:
- Empty filter lists
- Multiple positive filters
- Single positive filter
- Negative filters
- Combination of positive and negative filters
- Edge case where the same rule is both required and excluded
The test structure mirrors the existing
testFilterByTagmethod, maintaining consistency in the test suite.src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java (1)
384-405: Well-designed rule filter implementationThe implementation follows the same pattern as the existing
tagFiltermethod, ensuring consistency in the codebase. The predicate correctly handles:
- Empty filter lists (accepts all targets)
- Non-Rule targets (accepts only if no required rules)
- Rule targets (delegates to
TestTargetUtils.testMatchesRuleFilters)This consistent approach makes the code more maintainable and easier to understand.
src/main/java/com/google/devtools/build/lib/pkgcache/LoadingOptions.java (2)
131-144: Looks good: Implementation of thebuild_rule_filtersoption.The implementation of the build rule filters option is well-structured and follows the established pattern of other filter options in this class.
146-158: Looks good: Implementation of thetest_rule_filtersoption.The implementation of the test rule filters option is well-structured and follows the established pattern of other filter options in this class.
src/test/java/com/google/devtools/build/lib/packages/TestTargetUtilsTest.java (2)
117-142: Good test coverage for language filtering.The test properly initializes the TestFilter with various options and validates that the filter correctly includes or excludes rules based on language.
144-169: Good test coverage for rule name filtering.The test properly covers both positive and negative rule filtering, ensuring that the TestFilter correctly includes or excludes rules based on their names.
src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java (8)
158-159: Looks good: Added buildRuleFilter parameter to key method.The buildRuleFilter parameter has been properly added to the key method, enabling rule-based filtering for build targets.
169-170: Looks good: Updated constructor call with buildRuleFilter parameter.The buildRuleFilter parameter has been properly passed to the constructor.
186-187: Looks good: Updated keyWithoutFilters with buildRuleFilter parameter.The keyWithoutFilters method now correctly passes an empty rule filter list.
199-200: Looks good: Added buildRuleFilter field to TargetPatternPhaseKey.The buildRuleFilter field has been properly added to the TargetPatternPhaseKey class.
211-212: Looks good: Updated constructor to initialize buildRuleFilter field.The constructor now properly initializes the buildRuleFilter field with validation.
221-222: Looks good: Added Preconditions check for buildRuleFilter.The buildRuleFilter field is properly validated with Preconditions.checkNotNull.
259-261: Looks good: Added getter for buildRuleFilter.The getter method for buildRuleFilter is correctly implemented.
317-318: Looks good: Added buildRuleFilter to equals method.The equals method now properly compares the buildRuleFilter field.
src/main/java/com/google/devtools/build/lib/pkgcache/TestFilter.java (8)
49-50: Looks good: Updated forOptions method to include rule filters.The static factory method has been properly updated to include the test rule filters.
57-58: Looks good: Added testRuleFilterList field.The testRuleFilterList field has been properly added to the TestFilter class.
65-66: Looks good: Updated constructor parameter list.The constructor now properly accepts the test rule filters parameter.
71-72: Looks good: Updated field initialization in constructor.The constructor now properly initializes the testRuleFilterList field.
85-87: Looks good: Added rule filter to the predicate chain.The implementation now correctly incorporates the rule filter into the combined predicate if the rule filter list is not empty.
98-100: Looks good: Updated hashCode to include testRuleFilterList.The hashCode method now properly includes the testRuleFilterList field.
113-115: Looks good: Updated equals to include testRuleFilterList.The equals method now properly compares the testRuleFilterList field.
124-125: Looks good: Updated toString to include testRuleFilterList.The toString method now properly includes the testRuleFilterList field.
src/test/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseKeyTest.java (3)
56-56: The new buildRuleFilter parameter is correctly integrated into the test cases.The changes appropriately add the new
buildRuleFilterparameter to all test cases, maintaining parity with the existingbuildTagFilterparameter. This ensures that the equality testing forTargetPatternPhaseKeynow accounts for the new field.Also applies to: 66-66, 76-76, 86-86, 96-96, 106-106, 116-116, 126-126, 136-136, 146-146
156-156: Method signature properly updated to include the buildRuleFilter parameter.The
ofhelper method now correctly accepts the newbuildRuleFilterparameter and passes it to theTargetPatternPhaseValue.keymethod, ensuring that test cases can properly test equality with the new field.Also applies to: 172-172
180-180: Simplified helper method correctly forwards the buildRuleFilter parameter.The overloaded version of the
ofmethod now properly forwards an empty rule filter to the primary implementation, maintaining consistency with how it handles other parameters.
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