Update build scripts to Gradle 9.4.1 from 8.14.4#11272
Update build scripts to Gradle 9.4.1 from 8.14.4#11272AlexeyKuznetsov-DD wants to merge 3 commits intomasterfrom
Conversation
AlexeyKuznetsov-DD
added
tag: do not merge
There was a problem hiding this comment.
I only reviewed commit 1274a6f, there's some nice fixes, and ideas that align with previous work in #10402, there's also fixes that I don't think are useful for Gradle 9 migration like:
- def javaDir = buildDir.toPath().resolve("generated/sources/tries/java/${sourceSetName}")
+ def javaDir = project.layout.buildDirectory.dir("generated/sources/tries/java/${sourceSetName}")This commit alone touches a lot of files. Some changes can be split to smaller PRs.
I'm weighing on toggling off failOnNoDiscoveredTests, maybe there's some "file" detection to add in the addTestSuite methods.
| // Only change test JVM if it's not the one we are running the gradle build with | ||
| if ((jvmSpec as? SpecificInstallationToolchainSpec)?.javaHome == currentJavaHomePath.get()) { | ||
| project.providers.provider<JavaLauncher?> { null } | ||
| project.objects.property(JavaLauncher::class.java) |
There was a problem hiding this comment.
note: One of the changes in Gradle 9 is that properties do no allow null anymore. Maybe something better can be used there.
| test { | ||
| java { | ||
| srcDirs += ["$buildDir/generated/source/proto/test/java"] | ||
| srcDir tasks.named("generateTestProto").map { "${it.outputBaseDir}/java" } |
There was a problem hiding this comment.
issue: This might be eager. So we might need to use a different approach.
| description = 'Copy metrics.yaml files from integrations-core into resources' | ||
| inputDirectory = file("$projectDir/integrations-core") | ||
| outputDirectory = file("$buildDir/integrations-core-resources") | ||
| outputDirectory = layout.buildDirectory.dir("integrations-core-resources").get().asFile |
There was a problem hiding this comment.
suggestion: I see .get() here, I believe this could be improvement to rewrite the CopyMetricConfigsTask to use providers instead.
| testJvmConstraints { | ||
| minJavaVersion = JavaVersion.VERSION_21 | ||
| } | ||
| failOnNoDiscoveredTests = false |
There was a problem hiding this comment.
suggestion: I've seen this as well albeit not when trying Gradle 9.3 on dd-trace-java, IIRC... I think a better solution is to add that flag within the logic of addTestSuiteExtendingForDir, addTestSuiteForDir, this could prevent adding this every time these test suite methods are used.
| // dependency 'log4j-over-slf4j' brought in by :dd-java-agent:testing which would shadow | ||
| // the log4j module under test using a proxy to slf4j instead. | ||
| testCompile.exclude group: 'org.slf4j', module: 'log4j-over-slf4j' | ||
| testImplementation.exclude group: 'org.slf4j', module: 'log4j-over-slf4j' |
There was a problem hiding this comment.
question: What was the problem with testConmpile ?
| // Gradle 9 daemon JVM is JDK 17, but this WAR is deployed to a WildFly server | ||
| // that may run on Java 8 (testJvmConstraints maxJavaVersion=11). Pin a Java 8 | ||
| // toolchain so the produced bytecode loads on the lowest supported test JVM. | ||
| java { | ||
| toolchain { | ||
| languageVersion = JavaLanguageVersion.of(8) | ||
| } | ||
| } |
| // define the task that builds the EAR via a sub-Gradle invocation. | ||
| // | ||
| // The sub-build inherits this project's wrapper (Gradle 9.4.1), whose daemon | ||
| // requires JDK 17 — that's why JAVA_HOME is 17, not the test runtime version. | ||
| // The deployed WAR's compile target is pinned separately in | ||
| // spring-ear/war/build.gradle via a Java toolchain. | ||
| tasks.register('earBuild', Exec) { | ||
| workingDir "$appDir" | ||
| environment += [ | ||
| "GRADLE_OPTS": "-Dorg.gradle.jvmargs='-Xmx512M'", | ||
| "JAVA_HOME": getLazyJavaHomeFor(8) | ||
| "JAVA_HOME": getLazyJavaHomeFor(17) | ||
| ] | ||
| commandLine "$rootDir/${gradlewCommand}", "assemble", "--no-daemon", "--max-workers=4", "-PappBuildDir=$appBuildDir", "-PapiJar=${project(':dd-trace-api').tasks.jar.archiveFile.get()}" | ||
| commandLine "$rootDir/${gradlewCommand}", "assemble", "--no-daemon", "--max-workers=4", | ||
| "-PappBuildDir=${appBuildDir.get().asFile.absolutePath}", | ||
| "-PapiJar=${project(':dd-trace-api').tasks.jar.archiveFile.get().asFile.absolutePath}" |
There was a problem hiding this comment.
suggestion: We may need a more general way to invoke Gradle for sub projects. Actually, even relying on an older Gradle distribution is on the table.
|
|
||
| project.configurations.api.allDependencies.each { | ||
| if ((it instanceof ProjectDependency) || !(it instanceof SelfResolvingDependency)) { | ||
| if (it instanceof ModuleDependency) { |
There was a problem hiding this comment.
suggestion:
| if (it instanceof ModuleDependency) { | |
| if (it instanceof ProjectDependency || it instanceof ExternalModuleDependency) { |
There was a problem hiding this comment.
suggestion: Let's avoid again another script plugin, this could be a correct convention plugin dedicated for smoke tests.
Also, spring-boot-plugin.gradle and this can be "merged" as a single plugin.
| def stderr = new ByteArrayOutputStream() | ||
| def process = new ProcessBuilder('git', 'rev-parse', '--short', 'HEAD').start() | ||
| process.waitForProcessOutput(stdout, stderr) | ||
| if (process.exitValue() != 0) { | ||
| throw new GradleException("Unable to determine git hash: ${stderr.toString().trim()}") |
|
Also, did bumping to Groovy 4 was needed to make everything work ? |
@bric3 If I recall correctly I have to bump codenark and also smoke tests for gradle are using |
2 participants
DO NOT MERGE, experimenting
What Does This Do
Motivation
Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]
Note: Once your PR is ready to merge, add it to the merge queue by commenting
/merge./merge -ccancels the queue request./merge -f --reason "reason"skips all merge queue checks; please use this judiciously, as some checks do not run at the PR-level. For more information, see this doc.