From 89ea7298d9de8320fb651a6893d17850997dd716 Mon Sep 17 00:00:00 2001 From: Samuel Herman Date: Mon, 10 Feb 2025 15:38:39 -0800 Subject: [PATCH 1/5] clean JNI artifacts with ./gradlew clean Signed-off-by: Samuel Herman --- build.gradle | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/build.gradle b/build.gradle index 0a48aa6f3..374142434 100644 --- a/build.gradle +++ b/build.gradle @@ -405,6 +405,18 @@ tasks.register('cmakeJniLib', Exec) { standardOutput = outputStream } +// Makes sure that `./gradlew clean` removes all JNI build artifacts +tasks.clean.doFirst { + // Delete JNI build directory + delete "${projectDir}/jni/build" + // Delete JNI release directory + delete "${projectDir}/jni/release" + // Delete any other JNI-related directories that might contain build artifacts + delete "${projectDir}/jni/packages" + delete "${projectDir}/jni/bin" + delete "${projectDir}/jni/lib" +} + task buildNmslib(type:Exec) { dependsOn cmakeJniLib def makePath = findExecutable("make") From 45d344806ca37a9e1dee10d23d34146e26d63e84 Mon Sep 17 00:00:00 2001 From: Samuel Herman Date: Mon, 10 Feb 2025 16:33:07 -0800 Subject: [PATCH 2/5] nest release under build directory Signed-off-by: Samuel Herman --- CHANGELOG.md | 1 + build.gradle | 44 +++++++++++++++++++++--------------------- jni/cmake/macros.cmake | 4 ++-- 3 files changed, 25 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a290784bd..ccb5fe2fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,4 +58,5 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), * Upgrade jsonpath from 2.8.0 to 2.9.0[2325](https://github.com/opensearch-project/k-NN/pull/2325) * Bump Faiss commit from 1f42e81 to 0cbc2a8 to accelerate hamming distance calculation using _mm512_popcnt_epi64 intrinsic and also add avx512-fp16 instructions to boost performance [#2381](https://github.com/opensearch-project/k-NN/pull/2381) * Enabled indices.breaker.total.use_real_memory setting via build.gradle for integTest Cluster to catch heap CB in local ITs and github CI actions [#2395](https://github.com/opensearch-project/k-NN/pull/2395/) +* Enabled idempotency of local builds when using `./gradlew clean` and nest `jni/release` directory under `jni/build` for easier cleanup [#2516](https://github.com/opensearch-project/k-NN/pull/2516) ### Refactoring diff --git a/build.gradle b/build.gradle index 374142434..90f333d1c 100644 --- a/build.gradle +++ b/build.gradle @@ -373,8 +373,8 @@ tasks.register('cmakeJniLib', Exec) { def args = [] args.add(cmakePath) - args.add("-S jni") - args.add("-B jni/build") + args.add("-S jni") // CMakelists.txt directory + args.add("-B jni/build") // Build directory args.add("-DKNN_PLUGIN_VERSION=${opensearch_version}") args.add("-DAVX2_ENABLED=${avx2_enabled}") args.add("-DAVX512_ENABLED=${avx512_enabled}") @@ -409,39 +409,39 @@ tasks.register('cmakeJniLib', Exec) { tasks.clean.doFirst { // Delete JNI build directory delete "${projectDir}/jni/build" - // Delete JNI release directory - delete "${projectDir}/jni/release" - // Delete any other JNI-related directories that might contain build artifacts - delete "${projectDir}/jni/packages" - delete "${projectDir}/jni/bin" - delete "${projectDir}/jni/lib" } task buildNmslib(type:Exec) { dependsOn cmakeJniLib - def makePath = findExecutable("make") - logger.lifecycle("Using make at: ${makePath}") - // Ensure makePath is treated as a String. If parsing to an int is required, - // handle it explicitly, though a path typically should not be parsed as an int. - if (makePath.isEmpty()) { - throw new GradleException("Make not found in PATH. Please install Make.") + def cmakePath = findExecutable("cmake") + logger.lifecycle("Using cmake at: ${cmakePath}") + + if (cmakePath.isEmpty()) { + throw new GradleException("CMake not found in PATH. Please install CMake.") } + def outputStream = new ByteArrayOutputStream() - commandLine makePath, '-Cjni/build', 'opensearchknn_nmslib', '-j', "${nproc_count}" + commandLine cmakePath, + '--build', 'jni/build', + '--target', 'opensearchknn_nmslib', + '--parallel', "${nproc_count}" standardOutput = outputStream } task buildJniLib(type:Exec) { dependsOn cmakeJniLib - def makePath = findExecutable("make") - logger.lifecycle("Using make at: ${makePath}") - // Ensure makePath is treated as a String. If parsing to an int is required, - // handle it explicitly, though a path typically should not be parsed as an int. - if (makePath.isEmpty()) { - throw new GradleException("Make not found in PATH. Please install Make.") + def cmakePath = findExecutable("cmake") + logger.lifecycle("Using cmake at: ${cmakePath}") + + if (cmakePath.isEmpty()) { + throw new GradleException("CMake not found in PATH. Please install CMake.") } + def outputStream = new ByteArrayOutputStream() - commandLine makePath, '-Cjni/build', 'opensearchknn_faiss', 'opensearchknn_common', '-j', "${nproc_count}" + commandLine cmakePath, + '--build', 'jni/build', + '--target', 'opensearchknn_faiss', 'opensearchknn_common', + '--parallel', "${nproc_count}" standardOutput = outputStream } diff --git a/jni/cmake/macros.cmake b/jni/cmake/macros.cmake index 773033b7e..801b35c56 100644 --- a/jni/cmake/macros.cmake +++ b/jni/cmake/macros.cmake @@ -9,8 +9,8 @@ macro(opensearch_set_common_properties TARGET) if (NOT "${WIN32}" STREQUAL "") # Use RUNTIME_OUTPUT_DIRECTORY, to build the target library in the specified directory at runtime. - set_target_properties(${TARGET} PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/release) + set_target_properties(${TARGET} PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/release) else() - set_target_properties(${TARGET} PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/release) + set_target_properties(${TARGET} PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/release) endif() endmacro() From 867a276d90f7f91368a80a19ed16a12a1cba65d7 Mon Sep 17 00:00:00 2001 From: Samuel Herman Date: Mon, 10 Feb 2025 16:40:06 -0800 Subject: [PATCH 3/5] adjust all references to the old release path Signed-off-by: Samuel Herman --- .gitignore | 4 ---- build.gradle | 10 +++++----- qa/restart-upgrade/build.gradle | 6 +++--- qa/rolling-upgrade/build.gradle | 14 +++++++------- 4 files changed, 15 insertions(+), 19 deletions(-) diff --git a/.gitignore b/.gitignore index 34f7ff154..8dc6c2889 100644 --- a/.gitignore +++ b/.gitignore @@ -18,14 +18,10 @@ jni/CMakeCache.txt jni/CMakeFiles jni/Makefile jni/cmake_install.cmake -jni/release -jni/packages jni/CTestTestfile.cmake jni/KNNPlugin_JNI.cbp jni/Testing/ -jni/_deps/ jni/bin/ -jni/lib/ jni/jni_test* jni/googletest* jni/cmake/*.cmake-e diff --git a/build.gradle b/build.gradle index 90f333d1c..51da2c54b 100644 --- a/build.gradle +++ b/build.gradle @@ -449,12 +449,12 @@ test { dependsOn buildNmslib dependsOn buildJniLib systemProperty 'tests.security.manager', 'false' - systemProperty "java.library.path", "$rootDir/jni/release" + systemProperty "java.library.path", "$rootDir/jni/build/release" //this change enables mockito-inline that supports mocking of static classes/calls systemProperty "jdk.attach.allowAttachSelf", true if (Os.isFamily(Os.FAMILY_WINDOWS)) { // Add the paths of built JNI libraries and its dependent libraries to PATH variable in System variables - environment('PATH', System.getenv('PATH') + ";$rootDir/jni/release" + ";$rootDir/src/main/resources/windowsDependencies") + environment('PATH', System.getenv('PATH') + ";$rootDir/jni/build/release" + ";$rootDir/src/main/resources/windowsDependencies") } } @@ -466,7 +466,7 @@ integTest { } systemProperty 'tests.security.manager', 'false' systemProperty 'java.io.tmpdir', opensearch_tmp_dir.absolutePath - systemProperty "java.library.path", "$rootDir/jni/release" + systemProperty "java.library.path", "$rootDir/jni/build/release" // allows integration test classes to access test resource from project root path systemProperty('project.root', project.rootDir.absolutePath) @@ -518,7 +518,7 @@ testClusters.integTest { plugin(project.tasks.bundlePlugin.archiveFile) if (Os.isFamily(Os.FAMILY_WINDOWS)) { // Add the paths of built JNI libraries and its dependent libraries to PATH variable in System variables - environment('PATH', System.getenv('PATH') + ";$rootDir/jni/release" + ";$rootDir/src/main/resources/windowsDependencies") + environment('PATH', System.getenv('PATH') + ";$rootDir/jni/build/release" + ";$rootDir/src/main/resources/windowsDependencies") } // Cluster shrink exception thrown if we try to set numberOfNodes to 1, so only apply if > 1 @@ -533,7 +533,7 @@ testClusters.integTest { debugPort += 1 } } - systemProperty("java.library.path", "$rootDir/jni/release") + systemProperty("java.library.path", "$rootDir/jni/build/release") systemProperty propertyKeys.breaker.useRealMemory, getBreakerSetting() } diff --git a/qa/restart-upgrade/build.gradle b/qa/restart-upgrade/build.gradle index b444f6af7..64f4dd4f5 100644 --- a/qa/restart-upgrade/build.gradle +++ b/qa/restart-upgrade/build.gradle @@ -26,7 +26,7 @@ testClusters { environment "LD_LIBRARY_PATH", "${buildDir}/testclusters/${baseName}-0/distro/${knn_bwc_version_no_qualifier}-ARCHIVE/plugins/opensearch-knn/knnlib;${buildDir}/testclusters/${baseName}-0/distro/${knn_bwc_version_no_qualifier}-ARCHIVE/plugins/opensearch-knn/lib" if (Os.isFamily(Os.FAMILY_WINDOWS)) { // While running on Windows OS, setting the PATH environment variable to include the paths to dlls of JNI libraries and windows dependencies - environment('PATH', System.getenv('PATH') + ";$rootDir/jni/release" + ";$rootDir/src/main/resources/windowsDependencies") + environment('PATH', System.getenv('PATH') + ";$rootDir/jni/build/release" + ";$rootDir/src/main/resources/windowsDependencies") systemProperty "java.library.path", "${buildDir}/testclusters/${baseName}-0/distro/${knn_bwc_version_no_qualifier}-ARCHIVE/plugins/opensearch-knn/knnlib;${buildDir}/testclusters/${baseName}-0/distro/${knn_bwc_version_no_qualifier}-ARCHIVE/plugins/opensearch-knn/lib" } else { systemProperty "java.library.path", "${buildDir}/testclusters/${baseName}-0/distro/${knn_bwc_version_no_qualifier}-ARCHIVE/plugins/opensearch-knn/knnlib:${buildDir}/testclusters/${baseName}-0/distro/${knn_bwc_version_no_qualifier}-ARCHIVE/plugins/opensearch-knn/lib" @@ -140,8 +140,8 @@ testClusters { dependsOn rootProject.tasks.assemble useCluster testClusters."${baseName}" doFirst { - testClusters."${baseName}".environment("LD_LIBRARY_PATH", "$rootDir/jni/release") - testClusters."${baseName}".systemProperty("java.library.path", "$rootDir/jni/release") + testClusters."${baseName}".environment("LD_LIBRARY_PATH", "$rootDir/jni/build/release") + testClusters."${baseName}".systemProperty("java.library.path", "$rootDir/jni/build/release") testClusters."${baseName}".upgradeAllNodesAndPluginsToNextVersion([rootProject.tasks.bundlePlugin.archiveFile]) } systemProperty 'tests.rest.bwcsuite_cluster', 'upgraded_cluster' diff --git a/qa/rolling-upgrade/build.gradle b/qa/rolling-upgrade/build.gradle index 13145a5f3..33413d086 100644 --- a/qa/rolling-upgrade/build.gradle +++ b/qa/rolling-upgrade/build.gradle @@ -26,7 +26,7 @@ testClusters { environment "LD_LIBRARY_PATH", "${buildDir}/testclusters/${baseName}-0/distro/${knn_bwc_version_no_qualifier}-ARCHIVE/plugins/opensearch-knn/knnlib;${buildDir}/testclusters/${baseName}-0/distro/${knn_bwc_version_no_qualifier}-ARCHIVE/plugins/opensearch-knn/lib" if (Os.isFamily(Os.FAMILY_WINDOWS)) { // While running on Windows OS, setting the PATH environment variable to include the paths to dlls of JNI libraries and windows dependencies - environment('PATH', System.getenv('PATH') + ";$rootDir/jni/release" + ";$rootDir/src/main/resources/windowsDependencies") + environment('PATH', System.getenv('PATH') + ";$rootDir/jni/build/release" + ";$rootDir/src/main/resources/windowsDependencies") systemProperty "java.library.path", "${buildDir}/testclusters/${baseName}-0/distro/${knn_bwc_version_no_qualifier}-ARCHIVE/plugins/opensearch-knn/knnlib;${buildDir}/testclusters/${baseName}-0/distro/${knn_bwc_version_no_qualifier}-ARCHIVE/plugins/opensearch-knn/lib" } else { systemProperty "java.library.path", "${buildDir}/testclusters/${baseName}-0/distro/${knn_bwc_version_no_qualifier}-ARCHIVE/plugins/opensearch-knn/knnlib:${buildDir}/testclusters/${baseName}-0/distro/${knn_bwc_version_no_qualifier}-ARCHIVE/plugins/opensearch-knn/lib" @@ -57,8 +57,8 @@ task testAgainstOneThirdUpgradedCluster(type: StandaloneRestIntegTestTask) { dependsOn rootProject.tasks.assemble dependsOn "testAgainstOldCluster" doFirst { - testClusters."${baseName}".getNodes().getAt("${baseName}" + "-0").environment("LD_LIBRARY_PATH", "$rootDir/jni/release") - testClusters."${baseName}".getNodes().getAt("${baseName}" + "-0").systemProperty("java.library.path", "$rootDir/jni/release") + testClusters."${baseName}".getNodes().getAt("${baseName}" + "-0").environment("LD_LIBRARY_PATH", "$rootDir/jni/build/release") + testClusters."${baseName}".getNodes().getAt("${baseName}" + "-0").systemProperty("java.library.path", "$rootDir/jni/build/release") testClusters."${baseName}".upgradeNodeAndPluginToNextVersion([rootProject.tasks.bundlePlugin.archiveFile]) } systemProperty 'tests.rest.bwcsuite_cluster', 'mixed_cluster' @@ -76,8 +76,8 @@ task testAgainstTwoThirdsUpgradedCluster(type: StandaloneRestIntegTestTask) { dependsOn "testAgainstOneThirdUpgradedCluster" useCluster testClusters."${baseName}" doFirst { - testClusters."${baseName}".getNodes().getAt("${baseName}" + "-1").environment("LD_LIBRARY_PATH", "$rootDir/jni/release") - testClusters."${baseName}".getNodes().getAt("${baseName}" + "-1").systemProperty("java.library.path", "$rootDir/jni/release") + testClusters."${baseName}".getNodes().getAt("${baseName}" + "-1").environment("LD_LIBRARY_PATH", "$rootDir/jni/build/release") + testClusters."${baseName}".getNodes().getAt("${baseName}" + "-1").systemProperty("java.library.path", "$rootDir/jni/build/release") testClusters."${baseName}".upgradeNodeAndPluginToNextVersion([rootProject.tasks.bundlePlugin.archiveFile]) } systemProperty 'tests.rest.bwcsuite_cluster', 'mixed_cluster' @@ -95,8 +95,8 @@ task testRollingUpgrade(type: StandaloneRestIntegTestTask) { dependsOn "testAgainstTwoThirdsUpgradedCluster" useCluster testClusters."${baseName}" doFirst { - testClusters."${baseName}".getNodes().getAt("${baseName}" + "-2").environment("LD_LIBRARY_PATH", "$rootDir/jni/release") - testClusters."${baseName}".getNodes().getAt("${baseName}" + "-2").systemProperty("java.library.path", "$rootDir/jni/release") + testClusters."${baseName}".getNodes().getAt("${baseName}" + "-2").environment("LD_LIBRARY_PATH", "$rootDir/jni/build/release") + testClusters."${baseName}".getNodes().getAt("${baseName}" + "-2").systemProperty("java.library.path", "$rootDir/jni/build/release") testClusters."${baseName}".upgradeNodeAndPluginToNextVersion([rootProject.tasks.bundlePlugin.archiveFile]) } mustRunAfter "testAgainstOneThirdUpgradedCluster" From 25ae31bb418d4192313bdf9715a8057707b4e160 Mon Sep 17 00:00:00 2001 From: Samuel Herman Date: Mon, 10 Feb 2025 19:55:14 -0800 Subject: [PATCH 4/5] remove irrelevant paths from gitignore, add jni/build Signed-off-by: Samuel Herman --- .gitignore | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/.gitignore b/.gitignore index 8dc6c2889..016891ded 100644 --- a/.gitignore +++ b/.gitignore @@ -14,22 +14,11 @@ out/ oss/* *.iml -jni/CMakeCache.txt -jni/CMakeFiles -jni/Makefile -jni/cmake_install.cmake -jni/CTestTestfile.cmake -jni/KNNPlugin_JNI.cbp jni/Testing/ jni/bin/ -jni/jni_test* -jni/googletest* +jni/build/ jni/cmake/*.cmake-e -jni/.cmake jni/.idea -jni/build.ninja -jni/.ninja_deps -jni/.ninja_log benchmarks/perf-tool/okpt/output benchmarks/perf-tool/okpt/dev From faab07315831795139084f607839824a22bdbfd4 Mon Sep 17 00:00:00 2001 From: Samuel Herman Date: Mon, 10 Feb 2025 20:48:46 -0800 Subject: [PATCH 5/5] fix logging on linux Signed-off-by: Samuel Herman --- build.gradle | 4 ---- 1 file changed, 4 deletions(-) diff --git a/build.gradle b/build.gradle index 51da2c54b..b5f715847 100644 --- a/build.gradle +++ b/build.gradle @@ -420,12 +420,10 @@ task buildNmslib(type:Exec) { throw new GradleException("CMake not found in PATH. Please install CMake.") } - def outputStream = new ByteArrayOutputStream() commandLine cmakePath, '--build', 'jni/build', '--target', 'opensearchknn_nmslib', '--parallel', "${nproc_count}" - standardOutput = outputStream } task buildJniLib(type:Exec) { @@ -437,12 +435,10 @@ task buildJniLib(type:Exec) { throw new GradleException("CMake not found in PATH. Please install CMake.") } - def outputStream = new ByteArrayOutputStream() commandLine cmakePath, '--build', 'jni/build', '--target', 'opensearchknn_faiss', 'opensearchknn_common', '--parallel', "${nproc_count}" - standardOutput = outputStream } test {