-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Run build-tools test with Gradle jdk #49459
Run build-tools test with Gradle jdk #49459
Conversation
The test task is configured to use the runtime java version, but there are issues with the version of groovy used by gradle pre 6.0. In order to workaround this, we use the compiler java to execute the build-tools tests. Closes elastic#49404
Pinging @elastic/es-core-infra (:Core/Infra/Build) |
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.
This brings up a good question, which is that we don't well define what versions of Java our build runtime is compatible with. This is mostly minimumCompilerVersion
but there are exceptions here. Essentially, because we are still on Gradle 5.x, you cannot run the build with Java 13, thus it makes no sense to test build-tools
against that version.
With that in mind, I think the better thing to do is simply to disable our build-tools tests entirely on those Java versions. Essentially, those matrix jobs are not applicable to build-tools
, so there's no point in reconfiguring the build to test with another java version that already overlaps with another matrix build.
Let's do something like this:
tasks.withType(Test) {
onlyIf {
org.elasticsearch.gradle.info.BuildParams.runtimeJavaVersion <= JavaVersion.VERSION_12
}
}
I made the proposed change but I have a concern. In the default developer setup, tests will not be run for build-tools at all. Unless a runtime java home is specified, we default to using JDK 13 as the runtime java home (the provisioned jdk) so these tests will only run in matrix jobs if I understand correctly. |
Good point. So I'd do two things:
|
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.
LGTM 👍
buildSrc/build.gradle
Outdated
@@ -223,8 +224,13 @@ if (project != rootProject) { | |||
} | |||
check.dependsOn(integTest) | |||
|
|||
// for now we hardcode the tests for our build to use the gradle jvm. | |||
tasks.withType(Test).configureEach { | |||
it.executable = Jvm.current().getJavaHome().toPath().resolve("bin").resolve("java").toAbsolutePath() |
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.
This can be simplified to Jvm.current().getJavaExecutable()
The test task is configured to use the runtime java version, but there are issues with the version of groovy used by gradle pre 6.0. In order to workaround this, we use the Gradle JDK to execute the build-tools tests. Closes elastic#49404 Closes elastic#49253
The test task is configured to use the runtime java version, but there
are issues with the version of groovy used by gradle pre 6.0. In order
to workaround this, we use the Gradle JDK to execute the build-tools
tests.
Closes #49404
Closes #49253