diff --git a/CHANGELOG.md b/CHANGELOG.md index ec142864f..feb1411e3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Enhancements ### Bug Fixes ### Infrastructure +* Upgrade gradle to 8.4 [1289](https://github.com/opensearch-project/k-NN/pull/1289) ### Documentation ### Maintenance * Update developer guide to include M1 Setup [#1222](https://github.com/opensearch-project/k-NN/pull/1222) diff --git a/build-tools/knnplugin-coverage.gradle b/build-tools/knnplugin-coverage.gradle index 386cfcfda..eb3582dab 100644 --- a/build-tools/knnplugin-coverage.gradle +++ b/build-tools/knnplugin-coverage.gradle @@ -3,12 +3,15 @@ * SPDX-License-Identifier: Apache-2.0 */ +apply plugin: 'jacoco' + +jacoco { + toolVersion = "0.8.10" +} + /** - * ES Plugin build tools don't work with the Gradle Jacoco Plugin to report coverage out of the box. - * https://github.com/elastic/elasticsearch/issues/28867. - * - * This code sets up coverage reporting manually for ES plugin tests. This is complicated because: - * 1. The ES integTest Task doesn't implement Gradle's JavaForkOptions so we have to manually start the jacoco agent with the test JVM + * This code sets up coverage reporting manually for the k-NN plugin tests. This is complicated because: + * 1. The OS integTest Task doesn't implement Gradle's JavaForkOptions so we have to manually start the jacoco agent with the test JVM * 2. The cluster nodes are stopped using 'kill -9' which means jacoco can't dump it's execution output to a file on VM shutdown * 3. The Java Security Manager prevents JMX from writing execution output to the file. * @@ -16,66 +19,31 @@ * cluster is stopped and dump it to a file. Luckily our current security policy seems to allow this. This will also probably * break if there are multiple nodes in the integTestCluster. But for now... it sorta works. */ - -import org.apache.tools.ant.taskdefs.condition.Os -apply plugin: 'jacoco' - -// Get gradle to generate the required jvm agent arg for us using a dummy tasks of type Test. Unfortunately Elastic's -// testing tasks don't derive from Test so the jacoco plugin can't do this automatically. -def jacocoDir = "${buildDir}/jacoco" -task dummyTest(type: Test) { - enabled = false - workingDir = file("/") // Force absolute path to jacoco agent jar - jacoco { - destinationFile = file("${jacocoDir}/test.exec") - destinationFile.parentFile.mkdirs() - jmx = true - } -} - -task dummyIntegTest(type: Test) { - enabled = false - workingDir = file("/") // Force absolute path to jacoco agent jar +integTest { jacoco { - destinationFile = file("${jacocoDir}/integTest.exec") - destinationFile.parentFile.mkdirs() jmx = true } -} -integTest { - systemProperty 'jacoco.dir', "${jacocoDir}" + systemProperty 'jacoco.dir', project.layout.buildDirectory.get().file("jacoco").asFile.absolutePath systemProperty 'jmx.serviceUrl', "service:jmx:rmi:///jndi/rmi://127.0.0.1:7777/jmxrmi" } jacocoTestReport { dependsOn integTest, test - executionData.from = [dummyTest.jacoco.destinationFile, dummyIntegTest.jacoco.destinationFile] - sourceDirectories.from = sourceSets.main.java.sourceDirectories - classDirectories.from = files(sourceSets.main.java.outputDir) - + executionData.from = [integTest.jacoco.destinationFile, test.jacoco.destinationFile] reports { - html.enabled = true // human readable - csv.enabled = true - xml.enabled = true // for coverlay + html.getRequired().set(true) // human readable + csv.getRequired().set(true) + xml.getRequired().set(true) // for coverlay } } -afterEvaluate { - jacocoTestReport.dependsOn integTest +testClusters.integTest { + jvmArgs " ${integTest.jacoco.getAsJvmArg()}" - testClusters.integTest { - if (Os.isFamily(Os.FAMILY_WINDOWS)) { - // Replacing build with absolute path to fix the error "error opening zip file or JAR manifest missing : /build/tmp/expandedArchives/..../jacocoagent.jar" - jvmArgs " ${dummyIntegTest.jacoco.getAsJvmArg()}".replace('build',"${buildDir}") - } else { - jvmArgs " ${dummyIntegTest.jacoco.getAsJvmArg()}".replace('javaagent:','javaagent:/') - } - - systemProperty 'com.sun.management.jmxremote', "true" - systemProperty 'com.sun.management.jmxremote.authenticate', "false" - systemProperty 'com.sun.management.jmxremote.port', "7777" - systemProperty 'com.sun.management.jmxremote.ssl', "false" - systemProperty 'java.rmi.server.hostname', "127.0.0.1" - } + systemProperty 'com.sun.management.jmxremote', "true" + systemProperty 'com.sun.management.jmxremote.authenticate', "false" + systemProperty 'com.sun.management.jmxremote.port', "7777" + systemProperty 'com.sun.management.jmxremote.ssl', "false" + systemProperty 'java.rmi.server.hostname', "127.0.0.1" } diff --git a/build.gradle b/build.gradle index bcd9b59bf..86df9eed5 100644 --- a/build.gradle +++ b/build.gradle @@ -36,9 +36,8 @@ plugins { id 'java-library' id 'java-test-fixtures' id 'idea' - id 'jacoco' id "com.diffplug.spotless" version "6.3.0" apply false - id 'io.freefair.lombok' version '6.4.3' + id 'io.freefair.lombok' version '8.4' } apply from: 'gradle/formatting.gradle' @@ -123,6 +122,9 @@ compileJava { compileTestJava { options.compilerArgs.addAll(["-processor", 'lombok.launch.AnnotationProcessorHider$AnnotationProcessor']) } +compileTestFixturesJava { + options.compilerArgs.addAll(["-processor", 'lombok.launch.AnnotationProcessorHider$AnnotationProcessor']) +} def usingRemoteCluster = System.properties.containsKey('tests.rest.cluster') || System.properties.containsKey('tests.cluster') def usingMultiNode = project.properties.containsKey('numNodes') @@ -135,10 +137,6 @@ if (!usingRemoteCluster) { } } -jacoco { - toolVersion = "0.8.7" -} - check.dependsOn spotlessCheck check.dependsOn jacocoTestReport diff --git a/gradle/wrapper/gradle-wrapper.jar b/gradle/wrapper/gradle-wrapper.jar index 943f0cbfa..7f93135c4 100644 Binary files a/gradle/wrapper/gradle-wrapper.jar and b/gradle/wrapper/gradle-wrapper.jar differ diff --git a/gradle/wrapper/gradle-wrapper.properties b/gradle/wrapper/gradle-wrapper.properties index fd0d6c243..b42703b94 100644 --- a/gradle/wrapper/gradle-wrapper.properties +++ b/gradle/wrapper/gradle-wrapper.properties @@ -5,8 +5,8 @@ distributionBase=GRADLE_USER_HOME distributionPath=wrapper/dists -distributionSha256Sum=518a863631feb7452b8f1b3dc2aaee5f388355cc3421bbd0275fbeadd77e84b2 -distributionUrl=https\://services.gradle.org/distributions/gradle-7.6.1-all.zip +distributionSha256Sum=f2b9ed0faf8472cbe469255ae6c86eddb77076c75191741b4a462f33128dd419 +distributionUrl=https\://services.gradle.org/distributions/gradle-8.4-all.zip networkTimeout=10000 zipStoreBase=GRADLE_USER_HOME zipStorePath=wrapper/dists diff --git a/gradlew b/gradlew index ab17f01df..a71dbba9b 100755 --- a/gradlew +++ b/gradlew @@ -87,10 +87,8 @@ done # This is normally unused # shellcheck disable=SC2034 APP_BASE_NAME=${0##*/} -APP_HOME=$( cd "${APP_HOME:-./}" && pwd -P ) || exit - -# Add default JVM options here. You can also use JAVA_OPTS and GRADLE_OPTS to pass JVM options to this script. -DEFAULT_JVM_OPTS='"-Xmx64m" "-Xms64m"' +# Discard cd standard output in case $CDPATH is set (https://github.com/gradle/gradle/issues/25036) +APP_HOME=$( cd "${APP_HOME:-./}" > /dev/null && pwd -P ) || exit # Use the maximum available, or set MAX_FD != -1 to use that value. MAX_FD=maximum @@ -137,10 +135,13 @@ location of your Java installation." fi else JAVACMD=java - which java >/dev/null 2>&1 || die "ERROR: JAVA_HOME is not set and no 'java' command could be found in your PATH. + if ! command -v java >/dev/null 2>&1 + then + die "ERROR: JAVA_HOME is not set and no 'java' command could be found in your PATH. Please set the JAVA_HOME variable in your environment to match the location of your Java installation." + fi fi # Increase the maximum file descriptors if we can. @@ -148,7 +149,7 @@ if ! "$cygwin" && ! "$darwin" && ! "$nonstop" ; then case $MAX_FD in #( max*) # In POSIX sh, ulimit -H is undefined. That's why the result is checked to see if it worked. - # shellcheck disable=SC3045 + # shellcheck disable=SC2039,SC3045 MAX_FD=$( ulimit -H -n ) || warn "Could not query maximum file descriptor limit" esac @@ -156,7 +157,7 @@ if ! "$cygwin" && ! "$darwin" && ! "$nonstop" ; then '' | soft) :;; #( *) # In POSIX sh, ulimit -n is undefined. That's why the result is checked to see if it worked. - # shellcheck disable=SC3045 + # shellcheck disable=SC2039,SC3045 ulimit -n "$MAX_FD" || warn "Could not set maximum file descriptor limit to $MAX_FD" esac @@ -201,11 +202,15 @@ if "$cygwin" || "$msys" ; then done fi -# Collect all arguments for the java command; -# * $DEFAULT_JVM_OPTS, $JAVA_OPTS, and $GRADLE_OPTS can contain fragments of -# shell script including quotes and variable substitutions, so put them in -# double quotes to make sure that they get re-expanded; and -# * put everything else in single quotes, so that it's not re-expanded. + +# Add default JVM options here. You can also use JAVA_OPTS and GRADLE_OPTS to pass JVM options to this script. +DEFAULT_JVM_OPTS='"-Xmx64m" "-Xms64m"' + +# Collect all arguments for the java command: +# * DEFAULT_JVM_OPTS, JAVA_OPTS, JAVA_OPTS, and optsEnvironmentVar are not allowed to contain shell fragments, +# and any embedded shellness will be escaped. +# * For example: A user cannot expect ${Hostname} to be expanded, as it is an environment variable and will be +# treated as '${Hostname}' itself on the command line. set -- \ "-Dorg.gradle.appname=$APP_BASE_NAME" \ diff --git a/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java b/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java index 011ec712c..642b8bfb9 100644 --- a/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java +++ b/src/testFixtures/java/org/opensearch/knn/KNNRestTestCase.java @@ -8,6 +8,7 @@ import com.google.common.base.Charsets; import com.google.common.io.Resources; import com.google.common.primitives.Floats; +import lombok.extern.log4j.Log4j2; import org.apache.commons.lang.StringUtils; import org.apache.hc.core5.http.io.entity.EntityUtils; import org.opensearch.core.common.bytes.BytesReference; @@ -52,7 +53,6 @@ import java.net.URL; import java.nio.file.Files; import java.nio.file.Path; -import java.nio.file.Paths; import java.util.ArrayList; import java.util.Arrays; import java.util.Base64; @@ -109,6 +109,7 @@ /** * Base class for integration tests for KNN plugin. Contains several methods for testing KNN ES functionality. */ +@Log4j2 public class KNNRestTestCase extends ODFERestTestCase { public static final String INDEX_NAME = "test_index"; public static final String FIELD_NAME = "test_field"; @@ -136,9 +137,10 @@ public static void dumpCoverage() throws IOException, MalformedObjectNameExcepti false ); - Path path = Paths.get(jacocoBuildPath + "/integTest.exec"); + Path path = Path.of(jacocoBuildPath, "integTest.exec"); Files.write(path, proxy.getExecutionData(false)); } catch (Exception ex) { + log.error("Failed to dump coverage: ", ex); throw new RuntimeException("Failed to dump coverage: " + ex); } }