Clean JNI artifacts with ./gradlew clean (opensearch-project#2516)

* clean JNI artifacts with ./gradlew clean Signed-off-by: Samuel Herman <[email protected]> * nest release under build directory Signed-off-by: Samuel Herman <[email protected]> * adjust all references to the old release path Signed-off-by: Samuel Herman <[email protected]> * remove irrelevant paths from gitignore, add jni/build Signed-off-by: Samuel Herman <[email protected]> * fix logging on linux Signed-off-by: Samuel Herman <[email protected]> --------- Signed-off-by: Samuel Herman <[email protected]>
Vikasht34 · Feb 11, 2025 · 359a37b · 359a37b
1 parent 82e314e
commit 359a37b
Show file tree

Hide file tree

Showing 6 changed files with 47 additions and 53 deletions.
diff --git a/.gitignore b/.gitignore
@@ -14,26 +14,11 @@ out/
 oss/*
 *.iml
 
-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/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

diff --git 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
@@ -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}")
@@ -405,44 +405,52 @@ 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"
+}
+
 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}"
-    standardOutput = outputStream
+
+    commandLine cmakePath,
+            '--build', 'jni/build',
+            '--target', 'opensearchknn_nmslib',
+            '--parallel', "${nproc_count}"
 }
 
 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}"
-    standardOutput = outputStream
+
+    commandLine cmakePath,
+            '--build', 'jni/build',
+            '--target', 'opensearchknn_faiss', 'opensearchknn_common',
+            '--parallel', "${nproc_count}"
 }
 
 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")
     }
 }
 
@@ -454,7 +462,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)
 
@@ -506,7 +514,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
@@ -521,7 +529,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/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()
diff --git 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
@@ -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"