Upgrade to gradle 8.6 #3056
Upgrade to gradle 8.6 #3056
Conversation
WalkthroughThis update primarily focuses on upgrading the project to Gradle 8.x, enhancing build scripts for better compatibility with newer Java versions, and improving the code quality tools. The changes dynamically set JVM targets to align with the current Java version, update the Gradle wrapper, and refine scripts for better performance and compliance. Additionally, there's an adjustment in dependency management logic and plugin configurations to streamline the build process. Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
@vlsi Please, could you review? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
gradle/wrapper/gradle-wrapper.jaris excluded by:!**/*.jar
Files selected for processing (12)
- build-logic/code-quality/build.gradle.kts (1 hunks)
- build-logic/jvm/build.gradle.kts (1 hunks)
- build-logic/jvm/src/main/kotlin/testng.java.gradle.kts (1 hunks)
- build-logic/jvm/src/main/kotlin/testng.kotlin-library.gradle.kts (1 hunks)
- build-logic/publishing/build.gradle.kts (1 hunks)
- build-logic/publishing/src/main/kotlin/buildlogic/FirstLayerDependencies.kt (2 hunks)
- gradle.properties (2 hunks)
- gradle/wrapper/gradle-wrapper.properties (1 hunks)
- gradlew (4 hunks)
- gradlew.bat (4 hunks)
- settings.gradle.kts (2 hunks)
- testng-test-osgi/testng-test-osgi-build.gradle.kts (2 hunks)
Files skipped from review due to trivial changes (1)
- build-logic/jvm/src/main/kotlin/testng.kotlin-library.gradle.kts
Additional comments: 20
gradle/wrapper/gradle-wrapper.properties (3)
- 2-2: Ensure the new
distributionPathsetting aligns with the project's directory structure and Gradle's conventions.- 3-3: The update to
distributionUrlfor Gradle 8.5 is correct and follows the standard format for Gradle distribution URLs.- 4-4: Adding
networkTimeout=10000is a good practice to avoid build failures due to network issues. Verify that this timeout value suits the project's needs.build-logic/code-quality/build.gradle.kts (2)
- 13-13: Updating
autostyle-plugin-gradleto version 4.0 should be verified for compatibility with the project's code style configurations.- 18-18: Dynamically setting
jvmTargetto match the current Java version is a good practice for ensuring compatibility.build-logic/jvm/build.gradle.kts (1)
- 20-20: Dynamically setting
jvmTargetto the current Java version's major version is a good practice.build-logic/publishing/build.gradle.kts (1)
- 20-20: Consistent use of dynamic
jvmTargetsetting across modules is a good practice for Java version compatibility.gradle.properties (1)
- 24-26: Adding
systemProp.sonar.gradle.skipCompile=trueto address a Gradle warning with SonarQube plugin version 4.x is a targeted fix. Ensure this property is compatible with the project's SonarQube integration.settings.gradle.kts (2)
- 10-10: Adding the
org.gradle.toolchains.foojay-resolver-conventionplugin at version "0.5.0" could enhance Java toolchain resolution. Verify its integration and benefits for the project.- 10-10: Ensure the removal of
enableFeaturePreview("VERSION_CATALOGS")aligns with Gradle 8.5's capabilities and the project's dependency management strategy.build-logic/jvm/src/main/kotlin/testng.java.gradle.kts (1)
- 12-14: Dynamically setting the Java toolchain language version to the current Java version's major version is a good practice for ensuring compatibility.
gradlew.bat (3)
- 17-17: Adjusting the handling of the
%DEBUG%environment variable with@echo offconditionally improves script readability and debugging experience.- 28-29: Setting
DIRNAMEto.if it's empty ensures a fallback directory is always defined, improving the script's reliability in various execution contexts.- 44-44: The check for
java.exein the PATH and the subsequent error message if not found are crucial for early detection of configuration issues. Ensure the error messaging is clear and actionable.testng-test-osgi/testng-test-osgi-build.gradle.kts (1)
- 52-52: Handling potential null values for the
typefield in thegenerateDependenciesPropertiestask improves the script's robustness.build-logic/publishing/src/main/kotlin/buildlogic/FirstLayerDependencies.kt (1)
- 61-62: Refining
usageKindcalculation usingreplaceFirstCharimproves clarity and correctness. Verify this change aligns with the project's dependency resolution strategy.gradlew (4)
- 1-1: Adding a shebang line
#!/bin/shat the beginning of the script ensures it is executed with a POSIX-compliant shell, enhancing portability and compliance.- 68-80: Improving symlink handling with a loop ensures that the script correctly resolves the actual path of the script file, enhancing reliability in environments where symlinks are common.
- 170-175: Adjusting paths to Windows format for Cygwin or MSYS environments ensures compatibility and correct behavior of the script in Windows-based POSIX environments.
- 237-242: Using
evalandxargsto parse environment variables and construct the Java command arguments list is a robust approach to handling complex argument structures. Ensure this method does not introduce security vulnerabilities.
| } | ||
|
|
||
| tasks.withType<KotlinCompile>().configureEach { | ||
| kotlinOptions { | ||
| jvmTarget = "11" | ||
| jvmTarget = JavaVersion.current().majorVersion |
There was a problem hiding this comment.
That binds the resulting binaries to the Java you use for launching Gradle.
It would be better to use a fixed target like target=11 or target=17 to avoid issues like "new testng is not compatible with Java 17 because Java 21 was used when building it"
There was a problem hiding this comment.
It was intentional: I supposed that the release pipeline will use the appropriate jvm version.
I suppose what you did on jmeter is a better approach but I need to understand how to manage a version everywhere it is needed.
There was a problem hiding this comment.
For JMeter, we manage it here: https://github.com/apache/jmeter/blob/master/build-logic/build-parameters/build.gradle.kts#L40-L49
In a nutshell, we ask Gradle to use specific JDK for building the code, and use a fixed "JVM target" release.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
gradle/wrapper/gradle-wrapper.jaris excluded by:!**/*.jar
Files selected for processing (2)
- gradle/wrapper/gradle-wrapper.properties (1 hunks)
- gradlew (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- gradle/wrapper/gradle-wrapper.properties
Additional comments: 8
gradlew (8)
- 25-38: Ensure POSIX compliance by verifying that all POSIX shell features used are correctly implemented and necessary for the script's functionality. This includes functions, expansions, compound commands, and built-in commands.
- 68-80: The symlink resolution logic appears correct but test extensively across different environments to ensure it handles all types of symlinks, especially daisy-chained ones, without issues.
- 87-87: Verify that
APP_HOMEis correctly determined in environments with complex directory structures or when the script is invoked from various locations.- 108-112: The OS-specific flags (
cygwin,msys,darwin,nonstop) are set based on the output ofuname. Confirm that these flags are used appropriately throughout the script to handle OS-specific behavior.- 130-249: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [121-140]
Ensure that the Java command determination logic correctly handles different
JAVA_HOMEconfigurations and provides clear error messages when Java is not found. This is crucial for user experience and script robustness.
- 144-159: The logic to increase the maximum file descriptors is important for performance. Verify that it works as expected across different Unix-like systems, especially given the POSIX shell's limitations.
- 171-197: The path conversion for Cygwin or MSYS environments is complex. Ensure that it correctly handles all argument types, including options and file paths, without introducing bugs or security issues.
- 202-247: The argument parsing and JVM option handling using
xargsandsedare intricate. Verify that this approach correctly handles all edge cases, especially concerning special characters and whitespace in JVM options.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (8)
- build-logic/basics/build.gradle.kts (1 hunks)
- build-logic/code-quality/build.gradle.kts (1 hunks)
- build-logic/jvm/build.gradle.kts (1 hunks)
- build-logic/jvm/src/main/kotlin/testng.java.gradle.kts (1 hunks)
- build-logic/jvm/src/main/kotlin/testng.kotlin-library.gradle.kts (1 hunks)
- build-logic/publishing/build.gradle.kts (1 hunks)
- gradle.properties (2 hunks)
- settings.gradle.kts (2 hunks)
Files skipped from review as they are similar to previous changes (7)
- build-logic/code-quality/build.gradle.kts
- build-logic/jvm/build.gradle.kts
- build-logic/jvm/src/main/kotlin/testng.java.gradle.kts
- build-logic/jvm/src/main/kotlin/testng.kotlin-library.gradle.kts
- build-logic/publishing/build.gradle.kts
- gradle.properties
- settings.gradle.kts
| tasks.withType<JavaCompile>().configureEach { | ||
| options.release.set((project.property("targetJavaVersion") as String).toInt()) | ||
| } |
There was a problem hiding this comment.
Ensure that the "targetJavaVersion" project property is always defined and valid. If it's missing or not a valid integer, this code will throw an exception at runtime.
Consider adding error handling or a default value for "targetJavaVersion" to prevent build failures.
|
@vlsi I tried to update according to your recommendations. Please, review. Now, I'm stuck with the issue: The property is declared in |
Fixes #3054.
Still some gradle warnings:
Summary by CodeRabbit