Add an integration test that runs on JDK-8 #795
Conversation
reta
force-pushed
the
issue-778
branch
2 times, most recently
from
6cbd85f to
e785ec3
Compare
reta
requested review from
Bukhtawar,
dblock,
szczepanczykd,
madhusudhankonda,
saratvemulapalli and
VachaShah
as code owners
Signed-off-by: Andriy Redko <[email protected]>
java-client/build.gradle.kts
Outdated
| @@ -369,5 +372,17 @@ if (JavaVersion.current() >= JavaVersion.VERSION_11) { | |||
| testClassesDirs += java11.output.classesDirs | |||
| classpath = sourceSets["java11"].runtimeClasspath | |||
| } | |||
|
|
|||
| } else if (jdk8compatibility == true) { | |||
| java { | |||
There was a problem hiding this comment.
@dblock @VachaShah curious what you think about this approach: we cannot run Gradle build using JDK-8 but we could use toolchains to run tests under JDK-8.
java-client/build.gradle.kts
Outdated
| if (JavaVersion.current() >= JavaVersion.VERSION_11) { | ||
| // Use `-Pcheck-jdk8-compatibility=true` to | ||
| val jdk8compatibility = (project.findProperty("check-jdk8-compatibility") as String?).toBoolean() | ||
| if (JavaVersion.current() >= JavaVersion.VERSION_11 && jdk8compatibility == false) { |
There was a problem hiding this comment.
I dislike the whole check-jdk8-compatibility=true business. Why do we need this at all and not just this?
if (JavaVersion.current() >= JavaVersion.VERSION_11) {
...
} else if (JavaVersion.current() >= JavaVersion.VERSION_8) {
...
}There was a problem hiding this comment.
That the thing, the JavaVersion.current() is the version Gradle run with, it is always 11 or higher. I will try to experiment with toolchains here (alternatively, we could make all tests JDK-8 compatible and that would eliminate the conditional part)
There was a problem hiding this comment.
Ok, I see.
Maybe we could improve on this by introducing optional JAVA_TOOLCHAIN_VERSION and default it to 11. Basically avoiding this whole if logic?
Stepping back, this does seem to work, so we should merge it and improve it later if you can't come up with something a lot nicer. Leave some TODOs with an explanation at the least?
There was a problem hiding this comment.
This is a good idea, thanks @dblock , I refactored it to use similar to OpenSearch core approach with runtime.java version.
reta
force-pushed
the
issue-778
branch
2 times, most recently
from
0c157de to
40ee090
Compare
reta
force-pushed
the
issue-778
branch
3 times, most recently
from
9e7fbc1 to
74282dd
Compare
As always, Windows have troubles with env variables that have dots |
Signed-off-by: Andriy Redko <[email protected]>
* Add an integration test that runs on JDK-8 Signed-off-by: Andriy Redko <[email protected]> * Refactor the runtime Java selection Signed-off-by: Andriy Redko <[email protected]> --------- Signed-off-by: Andriy Redko <[email protected]> (cherry picked from commit f8ec75e) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* Add an integration test that runs on JDK-8 Signed-off-by: Andriy Redko <[email protected]> * Refactor the runtime Java selection Signed-off-by: Andriy Redko <[email protected]> --------- Signed-off-by: Andriy Redko <[email protected]> (cherry picked from commit f8ec75e) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Signed-off-by: Andriy Redko <[email protected]>
Description
Add an integration test that runs on JDK-8
Issues Resolved
Closes #778
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.