From 359a37b7812d478a93441b819a31fe5580abee32 Mon Sep 17 00:00:00 2001 From: sam-herman <97131656+sam-herman@users.noreply.github.com> Date: Mon, 10 Feb 2025 22:21:00 -0800 Subject: [PATCH] Clean JNI artifacts with ./gradlew clean (#2516) * clean JNI artifacts with ./gradlew clean Signed-off-by: Samuel Herman * nest release under build directory Signed-off-by: Samuel Herman * adjust all references to the old release path Signed-off-by: Samuel Herman * remove irrelevant paths from gitignore, add jni/build Signed-off-by: Samuel Herman * fix logging on linux Signed-off-by: Samuel Herman --------- Signed-off-by: Samuel Herman --- .gitignore | 17 +--------- CHANGELOG.md | 1 + build.gradle | 58 +++++++++++++++++++-------------- jni/cmake/macros.cmake | 4 +-- qa/restart-upgrade/build.gradle | 6 ++-- qa/rolling-upgrade/build.gradle | 14 ++++---- 6 files changed, 47 insertions(+), 53 deletions(-) diff --git a/.gitignore b/.gitignore index 34f7ff154..016891ded 100644 --- 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 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 0a48aa6f3..b5f715847 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}") @@ -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 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() 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"