Refactor: Migration to Version Catalog completed#1386
Conversation
|
Warning Rate limit exceeded@msridhar has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 7 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe PR removes the external Gradle script Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jdk-recent-unit-tests/build.gradle (1)
64-68: Incomplete migration:deps.versionsreferences remain and will cause build failure.The code still references
deps.versions.errorProneApianddeps.versions.errorProneLatest, but these are not defined in the version catalog. Two locations require updates:
jdk-recent-unit-tests/build.gradle:67buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle:72Add
errorProneLatesttogradle/libs.versions.tomlunder the[versions]section, then update both references to uselibs.versions.errorProneLatest.get()syntax.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (27)
build.gradle(4 hunks)gradle/dependencies.gradle(0 hunks)gradle/libs.versions.toml(1 hunks)guava-recent-unit-tests/build.gradle(2 hunks)jar-infer/jar-infer-cli/build.gradle(1 hunks)jar-infer/jar-infer-lib/build.gradle(1 hunks)jar-infer/nullaway-integration-test/build.gradle(1 hunks)jar-infer/test-android-lib-jarinfer/build.gradle(2 hunks)jar-infer/test-java-lib-jarinfer/build.gradle(1 hunks)jdk-annotations/astubx-generator/build.gradle(1 hunks)jdk-annotations/jdk-integration-test/build.gradle(1 hunks)jdk-annotations/test-annotated/build.gradle(1 hunks)jdk-annotations/test-unannotated/build.gradle(1 hunks)jdk-javac-plugin/build.gradle(1 hunks)jdk-recent-unit-tests/build.gradle(1 hunks)jmh/build.gradle(2 hunks)library-model/library-model-generator-integration-test/build.gradle(2 hunks)library-model/library-model-generator/build.gradle(2 hunks)library-model/test-library-model-generator/build.gradle(1 hunks)nullaway/build.gradle(6 hunks)sample-app/build.gradle(3 hunks)sample-library-model/build.gradle(1 hunks)sample/build.gradle(1 hunks)test-java-lib-lombok/build.gradle(3 hunks)test-java-lib/build.gradle(1 hunks)test-java-module/build.gradle(1 hunks)test-library-models/build.gradle(1 hunks)
💤 Files with no reviewable changes (1)
- gradle/dependencies.gradle
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
sample/build.gradlejdk-annotations/jdk-integration-test/build.gradletest-java-lib/build.gradlejar-infer/nullaway-integration-test/build.gradlejdk-javac-plugin/build.gradlejmh/build.gradlejdk-recent-unit-tests/build.gradlebuild.gradlelibrary-model/library-model-generator-integration-test/build.gradletest-java-lib-lombok/build.gradlejdk-annotations/test-unannotated/build.gradlejdk-annotations/test-annotated/build.gradleguava-recent-unit-tests/build.gradlenullaway/build.gradle
📚 Learning: 2025-11-25T22:43:06.446Z
Learnt from: CR
Repo: uber/NullAway PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T22:43:06.446Z
Learning: Run only the tests for the main NullAway module using `./gradlew :nullaway:test` unless specifically asked to run tests in a different module
Applied to files:
jdk-annotations/jdk-integration-test/build.gradletest-java-lib/build.gradlejar-infer/nullaway-integration-test/build.gradlejmh/build.gradlejdk-recent-unit-tests/build.gradlebuild.gradlelibrary-model/library-model-generator-integration-test/build.gradleguava-recent-unit-tests/build.gradlenullaway/build.gradle
📚 Learning: 2025-11-25T22:43:06.446Z
Learnt from: CR
Repo: uber/NullAway PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T22:43:06.446Z
Learning: Use the `--tests` flag with gradle to run specific test classes or methods, e.g., `./gradlew :nullaway:test --tests "com.uber.nullaway.NullAwayTest"`
Applied to files:
jdk-annotations/jdk-integration-test/build.gradlejdk-recent-unit-tests/build.gradlelibrary-model/library-model-generator-integration-test/build.gradle
📚 Learning: 2025-10-09T19:59:16.543Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1243
File: jdk-annotations/astubx-generator/build.gradle:22-22
Timestamp: 2025-10-09T19:59:16.543Z
Learning: When disabling testJdk17 tasks for modules requiring JDK 21, use `onlyIf { false }` to skip the task:
```gradle
tasks.named("testJdk17").configure {
onlyIf { false }
}
```
Do not use `doFirst { throw new GradleException(...) }` as it will cause CI failures when the task is executed.
Applied to files:
build.gradleguava-recent-unit-tests/build.gradlenullaway/build.gradle
📚 Learning: 2025-08-28T04:54:20.953Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Applied to files:
nullaway/build.gradle
🔇 Additional comments (31)
jdk-annotations/jdk-integration-test/build.gradle (1)
21-25: LGTM! Dependency coordinates migrated correctly.The test dependencies have been successfully migrated to the version catalog pattern with the junit exclusion preserved.
test-java-module/build.gradle (1)
22-22: LGTM! Dependency coordinate migrated correctly.The jspecify dependency has been successfully migrated to the version catalog pattern.
jdk-annotations/test-unannotated/build.gradle (1)
6-9: LGTM! Dependency coordinates migrated correctly.The dependencies have been successfully migrated to the version catalog pattern across compileOnly, annotationProcessor, and api configurations.
jmh/build.gradle (2)
43-44: LGTM! Dependency coordinates migrated correctly.The Error Prone Core and JSpecify dependencies have been successfully migrated to the version catalog pattern.
70-70: LGTM! Test dependency migrated correctly.The JUnit4 test dependency has been successfully migrated to the version catalog pattern.
jar-infer/nullaway-integration-test/build.gradle (1)
22-25: LGTM! Test dependencies migrated correctly.The test dependencies have been successfully migrated to the version catalog pattern with the junit exclusion preserved.
jar-infer/test-android-lib-jarinfer/build.gradle (2)
25-25: Verify version catalog entries for Android SDK versions.The Integer.parseInt pattern is the standard approach for using version catalog with Android SDK versions. Ensure that
libs.versions.min.sdkis properly defined ingradle/libs.versions.toml.
20-20: Version catalog entries are correctly defined.The Integer.parseInt pattern properly converts version catalog strings to integers. Both
compile-sdkandmin-sdkare defined ingradle/libs.versions.toml(as "30" and "16" respectively) and correctly referenced vialibs.versions.compile.sdkandlibs.versions.min.sdkin the build.gradle file.sample-library-model/build.gradle (1)
24-28: LGTM! Dependency coordinates migrated correctly.The dependencies have been successfully migrated to the version catalog pattern across compileOnly and annotationProcessor configurations.
library-model/library-model-generator-integration-test/build.gradle (1)
24-36: LGTM! All dependency coordinates migrated correctly.The dependencies have been successfully migrated to the version catalog pattern across all configurations (testImplementation, implementation, compileOnly, annotationProcessor) with the junit exclusion preserved.
jdk-recent-unit-tests/build.gradle (1)
36-44: LGTM! Dependencies migrated to version catalog.The test dependencies have been correctly updated to use the version catalog coordinates.
test-library-models/build.gradle (1)
24-37: LGTM! Clean migration to version catalog.All dependencies have been correctly migrated from
deps.*tolibs.*coordinates. The exclusion syntax remains properly formatted.guava-recent-unit-tests/build.gradle (1)
29-43: Verify hardcoded versions are intentional.Lines 38-39 use hardcoded version strings
"2.14.0"for Error Prone dependencies in theerrorProneOldestconfiguration. While this may be intentional for backward compatibility testing (as suggested by the configuration name and comments), consider whether these versions should be defined in the version catalog for consistency and maintainability.If these specific old versions need to remain hardcoded for testing purposes, consider adding a comment explaining why they differ from the catalog-managed versions.
jar-infer/test-java-lib-jarinfer/build.gradle (1)
68-73: LGTM! Dependencies correctly migrated.All dependency coordinates have been properly updated to use version catalog references.
jdk-annotations/test-annotated/build.gradle (1)
11-19: LGTM! Clean dependency migration.All dependencies have been correctly updated to use the version catalog coordinates.
test-java-lib/build.gradle (1)
23-30: LGTM! Dependencies migrated correctly.All visible dependency coordinates have been properly updated to use version catalog references.
jar-infer/jar-infer-lib/build.gradle (1)
27-43: LGTM! Comprehensive migration to version catalog.All dependencies including API, implementation, test, and runtime scopes have been correctly migrated to use version catalog coordinates. The use of
libs.bundles.walaproperly leverages the bundle feature for grouped dependencies.library-model/test-library-model-generator/build.gradle (1)
57-62: LGTM! Dependencies correctly updated.All dependency coordinates have been properly migrated to the version catalog pattern.
sample/build.gradle (1)
27-28: LGTM! Clean migration to version catalog.The dependency coordinates have been correctly updated from the legacy
deps.*pattern to the version cataloglibs.*pattern.gradle/libs.versions.toml (3)
52-54: Consider using integers for SDK versions to avoid repeated parsing.The Android SDK versions are defined as strings, requiring
Integer.parseInt()calls at every usage site (e.g., in sample-app/build.gradle). While this works, it adds boilerplate.However, Gradle Version Catalogs only support string values, so the current approach of storing SDK versions as strings and parsing them is the standard pattern for TOML-based catalogs.
1-129: Well-structured version catalog.The version catalog is properly organized with clear sections for versions, libraries, bundles, and plugins. The migration centralizes dependency management effectively.
11-11: Remove this comment.Android Gradle Plugin version 8.13.0 is a valid, released version available in Maven repositories. The version is not unusually high and does not require verification.
sample-app/build.gradle (2)
22-22: LGTM! Standard approach for SDK version parsing from version catalog.Using
Integer.parseInt()to convert string-based SDK versions from the TOML catalog is the correct pattern, as Gradle Version Catalogs only support string values.Also applies to: 27-28
43-49: LGTM! Dependencies correctly migrated to version catalog.All dependency coordinates have been properly updated from
deps.*tolibs.*pattern.jar-infer/jar-infer-cli/build.gradle (1)
12-20: LGTM! Dependencies correctly migrated with exclusions preserved.All dependency coordinates have been properly updated to version catalog references, and the JUnit exclusion for error-prone test helpers is correctly maintained.
test-java-lib-lombok/build.gradle (1)
25-30: LGTM! Lombok and other dependencies correctly migrated.All dependency coordinates have been properly updated to version catalog references, maintaining the correct configurations (annotationProcessor, api, compileOnly).
library-model/library-model-generator/build.gradle (1)
22-31: LGTM! All dependencies correctly migrated to version catalog.The dependency coordinates have been properly updated, including the correct mapping of JavaParser to its split modules (core and symbol-solver-core).
jdk-annotations/astubx-generator/build.gradle (1)
32-39: LGTM! Dependencies correctly migrated with exclusions preserved.All dependency coordinates have been properly updated to version catalog references, maintaining the JUnit exclusion in the error-prone test helpers dependency.
jdk-javac-plugin/build.gradle (1)
45-56: LGTM! All dependencies correctly migrated to version catalog.All dependency coordinates have been properly updated across implementation, testImplementation, and testModulePath configurations, with the JUnit exclusion correctly preserved.
build.gradle (1)
26-27: LGTM! Version catalog migration is correct.The migration from legacy
deps.*references to type-safelibs.*accessors is implemented correctly throughout this file. The buildscript dependencies, plugin aliases, and version accessors all follow standard Gradle Version Catalog conventions.Also applies to: 32-37, 64-64, 130-130, 147-147
nullaway/build.gradle (1)
36-84: LGTM! Dependency migrations are correct.All dependency declarations have been successfully migrated from
deps.*tolibs.*references. The hardcoded Error Prone 2.14.0 versions for theerrorProneOldestconfiguration are appropriate since they intentionally pin to a specific old version for compatibility testing.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jdk-recent-unit-tests/build.gradle (1)
64-71: Fix: Set errorProneVersionToCompileAgainst to the oldest Error Prone version.Both
errorProneVersionToCompileAgainstandlatestErrorProneVersionare currently set tolibs.versions.error.prone.get()(2.42.0). This makes the conditionerrorProneVersionToCompileAgainst == latestErrorProneVersionalways true, defeating the intended logic. The version catalog defineserror-prone-oldest = "2.14.0", indicating the build should test against multiple Error Prone versions. To properly skip tests on older versions when running on JDK 24+,errorProneVersionToCompileAgainstshould be set tolibs.versions.error.prone.oldest.get().
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
build.gradle(4 hunks)jdk-recent-unit-tests/build.gradle(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
jdk-recent-unit-tests/build.gradlebuild.gradle
📚 Learning: 2025-11-25T22:43:06.446Z
Learnt from: CR
Repo: uber/NullAway PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T22:43:06.446Z
Learning: Run only the tests for the main NullAway module using `./gradlew :nullaway:test` unless specifically asked to run tests in a different module
Applied to files:
jdk-recent-unit-tests/build.gradle
📚 Learning: 2025-11-25T22:43:06.446Z
Learnt from: CR
Repo: uber/NullAway PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T22:43:06.446Z
Learning: Use the `--tests` flag with gradle to run specific test classes or methods, e.g., `./gradlew :nullaway:test --tests "com.uber.nullaway.NullAwayTest"`
Applied to files:
jdk-recent-unit-tests/build.gradle
📚 Learning: 2025-10-09T19:59:16.543Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1243
File: jdk-annotations/astubx-generator/build.gradle:22-22
Timestamp: 2025-10-09T19:59:16.543Z
Learning: When disabling testJdk17 tasks for modules requiring JDK 21, use `onlyIf { false }` to skip the task:
```gradle
tasks.named("testJdk17").configure {
onlyIf { false }
}
```
Do not use `doFirst { throw new GradleException(...) }` as it will cause CI failures when the task is executed.
Applied to files:
jdk-recent-unit-tests/build.gradle
🔇 Additional comments (6)
jdk-recent-unit-tests/build.gradle (1)
34-46: LGTM!The dependency migrations from
deps.*to version catalog references (libs.*) are correct and follow the standard Version Catalog pattern. All test dependencies, including JUnit, AssertJ, Error Prone test helpers, and module path configurations, have been properly migrated.build.gradle (5)
19-29: LGTM!Buildscript classpath dependencies have been correctly migrated to version catalog references.
31-38: LGTM!Plugin declarations have been correctly migrated to version catalog aliases using the
alias(libs.plugins.*)syntax, with appropriateapply falseflags preserved.
40-43: Both Error Prone version properties are identical.Both
errorProneVersionToCompileAgainstandlatestErrorProneVersionare set to the same catalog version (libs.versions.error.prone.get()). This causes the conditional check injdk-recent-unit-tests/build.gradle(line 67) to always evaluate totrue, effectively disabling version-based test gating. Confirm this is the desired behavior if you've consolidated to a single Error Prone version across the project.
68-141: LGTM!Subprojects configuration has been correctly updated:
- Error Prone core dependency migrated to
libs.error.prone.core- Google Java Format version now retrieved from the version catalog
- All changes maintain the existing build logic while adopting version catalog references
143-158: LGTM!Spotless configuration correctly references the Google Java Format version from the version catalog in both the main
spotlessandspotlessPredeclareblocks.
|
Amazing contribution! Will review as soon as I can. Thank you! |
|
Don't worry about failure of spring CI job, that's a known issue |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1386 +/- ##
=========================================
Coverage 88.19% 88.19%
Complexity 2616 2616
=========================================
Files 95 95
Lines 8675 8675
Branches 1743 1743
=========================================
Hits 7651 7651
Misses 510 510
Partials 514 514 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
msridhar
left a comment
There was a problem hiding this comment.
Overall this looks great @vitinh0z. I appreciate you must have had to put a lot of effort into it, but I think it'll pay off for the project nicely. Thank you!
I have some questions so I can understand the changes a bit better, and a couple of minor comments.
|
|
||
| errorProneOldest deps.build.errorProneCheckApiOld | ||
| errorProneOldest(deps.build.errorProneTestHelpersOld) { | ||
| errorProneOldest "com.google.errorprone:error_prone_check_api:2.14.0" |
There was a problem hiding this comment.
Is there a way to keep the 2.14.0 version number in a variable somewhere so we don't have to repeat it across files?
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
gradle/libs.versions.toml
🔇 Additional comments (4)
gradle/libs.versions.toml (4)
53-113: Well-structured library definitions.The library declarations are properly organized with clear comments separating logical groups. All version references correctly map to keys defined in the
[versions]section, and the naming conventions are consistent throughout.
115-117: Bundles correctly reference defined libraries.Both bundles (
walaandjunit-jupiter) properly reference libraries defined in the[libraries]section, making it convenient to apply related dependencies together.
119-125: Plugin definitions are correct.All plugin declarations use the proper format and correctly reference version keys from the
[versions]section.
11-11: No action required. Version 8.13.0 is a valid, stable Android Gradle Plugin release.
| [versions] | ||
| asm = "9.3" | ||
| check-framework = "3.49.2" | ||
| support = "27.1.1" | ||
| wala = "1.6.12" | ||
| commons-cli = "1.4" | ||
| auto-value = "1.11.1" | ||
| auto-service = "1.1.1" | ||
| javaparser = "3.26.2" | ||
| google-java-format = "1.30.0" | ||
| android-gradle-plugin = "8.13.0" | ||
| gradle-maven-publish-plugin = "0.34.0" | ||
| spotless = "8.0.0" | ||
| errorprone-plugin = "4.3.0" | ||
| shadow = "8.3.8" | ||
| jmh = "0.7.1" | ||
| versions-plugin = "0.51.0" | ||
| shellcheck = "1.5.0" | ||
|
|
||
| errorProne = "2.42.0" | ||
|
|
||
| semver4j = "3.1.0" | ||
| jakarta-inject = "2.0.0" | ||
| javax-inject = "1" | ||
| gson = "2.13.1" | ||
| guava = "31.1-jre" | ||
| javax-validation = "2.0.1.Final" | ||
| jspecify = "1.0.0" | ||
| commons-io = "2.11.0" | ||
| junit4 = "4.13.2" | ||
| junit-jupiter = "5.14.0" | ||
| apiguardian = "1.0.0" | ||
| jetbrains-annotations = "24.1.0" | ||
| checker-compat-qual = "2.5.5" | ||
| rxjava2 = "2.1.2" | ||
| commons-lang3 = "3.8.1" | ||
| commons-lang = "2.6" | ||
| jsr305 = "3.0.2" | ||
| lombok = "1.18.38" | ||
| spring-beans = "5.3.7" | ||
| spring-context = "5.3.7" | ||
| grpc-core = "1.15.1" | ||
| mockito = "5.19.0" | ||
| javax-annotation = "1.3.2" | ||
| assertj = "3.23.1" | ||
| amazon-utils = "2.32.19" | ||
|
|
||
| # --- Android SDK Versions --- | ||
| compile-sdk = "30" | ||
| min-sdk = "16" | ||
| target-sdk = "30" | ||
|
|
||
| [libraries] | ||
| # --- Build Script Dependencies --- | ||
| semver4j = { module = "com.vdurmont:semver4j", version.ref = "semver4j" } | ||
| android-gradle-plugin = { module = "com.android.tools.build:gradle", version.ref = "android-gradle-plugin" } | ||
| gradle-maven-publish-plugin = { module = "com.vanniktech:gradle-maven-publish-plugin", version.ref = "gradle-maven-publish-plugin" } | ||
|
|
||
| # --- APT (Annotation Processing) --- | ||
| auto-value = { module = "com.google.auto.value:auto-value", version.ref = "auto-value" } | ||
| auto-value-annotations = { module = "com.google.auto.value:auto-value-annotations", version.ref = "auto-value" } | ||
| auto-service = { module = "com.google.auto.service:auto-service", version.ref = "auto-service" } | ||
| auto-service-annotations = { module = "com.google.auto.service:auto-service-annotations", version.ref = "auto-service" } | ||
| jakarta-inject = { module = "jakarta.inject:jakarta.inject-api", version.ref = "jakarta-inject" } | ||
| javax-inject = { module = "javax.inject:javax.inject", version.ref = "javax-inject" } | ||
|
|
||
| # --- Build / Implementation --- | ||
| asm = { module = "org.ow2.asm:asm", version.ref = "asm" } | ||
| asm-tree = { module = "org.ow2.asm:asm-tree", version.ref = "asm" } | ||
|
|
||
| # Error Prone Libraries | ||
| error-prone-check-api = { module = "com.google.errorprone:error_prone_check_api", version.ref = "errorProne" } | ||
| error-prone-core = { module = "com.google.errorprone:error_prone_core", version.ref = "errorProne" } | ||
| error-prone-test-helpers = { module = "com.google.errorprone:error_prone_test_helpers", version.ref = "errorProne" } | ||
|
|
||
| # Dataflow & Utils | ||
| checker-dataflow = { module = "org.checkerframework:dataflow-nullaway", version.ref = "check-framework" } | ||
| gson = { module = "com.google.code.gson:gson", version.ref = "gson" } | ||
| guava = { module = "com.google.guava:guava", version.ref = "guava" } | ||
| javaparser-core = { module = "com.github.javaparser:javaparser-core", version.ref = "javaparser" } | ||
| javaparser-symbol-solver-core = { module = "com.github.javaparser:javaparser-symbol-solver-core", version.ref = "javaparser" } | ||
| javax-validation = { module = "javax.validation:validation-api", version.ref = "javax-validation" } | ||
| jspecify = { module = "org.jspecify:jspecify", version.ref = "jspecify" } | ||
| commons-io = { module = "commons-io:commons-io", version.ref = "commons-io" } | ||
| commons-cli = { module = "commons-cli:commons-cli", version.ref = "commons-cli" } | ||
|
|
||
| # Wala Bundle | ||
| wala-util = { module = "com.ibm.wala:com.ibm.wala.util", version.ref = "wala" } | ||
| wala-shrike = { module = "com.ibm.wala:com.ibm.wala.shrike", version.ref = "wala" } | ||
| wala-core = { module = "com.ibm.wala:com.ibm.wala.core", version.ref = "wala" } | ||
|
|
||
| # --- Support --- | ||
| appcompat = { module = "com.android.support:appcompat-v7", version.ref = "support" } | ||
|
|
||
| # --- Test --- | ||
| junit4 = { module = "junit:junit", version.ref = "junit4" } | ||
| junit-jupiter-api = { module = "org.junit.jupiter:junit-jupiter-api", version.ref = "junit-jupiter" } | ||
| apiguardian = { module = "org.apiguardian:apiguardian-api", version.ref = "apiguardian" } | ||
| jetbrains-annotations = { module = "org.jetbrains:annotations", version.ref = "jetbrains-annotations" } | ||
| checker-qual = { module = "org.checkerframework:checker-qual", version.ref = "check-framework" } | ||
| checker-compat-qual = { module = "org.checkerframework:checker-compat-qual", version.ref = "checker-compat-qual" } | ||
| rxjava2 = { module = "io.reactivex.rxjava2:rxjava", version.ref = "rxjava2" } | ||
| commons-lang3 = { module = "org.apache.commons:commons-lang3", version.ref = "commons-lang3" } | ||
| commons-lang = { module = "commons-lang:commons-lang", version.ref = "commons-lang" } | ||
| jsr305 = { module = "com.google.code.findbugs:jsr305", version.ref = "jsr305" } | ||
| lombok = { module = "org.projectlombok:lombok", version.ref = "lombok" } | ||
| spring-beans = { module = "org.springframework:spring-beans", version.ref = "spring-beans" } | ||
| spring-context = { module = "org.springframework:spring-context", version.ref = "spring-context" } | ||
| grpc-core = { module = "io.grpc:grpc-core", version.ref = "grpc-core" } | ||
| mockito = { module = "org.mockito:mockito-core", version.ref = "mockito" } | ||
| javax-annotation = { module = "javax.annotation:javax.annotation-api", version.ref = "javax-annotation" } | ||
| assertj = { module = "org.assertj:assertj-core", version.ref = "assertj" } | ||
| amazon-utils = { module = "software.amazon.awssdk:utils", version.ref = "amazon-utils" } | ||
|
|
||
| [bundles] | ||
| wala = ["wala-util", "wala-shrike", "wala-core"] | ||
| junit-jupiter = ["junit-jupiter-api", "apiguardian"] | ||
|
|
||
| [plugins] | ||
| spotless = { id = "com.diffplug.spotless", version.ref = "spotless" } | ||
| errorprone = { id = "net.ltgt.errorprone", version.ref = "errorprone-plugin" } | ||
| shadow = { id = "com.gradleup.shadow", version.ref = "shadow" } | ||
| jmh = { id = "me.champeau.jmh", version.ref = "jmh" } | ||
| versions = { id = "com.github.ben-manes.versions", version.ref = "versions-plugin" } | ||
| shellcheck = { id = "com.felipefzdz.gradle.shellcheck", version.ref = "shellcheck" } No newline at end of file |
There was a problem hiding this comment.
Well-structured migration to Gradle Version Catalog with security considerations.
The file is well-organized following TOML conventions with clear section comments and consistent naming. The centralized approach will improve maintainability.
However, address the security vulnerability: Guava 31.1-jre has CVE-2023-2976 (Medium severity) related to temp directory creation on Unix systems. Upgrade to version 32.0.1 or later to resolve this. Error Prone 2.42.0 and Mockito 5.19.0 have no known vulnerabilities.
🤖 Prompt for AI Agents
In gradle/libs.versions.toml around lines 1 to 125, the Guava version is pinned
to "31.1-jre" which is vulnerable (CVE-2023-2976); update the guava version
entry in the [versions] section to "32.0.1" (or later) and keep the guava
library references unchanged so they use the new version.ref; save the file and
run a build (or dependency resolution) to verify no conflicts, bump further if
your build tool suggests a higher compatible patch release.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
gradle/libs.versions.toml (1)
27-27: Security vulnerability: Upgrade Guava to address CVE-2023-2976.Guava 31.1-jre has a known security vulnerability (CVE-2023-2976, Medium severity) related to temp directory creation on Unix systems. Upgrade to version 32.0.1 or later.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
build.gradlegradle/libs.versions.tomlguava-recent-unit-tests/build.gradlenullaway/build.gradle
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
guava-recent-unit-tests/build.gradlenullaway/build.gradle
📚 Learning: 2025-11-25T22:43:06.446Z
Learnt from: CR
Repo: uber/NullAway PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T22:43:06.446Z
Learning: Run only the tests for the main NullAway module using `./gradlew :nullaway:test` unless specifically asked to run tests in a different module
Applied to files:
guava-recent-unit-tests/build.gradlenullaway/build.gradle
📚 Learning: 2025-08-28T04:54:20.953Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Applied to files:
nullaway/build.gradle
🔇 Additional comments (10)
gradle/libs.versions.toml (1)
1-126: Well-structured version catalog with clear organization.The version catalog is well-organized with clear section comments and consistent naming conventions. The migration to centralized dependency management will improve maintainability across the multi-module project.
guava-recent-unit-tests/build.gradle (1)
31-43: LGTM: Clean migration to version catalog coordinates.The dependency declarations have been correctly migrated to use version catalog references (libs.*) while maintaining the original exclusions. The use of
${oldestErrorProneVersion}for Error Prone oldest dependencies and the hardcoded Guava version for testing newer Guava versions (line 36) aligns with the module's purpose.build.gradle (4)
26-27: LGTM: Buildscript dependencies migrated to version catalog.The buildscript dependencies have been correctly updated to reference the version catalog (libs.*) instead of hardcoded coordinates.
32-38: LGTM: Plugins migrated to version catalog aliases.The plugins block has been correctly migrated to use version catalog aliases with proper apply flags preserved.
72-72: LGTM: Error Prone dependency migrated to version catalog.The Error Prone core dependency has been correctly updated to use the version catalog reference.
138-138: LGTM: Google Java Format version sourced from catalog.The Google Java Format configuration has been correctly updated to retrieve the version from the version catalog, maintaining consistency across the project.
Also applies to: 155-155
nullaway/build.gradle (4)
36-49: LGTM: Compile dependencies migrated to version catalog.The compile-time dependencies (annotations, AutoValue, AutoService, JSpecify, etc.) have been correctly migrated to use version catalog references while preserving the original dependency scopes and configurations.
50-78: LGTM: Test dependencies migrated to version catalog.The test dependencies have been correctly migrated to use version catalog references, maintaining all original exclusions (e.g., junit exclusion on line 53). The migration includes proper use of bundles (e.g.,
libs.bundles.junit.jupiteron line 56).
80-84: LGTM: Error Prone oldest configuration uses centralized version variable.The errorProneOldest configuration correctly uses the
${oldestErrorProneVersion}variable (defined in root build.gradle), which addresses the maintainer's concern about repeating the version number across files.Based on learnings, this centralization improves maintainability.
156-156: Condition may always evaluate to true given current root configuration.This condition compares
errorProneVersionToCompileAgainst == latestErrorProneVersion, but in the root build.gradle (lines 40-44), both variables are set to the same value (libs.versions.errorProne.get()). Unless there's a plan to make these configurable or different in certain build scenarios, this condition will always be true.If the variables are intended to be identical, consider simplifying the logic or documenting the rationale for keeping separate variables.
|
Sorry for not having fixed it. I had to do other things and there was no time to make the changes to the PR |
No problem! I'm doing a bit of cleanup and then I think this will be good to go |
msridhar
left a comment
There was a problem hiding this comment.
Thanks again for the great contribution!
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
build.gradlebuildSrc/src/main/groovy/nullaway.java-test-conventions.gradlejdk-recent-unit-tests/build.gradlenullaway/build.gradle
💤 Files with no reviewable changes (1)
- buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
jdk-recent-unit-tests/build.gradlenullaway/build.gradle
📚 Learning: 2025-11-25T22:43:06.446Z
Learnt from: CR
Repo: uber/NullAway PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T22:43:06.446Z
Learning: Run only the tests for the main NullAway module using `./gradlew :nullaway:test` unless specifically asked to run tests in a different module
Applied to files:
jdk-recent-unit-tests/build.gradlenullaway/build.gradle
📚 Learning: 2025-11-25T22:43:06.446Z
Learnt from: CR
Repo: uber/NullAway PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T22:43:06.446Z
Learning: Use the `--tests` flag with gradle to run specific test classes or methods, e.g., `./gradlew :nullaway:test --tests "com.uber.nullaway.NullAwayTest"`
Applied to files:
jdk-recent-unit-tests/build.gradle
📚 Learning: 2025-10-09T19:59:16.543Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1243
File: jdk-annotations/astubx-generator/build.gradle:22-22
Timestamp: 2025-10-09T19:59:16.543Z
Learning: When disabling testJdk17 tasks for modules requiring JDK 21, use `onlyIf { false }` to skip the task:
```gradle
tasks.named("testJdk17").configure {
onlyIf { false }
}
```
Do not use `doFirst { throw new GradleException(...) }` as it will cause CI failures when the task is executed.
Applied to files:
jdk-recent-unit-tests/build.gradlenullaway/build.gradle
📚 Learning: 2025-08-28T04:54:20.953Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Applied to files:
nullaway/build.gradle
🔇 Additional comments (6)
jdk-recent-unit-tests/build.gradle (1)
34-46: LGTM! Clean migration to version catalog.All test dependencies have been correctly migrated from
deps.*tolibs.*coordinates. The JUnit exclusion clause is properly preserved, and the use oflibs.bundles.junit.jupiterfollows best practices for dependency bundles.build.gradle (3)
19-38: LGTM! Buildscript and plugins correctly migrated.The buildscript dependencies and plugins block have been properly migrated to use version catalog references. The
alias()syntax is correct for plugin declarations, and theapply falseflags are appropriately preserved.
40-42: LGTM! Version variable properly exposed.The
oldestErrorProneVersionvariable is correctly extracted from the version catalog and exposed for use by subprojects (notably innullaway/build.gradleline 80). The previous review concern about redundant version variables no longer applies to this version of the code.
67-157: Version catalog references are correctly configured.The Error Prone core dependency and Google Java Format version are properly defined in the version catalog at
gradle/libs.versions.toml, and all references in the build script are accurate.nullaway/build.gradle (2)
34-48: LGTM! Compile dependencies correctly migrated.All compile-time dependencies have been properly migrated to version catalog references while preserving the appropriate dependency scopes (
compileOnly,api,implementation,annotationProcessor).
80-84: Error Prone oldest version configuration is correct.The
errorProneOldestconfiguration correctly uses string interpolation with${oldestErrorProneVersion}(defined in root build.gradle line 41), preserves the JUnit exclusion, and referenceslibs.guavafrom the version catalog. The mixing of explicit coordinates with parameterized versions and catalog references is appropriate here, with the version catalog properly defining errorProneOldest as "2.14.0" and errorProne as "2.42.0".
| testImplementation project(":annotations") | ||
| testImplementation deps.test.junit4 | ||
| testImplementation(deps.build.errorProneTestHelpers) { | ||
| testImplementation libs.junit4 | ||
| testImplementation(libs.error.prone.test.helpers) { | ||
| exclude group: "junit", module: "junit" | ||
| } | ||
| testImplementation deps.test.jetbrainsAnnotations | ||
| testImplementation deps.test.junit5Jupiter | ||
| testImplementation deps.test.cfQual | ||
| testImplementation deps.test.cfCompatQual | ||
| testImplementation deps.build.jspecify | ||
| testImplementation libs.jetbrains.annotations | ||
| testImplementation libs.bundles.junit.jupiter | ||
| testImplementation libs.checker.qual | ||
| testImplementation libs.checker.compat.qual | ||
| testImplementation libs.jspecify | ||
| testImplementation project(":test-java-lib") | ||
| testImplementation deps.apt.jakartaInject | ||
| testImplementation deps.apt.javaxInject | ||
| testImplementation deps.test.rxjava2 | ||
| testImplementation deps.test.commonsLang | ||
| testImplementation deps.test.commonsLang3 | ||
| testImplementation libs.jakarta.inject | ||
| testImplementation libs.javax.inject | ||
| testImplementation libs.rxjava2 | ||
| testImplementation libs.commons.lang | ||
| testImplementation libs.commons.lang3 | ||
| testImplementation project(":test-library-models") | ||
| testImplementation deps.test.lombok | ||
| testImplementation deps.test.springBeans | ||
| testImplementation deps.test.springContext | ||
| testImplementation deps.test.grpcCore | ||
| testImplementation libs.lombok | ||
| testImplementation libs.spring.beans | ||
| testImplementation libs.spring.context | ||
| testImplementation libs.grpc.core | ||
| testImplementation project(":test-java-lib-lombok") | ||
| testImplementation deps.test.mockito | ||
| testImplementation deps.test.javaxAnnotationApi | ||
| testImplementation deps.test.assertJ | ||
| testImplementation deps.test.amazonUtils | ||
| testImplementation libs.mockito | ||
| testImplementation libs.javax.annotation | ||
| testImplementation libs.assertj | ||
| testImplementation libs.amazon.utils | ||
| // This is for a test exposing a CFG construction failure in the Checker Framework. We can probably remove it once |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Address hardcoded Spark dependency for consistency.
The test dependencies have been successfully migrated to version catalog references with proper exclusions preserved. However, line 78 contains a hardcoded dependency that breaks the migration pattern:
testImplementation 'org.apache.spark:spark-sql_2.12:3.3.2'While the comment indicates this is temporary, for consistency with the version catalog migration, consider adding this to gradle/libs.versions.toml:
🔎 Suggested addition to gradle/libs.versions.toml
Add to the [versions] section:
spark-sql = "3.3.2"Add to the [libraries] section:
spark-sql = { group = "org.apache.spark", name = "spark-sql_2.12", version.ref = "spark-sql" }Then update line 78:
- testImplementation 'org.apache.spark:spark-sql_2.12:3.3.2'
+ testImplementation libs.spark.sqlThis maintains consistency with the migration goals while still documenting the dependency as temporary via the existing comment.
🤖 Prompt for AI Agents
In nullaway/build.gradle around lines 50-78 there is a hardcoded Spark
dependency that breaks the version-catalog migration; add a spark-sql entry to
gradle/libs.versions.toml (a version key and a libraries entry using
version.ref) and replace the hardcoded testImplementation
'org.apache.spark:spark-sql_2.12:3.3.2' with the catalog reference (e.g.,
testImplementation libs.spark-sql) keeping the existing comment that marks the
dependency as temporary so exclusions/intent are preserved.
Thank you for contributing to NullAway!
Please note that once you click "Create Pull Request" you will be asked to sign our Uber Contributor License Agreement via CLA assistant.
Before pressing the "Create Pull Request" button, please provide the following:
A description about what and why you are contributing, even if it's trivial.
The issue number(s) or PR number(s) in the description if you are contributing in response to those.
If applicable, unit tests.
Description
This PR modernizes the project's dependency management by migrating from the legacy
dependencies.gradle(Groovy Map) pattern to the standard Gradle Version Catalog (libs.versions.toml).Fixes #870
As a consequence, rips out the
epApiVersionfunctionality for setting the Error Prone API version as a build-time flag to Gradle; we weren't using this anymore anyway. If we want it in the future we should re-implement it in our proper build config.Changes made:
gradle/libs.versions.tomlwith all project versions, libraries, plugins, and bundles.build.gradlefiles (root, core, android samples, jar-infer, etc.) to use type-safe accessors (libs.*) instead of thedepsobject.gradle/dependencies.gradlefile.Why:
Validation
./gradlew clean buildlocally to ensure all modules compile and tests pass with the new configuration.sample-app,jar-infer) configure correctly with the new TOML-based versioning.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.