-
Notifications
You must be signed in to change notification settings - Fork 328
Fail build on JDK 21 versions before 21.0.8 #1261
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdds a configuration-time guard in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant Gradle as Gradle (configuration)
participant JRE as Runtime.version()
Dev->>Gradle: gradle build
Gradle->>JRE: Runtime.version() → feature(), update()
alt JDK 21 and update < 8
Gradle-->>Dev: Throw GradleException ("Require Java 21.0.8 or newer")
else Not JDK 21 or update ≥ 8
Gradle-->>Dev: Continue evaluating subprojects
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
build.gradle (1)
48-50: Consider adding a reference to the related issue for context.The error message is clear and actionable. Consider adding a reference to issue #1245 to provide additional context for developers encountering this error.
throw new GradleException( - "Building NullAway on JDK 21 requires at least 21.0.8, but found ${runtimeVersion}. Please upgrade to 21.0.8 or later, or to the most recent JDK release." + "Building NullAway on JDK 21 requires at least 21.0.8, but found ${runtimeVersion}. Please upgrade to 21.0.8 or later, or to the most recent JDK release. See https://github.com/uber/NullAway/issues/1245 for more details." )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
build.gradle(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: msridhar
PR: uber/NullAway#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.
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
PR: uber/NullAway#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:
build.gradle
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: JDK 17 on windows-latest with Error Prone 2.41.0
- GitHub Check: Build caffeine with snapshot
- GitHub Check: Build spring-framework with snapshot
🔇 Additional comments (2)
build.gradle (2)
45-52: LGTM! Well-implemented version check with clear error messaging.The implementation correctly validates the JDK 21 update version and provides a helpful error message. The check is placed appropriately before the subprojects block to prevent any configuration issues downstream.
45-52: Verify JDK 21 version-check logic handles all edge casesI didn’t find any other JDK-version guards or CI overrides in the repo beyond the snippet in build.gradle (lines 45–52). To be confident this only rejects 21.0.{0–7} while accepting 21.0.8+ (and doesn’t misfire on “21-ea” or “21.1.0+9”), please:
• Add unit tests (or a small SmokeTest) exercising Runtime.version() against version strings like:
– “21”
– “21.0.7”
– “21.0.8”
– “21.0.8+9”
– “21.1.0”
– “21+9”
– “21-ea”
• Confirm that feature()==21 and update() returns the numeric patch level in all these cases.
• If Runtime.Version.parse behavior changes in future JDK-21.x/22, adjust the guard accordingly.Locations to review:
- build.gradle lines 45–52 (the existing feature/update check)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1261 +/- ##
=========================================
Coverage 88.17% 88.17%
Complexity 2427 2427
=========================================
Files 92 92
Lines 8010 8010
Branches 1595 1595
=========================================
Hits 7063 7063
Misses 492 492
Partials 455 455 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
build.gradle (1)
45-52: Minor simplification and tweak to avoid redundancy and magic number.The inner
feature() == 21check makes the outerJavaVersion.current() == VERSION_21redundant. Also, name the minimum update to clarify intent and ease future bumps.Apply:
-if (JavaVersion.current() == JavaVersion.VERSION_21) { - def runtimeVersion = Runtime.version() - if (runtimeVersion.feature() == 21 && runtimeVersion.update() < 8) { - throw new GradleException( - "Building NullAway on JDK 21 requires at least 21.0.8, but found ${runtimeVersion}. Please upgrade to 21.0.8 or later, or to the most recent JDK release." - ) - } -} +def runtimeVersion = Runtime.version() +if (runtimeVersion.feature() == 21) { + def minUpdate = 8 + if (runtimeVersion.update() < minUpdate) { + throw new GradleException( + "Building NullAway on JDK 21 requires at least 21.0.${minUpdate}, but found ${runtimeVersion}. Please upgrade to 21.0.${minUpdate} or later, or use the most recent JDK release." + ) + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
build.gradle(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: msridhar
PR: uber/NullAway#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.
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
PR: uber/NullAway#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:
build.gradle
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: JDK 17 on windows-latest with Error Prone 2.41.0
- GitHub Check: Build spring-framework with snapshot
- GitHub Check: Build caffeine with snapshot
🔇 Additional comments (1)
build.gradle (1)
45-52: Early, configuration-time JDK guard is appropriate and aligns with PR intent.This will fail fast on unsupported 21.x runtimes and avoids confusing test failures. Placement before subprojects ensures a single, clear error.
Some JSpecify tests now fail on earlier JDK 21 versions (see #1245). To avoid confusion, fail the whole build if one of these versions is being used.
Summary by CodeRabbit