Upgrade gradle to v8.14.3#8950
Conversation
sarahchen6
added
type: enhancement
|
I believe the failure in if (currentJavaHomePath != testJvmHomePath) {
- def jvmSpec = new SpecificInstallationToolchainSpec(project.getObjects(), file(testJvmHomePath))
+ def jvmSpec = new SpecificInstallationToolchainSpec(project.services.get(org.gradle.api.internal.provider.PropertyFactory), file(testJvmHomePath))
// The provider always says that a value is present so we need to wrap it for proper error messages
Provider<JavaLauncher> launcher = providers.provider {This object is internal and has to use this internal API, there's no other options at this time I think. |
There was a problem hiding this comment.
When upgrading from Gradle <= 8.13, Groovy needs to be upgraded to 3.0.24: https://docs.gradle.org/current/userguide/upgrading_version_8.html#upgrade_to_groovy_3_0_24
There was a problem hiding this comment.
Changes in this file address aesthetic comments in the previous Gradle upgrade #8886
There was a problem hiding this comment.
Received the error The type Metric is not a valid substitute for the bounded parameter <M extends datadog.trace.api.telemetry.MetricCollector$Metric>. To resolve this, we needed strict type resolution to distinguish MetricCollector.Metric and the imported datadog.telemetry.api.Metric class. This type strictness follows patterns to explicitly use MetricCollector.Metric throughout the file.
There was a problem hiding this comment.
Much better without ambiguity ! TY
There was a problem hiding this comment.
Project dependencies was separated out due to the error The value for property 'dependencies.implementation' property 'dependencies' is final and cannot be changed any further. that prevented Gradle compilation. I think this is due to the deprecation of mutating configurations after observation that was introduced after 8.7: https://docs.gradle.org/current/userguide/upgrading_version_8.html#mutate_configuration_after_locking
There was a problem hiding this comment.
Indeed this looks like it : gradle/gradle#28867
Maybe drop a comment there, to explain why the documented method did not work. I wasn't able to reliably reproduce this issue. So there might be something odd happending.
There was a problem hiding this comment.
This change is needed due to the introduced error groovy.lang.GroovyRuntimeException: Could not find matching constructor for: org.gradle.jvm.toolchain.internal.SpecificInstallationToolchainSpec(org.gradle.api.internal.model.DefaultObjectFactory, File). I'm not sure exactly where this comes from, but since this is part of Gradle's internal implementation, breaking changes can be made unannounced.
There was a problem hiding this comment.
This internal Gradle type has changed twice its constructor signature since 8.5
- In 8.8 : Introduce daemon JVM criteria gradle/gradle#28731 (
ObjectsFactoryarg was removed) - In 8.13 : Daemon auto-provisioning downloads toolchain from provided url gradle/gradle#29166 (
PropertyFactoryarg was added)
There was a problem hiding this comment.
GenericProgressiveFutureListener argument extends ProgressiveFuture (ref), and with the Gradle upgrade, I get the error: The type Future is not a valid substitute for the bounded parameter <F extends io.netty.util.concurrent.ProgressiveFuture<?>>. Explicitly setting the type to ProgressiveFuture instead of Future resolved this.
There was a problem hiding this comment.
For reference, there is a warning on master on this, suggesting to use ProgressiveFuture instead:
There was a problem hiding this comment.
When trying to ./gradlew clean assemble, I ran across the error incompatible types: org.gradle.internal.service.scopes.BuildScopeServices cannot be converted to org.gradle.internal.service.DefaultServiceRegistry. Explicitly setting the buildScopeServices type to DefaultServiceRegistry (which is what injectCiVisibilityGradleListener takes) resolves this.
I'm thinking that the Gradle upgrade has led to stricter type resolution as seen from other changes needed in this PR 🤔
There was a problem hiding this comment.
The updates to the latest version of JUnit Jupiter and Platform address the error:
org.junit.platform.commons.JUnitException: TestEngine with ID 'junit-jupiter' failed to discover tests...
Caused by: org.junit.platform.commons.JUnitException: OutputDirectoryProvider not available; probably due to unaligned versions of the junit-platform-engine and junit-platform-launcher jars on the classpath/module path.
Not quite sure why the gradle upgrade initiated this, but others have faced the same problem (unplanned junit issue) and resolved it by updating versions.
There was a problem hiding this comment.
I'd like to use the oldest possible version here, as we want to ensure JUnit 5 instrumentation is working for older releases. Could you please double-check which artifacts' version changes when Gradle version is bumped?
You can use commands like the one below:
./gradlew :dd-java-agent:instrumentation:junit-5.3:dependencyInsight --configuration testRuntimeClasspath --dependency org.junit
There was a problem hiding this comment.
@nikita-tkachenko-datadog Maybe something you can do is to declare them as constraints in this case ?
https://docs.gradle.org/current/userguide/dependency_constraints.html
There was a problem hiding this comment.
Good to know. From the command you shared, it looks like 1.12.0 and 5.12.0 are the oldest versions we can pull in (e.g. org.junit.platform:junit-platform-suite-api:1.12.0 and org.junit.jupiter:junit-jupiter-api:5.12.0 get pulled in). I can change that now!
|
You may want to merge latest master, there were fixes on the gradle daemon smoke tests, see #8955 Unsure about the muzzle ones. |
sarahchen6
force-pushed
the
sarahchen6/upgrade-to-gradle-8.14
branch
from
d8450d8 to
0bb8eab
Compare
| if (gradleDaemonCmdLineParams) { | ||
| command += "-Dorg.gradle.jvmargs=$gradleDaemonCmdLineParams" | ||
| command += "-Dorg.gradle.jvmargs=$gradleDaemonCmdLineParams".toString() | ||
| } |
There was a problem hiding this comment.
This resolves errors I was getting with assigning GStringImpl to String[] (others' experiences: apache issue, stackoverflow issue). Seems like this should be resolved with groovy 3.0.7+, but I guess not…
sarahchen6
force-pushed
the
sarahchen6/upgrade-to-gradle-8.14
branch
from
581e03a to
fa6c44c
Compare
There was a problem hiding this comment.
The muzzle check for this instrumentation fails on a classloader mismatch when running on Gradle 8.14.3, but since this instrumentation targets Gradle 8.3, skip muzzle like what is done for 8_10_Instrumentation.
|
FYI 8.14.3 was just released |
sarahchen6
requested review from
AlexeyKuznetsov-DD,
bric3,
nikita-tkachenko-datadog,
pawelchcki and
ygree
and removed request for
a team
There was a problem hiding this comment.
Shall we also update grdale.lock for this and similar places?
There was a problem hiding this comment.
Today I learned where it came from
sarahchen6
force-pushed
the
sarahchen6/upgrade-to-gradle-8.14
branch
from
dc12875 to
2f16060
Compare
sarahchen6
force-pushed
the
sarahchen6/upgrade-to-gradle-8.14
branch
from
2f16060 to
7e140f4
Compare
PerfectSlayer
left a comment
PerfectSlayer
left a comment
There was a problem hiding this comment.
Not much feedback to give on this part 😓 I feel @bric3 to be way more knowledgeable on that part than me.
But mainly, I would make sure to get approval from CI Visibility before merging as changing their instrumentations.
bric3
left a comment
bric3
left a comment
There was a problem hiding this comment.
Overall it looks good ! Thank you for the upgrade !
There was a problem hiding this comment.
Today I learned where it came from
There was a problem hiding this comment.
@nikita-tkachenko-datadog Maybe something you can do is to declare them as constraints in this case ?
https://docs.gradle.org/current/userguide/dependency_constraints.html
There was a problem hiding this comment.
For reference, there is a warning on master on this, suggesting to use ProgressiveFuture instead:
| // 3.0.24 added support for JDK 24 via ASM 9.7.1 | ||
| // https://groovy-lang.org/changelogs/changelog-3.0.24.html | ||
| // https://asm.ow2.io/versions.html#9.7.1 | ||
| configurations.all { | ||
| resolutionStrategy { | ||
| force 'org.codehaus.groovy:groovy-all:3.0.24' | ||
| } | ||
| } |
There was a problem hiding this comment.
note: We may have to resurrect this code for JDK 25.
There was a problem hiding this comment.
This internal Gradle type has changed twice its constructor signature since 8.5
- In 8.8 : Introduce daemon JVM criteria gradle/gradle#28731 (
ObjectsFactoryarg was removed) - In 8.13 : Daemon auto-provisioning downloads toolchain from provided url gradle/gradle#29166 (
PropertyFactoryarg was added)
There was a problem hiding this comment.
Much better without ambiguity ! TY
There was a problem hiding this comment.
Indeed this looks like it : gradle/gradle#28867
Maybe drop a comment there, to explain why the documented method did not work. I wasn't able to reliably reproduce this issue. So there might be something odd happending.
Co-authored-by: Brice Dutheil <brice.dutheil@gmail.com>
6 participants
What Does This Do
Upgrade gradle to v8.14.3
Motivation
Upgrade to the latest version of gradle
Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any usefull labelsclose,fixor any linking keywords when referencing an issue.Use
solvesinstead, and assign the PR milestone to the issueJira ticket: https://datadoghq.atlassian.net/browse/LANGPLAT-660