From 4a98708a2059f2f035264146db67d95a8128b182 Mon Sep 17 00:00:00 2001 From: Naveen Date: Wed, 26 Oct 2022 04:26:11 +0000 Subject: [PATCH] Remove Git Patch and add branching logic for CMakeLists.txt Signed-off-by: Naveen --- .github/workflows/CI.yml | 2 +- build-tools/knnplugin-coverage.gradle | 1 - build.gradle | 2 - jni/CMakeLists.txt | 128 ++++++++++++++++---------- patches/windows/CMakeLists.patch | 119 ------------------------ scripts/windowsScript.ps1 | 10 +- 6 files changed, 84 insertions(+), 178 deletions(-) delete mode 100644 patches/windows/CMakeLists.patch diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index e661c8359..c7d7e28e5 100644 --- a/.github/workflows/CI.yml +++ b/.github/workflows/CI.yml @@ -17,7 +17,7 @@ jobs: matrix: java: [11, 17] - name: Build and Test k-NN Plugin on Ubuntu + name: Build and Test k-NN Plugin runs-on: ubuntu-latest steps: diff --git a/build-tools/knnplugin-coverage.gradle b/build-tools/knnplugin-coverage.gradle index a304e5255..386cfcfda 100644 --- a/build-tools/knnplugin-coverage.gradle +++ b/build-tools/knnplugin-coverage.gradle @@ -66,7 +66,6 @@ afterEvaluate { 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 { diff --git a/build.gradle b/build.gradle index 00f6644bc..3def63c2d 100644 --- a/build.gradle +++ b/build.gradle @@ -197,7 +197,6 @@ test { systemProperty 'tests.security.manager', 'false' systemProperty "java.library.path", "$rootDir/jni/release" 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") } @@ -242,7 +241,6 @@ testClusters.integTest { testDistribution = "ARCHIVE" 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") } diff --git a/jni/CMakeLists.txt b/jni/CMakeLists.txt index c6b2f5f22..75d4ba81b 100644 --- a/jni/CMakeLists.txt +++ b/jni/CMakeLists.txt @@ -35,6 +35,14 @@ if (${CMAKE_SYSTEM_NAME} STREQUAL Darwin) elseif(${CMAKE_SYSTEM_NAME} STREQUAL Linux) set(JVM_OS_TYPE linux) set(LIB_EXT .so) +elseif(${CMAKE_SYSTEM_NAME} STREQUAL Windows) +# Set the CXX_COMPILER_VERSION, CMAKE_CXX_FLAGS, JVM_OS_TYPE, prefix and extension for the target libraries that are built. + set(CXX_COMPILER_VERSION ${CMAKE_CXX_COMPILER_VERSION}) + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fpermissive") + set(JVM_OS_TYPE win32) + set(LIB_EXT .dll) + set(CMAKE_SHARED_LIBRARY_PREFIX "") + set(CMAKE_STATIC_LIBRARY_PREFIX "") else() message(FATAL_ERROR "Unable to run on system: ${CMAKE_SYSTEM_NAME}") endif() @@ -57,7 +65,14 @@ add_library(${TARGET_LIB_COMMON} SHARED ${CMAKE_CURRENT_SOURCE_DIR}/src/jni_util target_include_directories(${TARGET_LIB_COMMON} PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include $ENV{JAVA_HOME}/include $ENV{JAVA_HOME}/include/${JVM_OS_TYPE}) set_target_properties(${TARGET_LIB_COMMON} PROPERTIES SUFFIX ${LIB_EXT}) set_target_properties(${TARGET_LIB_COMMON} PROPERTIES POSITION_INDEPENDENT_CODE ON) -set_target_properties(${TARGET_LIB_COMMON} PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/release) + +if (WIN32) +# Use RUNTIME_OUTPUT_DIRECTORY, to build the target library (opensearchknn_common) in the specified directory at runtime. + set_target_properties(${TARGET_LIB_COMMON} PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/release) +else() + set_target_properties(${TARGET_LIB_COMMON} PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/release) +endif() + list(APPEND TARGET_LIBS ${TARGET_LIB_COMMON}) # ---------------------------------------------------------------------------- @@ -79,7 +94,13 @@ if (${CONFIG_NMSLIB} STREQUAL ON OR ${CONFIG_ALL} STREQUAL ON OR ${CONFIG_TEST} target_include_directories(${TARGET_LIB_NMSLIB} PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include $ENV{JAVA_HOME}/include $ENV{JAVA_HOME}/include/${JVM_OS_TYPE} ${CMAKE_CURRENT_SOURCE_DIR}/external/nmslib/similarity_search/include) set_target_properties(${TARGET_LIB_NMSLIB} PROPERTIES SUFFIX ${LIB_EXT}) set_target_properties(${TARGET_LIB_NMSLIB} PROPERTIES POSITION_INDEPENDENT_CODE ON) - set_target_properties(${TARGET_LIB_NMSLIB} PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/release) + + if (WIN32) + # Use RUNTIME_OUTPUT_DIRECTORY, to build the target library (opensearchknn_nmslib) in the specified directory at runtime. + set_target_properties(${TARGET_LIB_NMSLIB} PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/release) + else() + set_target_properties(${TARGET_LIB_NMSLIB} PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/release) + endif() list(APPEND TARGET_LIBS ${TARGET_LIB_NMSLIB}) endif () @@ -130,7 +151,13 @@ if (${CONFIG_FAISS} STREQUAL ON OR ${CONFIG_ALL} STREQUAL ON OR ${CONFIG_TEST} S target_include_directories(${TARGET_LIB_FAISS} PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include $ENV{JAVA_HOME}/include $ENV{JAVA_HOME}/include/${JVM_OS_TYPE} ${CMAKE_CURRENT_SOURCE_DIR}/external/faiss) set_target_properties(${TARGET_LIB_FAISS} PROPERTIES SUFFIX ${LIB_EXT}) set_target_properties(${TARGET_LIB_FAISS} PROPERTIES POSITION_INDEPENDENT_CODE ON) - set_target_properties(${TARGET_LIB_FAISS} PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/release) + + if (WIN32) + # Use RUNTIME_OUTPUT_DIRECTORY, to build the target library (opensearchknn_faiss) in the specified directory at runtime. + set_target_properties(${TARGET_LIB_FAISS} PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/release) + else() + set_target_properties(${TARGET_LIB_FAISS} PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/release) + endif() list(APPEND TARGET_LIBS ${TARGET_LIB_FAISS}) endif () @@ -138,51 +165,56 @@ endif () # --------------------------------------------------------------------------- # --------------------------------- TESTS ----------------------------------- -if (${CONFIG_ALL} STREQUAL ON OR ${CONFIG_TEST} STREQUAL ON) - # Reference - https://crascit.com/2015/07/25/cmake-gtest/ - configure_file(CMakeLists.txt.in googletest-download/CMakeLists.txt) - execute_process(COMMAND "${CMAKE_COMMAND}" -G "${CMAKE_GENERATOR}" . - WORKING_DIRECTORY "${CMAKE_BINARY_DIR}/googletest-download" - ) - execute_process(COMMAND "${CMAKE_COMMAND}" --build . - WORKING_DIRECTORY "${CMAKE_BINARY_DIR}/googletest-download" - ) - set(gtest_force_shared_crt ON CACHE BOOL "" FORCE) - - add_subdirectory("${CMAKE_BINARY_DIR}/googletest-src" - "${CMAKE_BINARY_DIR}/googletest-build" EXCLUDE_FROM_ALL - ) - add_executable( - jni_test - tests/faiss_wrapper_test.cpp - tests/nmslib_wrapper_test.cpp - tests/test_util.cpp) - - target_link_libraries( - jni_test - gtest_main - gmock_main - faiss - NonMetricSpaceLib - OpenMP::OpenMP_CXX - ${TARGET_LIB_FAISS} - ${TARGET_LIB_NMSLIB} - ${TARGET_LIB_COMMON} - ) - - target_include_directories(jni_test PRIVATE - ${CMAKE_CURRENT_SOURCE_DIR}/tests - ${CMAKE_CURRENT_SOURCE_DIR}/include - $ENV{JAVA_HOME}/include - $ENV{JAVA_HOME}/include/${JVM_OS_TYPE} - ${CMAKE_CURRENT_SOURCE_DIR}/external/faiss - ${CMAKE_CURRENT_SOURCE_DIR}/external/nmslib/similarity_search/include - ${gtest_SOURCE_DIR}/include - ${gmock_SOURCE_DIR}/include) - - - set_target_properties(jni_test PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/bin) -endif () +# Windows : Comment the TESTS for now because the tests are failing(failing to build jni_tests.exe) if we are building our target libraries as SHARED libraries. +# TODO: Fix the failing JNI TESTS on Windows +if (!WIN32) + if (${CONFIG_ALL} STREQUAL ON OR ${CONFIG_TEST} STREQUAL ON) + # Reference - https://crascit.com/2015/07/25/cmake-gtest/ + configure_file(CMakeLists.txt.in googletest-download/CMakeLists.txt) + execute_process(COMMAND "${CMAKE_COMMAND}" -G "${CMAKE_GENERATOR}" . + WORKING_DIRECTORY "${CMAKE_BINARY_DIR}/googletest-download" + ) + execute_process(COMMAND "${CMAKE_COMMAND}" --build . + WORKING_DIRECTORY "${CMAKE_BINARY_DIR}/googletest-download" + ) + set(gtest_force_shared_crt ON CACHE BOOL "" FORCE) + + add_subdirectory("${CMAKE_BINARY_DIR}/googletest-src" + "${CMAKE_BINARY_DIR}/googletest-build" EXCLUDE_FROM_ALL + ) + add_executable( + jni_test + tests/faiss_wrapper_test.cpp + tests/nmslib_wrapper_test.cpp + tests/test_util.cpp) + + target_link_libraries( + jni_test + gtest_main + gmock_main + faiss + NonMetricSpaceLib + OpenMP::OpenMP_CXX + ${TARGET_LIB_FAISS} + ${TARGET_LIB_NMSLIB} + ${TARGET_LIB_COMMON} + ) + + target_include_directories(jni_test PRIVATE + ${CMAKE_CURRENT_SOURCE_DIR}/tests + ${CMAKE_CURRENT_SOURCE_DIR}/include + $ENV{JAVA_HOME}/include + $ENV{JAVA_HOME}/include/${JVM_OS_TYPE} + ${CMAKE_CURRENT_SOURCE_DIR}/external/faiss + ${CMAKE_CURRENT_SOURCE_DIR}/external/nmslib/similarity_search/include + ${gtest_SOURCE_DIR}/include + ${gmock_SOURCE_DIR}/include) + + + set_target_properties(jni_test PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/bin) + endif () +endif() + # --------------------------------------------------------------------------- # -------------------------------- INSTALL ---------------------------------- diff --git a/patches/windows/CMakeLists.patch b/patches/windows/CMakeLists.patch deleted file mode 100644 index 7c8390fae..000000000 --- a/patches/windows/CMakeLists.patch +++ /dev/null @@ -1,119 +0,0 @@ -Index: jni/CMakeLists.txt -<+>UTF-8 -=================================================================== -Description: -We are making the following changes to jni/CMakeLists.txt file to build it on Windows OS -1. Set CXX_COMPILER_VERSION and CMAKE_CXX_FLAGS. -2. Add a new if else case when the CMAKE_SYSTEM_NAME matches Windows and set flags JVM_OS_TYPE, prefix and extension for the target libraries that are built. -3. Replace LIBRARY_OUTPUT_DIRECTORY with RUNTIME_OUTPUT_DIRECTORY, to build the target libraries(opensearchknn_common, opensearchknn_nmslib and opensearchknn_faiss) in the -specified directory at runtime. -4. Comment the TESTS for now because the tests are failing(failing to build jni_tests.exe) if we are building our target libraries as SHARED libraries. - -TODO: Fix the failing JNI TESTS - -=================================================================== -diff --git a/jni/CMakeLists.txt b/jni/CMakeLists.txt ---- a/jni/CMakeLists.txt (revision 78a2b5b2a83db52aded56ab74940a62c94a88b50) -+++ b/jni/CMakeLists.txt (date 1666128659873) -@@ -14,8 +14,10 @@ - set(TARGET_LIB_FAISS opensearchknn_faiss) # faiss JNI - set(TARGET_LIBS "") # Libs to be installed - -+set(CXX_COMPILER_VERSION ${CMAKE_CXX_COMPILER_VERSION}) - set(CMAKE_CXX_STANDARD 11) - set(CMAKE_CXX_STANDARD_REQUIRED True) -+set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fpermissive") - - option(CONFIG_FAISS "Configure faiss library build when this is on") - option(CONFIG_NMSLIB "Configure nmslib library build when this is on") -@@ -35,6 +37,11 @@ - elseif(${CMAKE_SYSTEM_NAME} STREQUAL Linux) - set(JVM_OS_TYPE linux) - set(LIB_EXT .so) -+elseif(${CMAKE_SYSTEM_NAME} STREQUAL Windows) -+ set(JVM_OS_TYPE win32) -+ set(LIB_EXT .dll) -+ set(CMAKE_SHARED_LIBRARY_PREFIX "") -+ set(CMAKE_STATIC_LIBRARY_PREFIX "") - else() - message(FATAL_ERROR "Unable to run on system: ${CMAKE_SYSTEM_NAME}") - endif() -@@ -57,7 +64,7 @@ - target_include_directories(${TARGET_LIB_COMMON} PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include $ENV{JAVA_HOME}/include $ENV{JAVA_HOME}/include/${JVM_OS_TYPE}) - set_target_properties(${TARGET_LIB_COMMON} PROPERTIES SUFFIX ${LIB_EXT}) - set_target_properties(${TARGET_LIB_COMMON} PROPERTIES POSITION_INDEPENDENT_CODE ON) --set_target_properties(${TARGET_LIB_COMMON} PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/release) -+set_target_properties(${TARGET_LIB_COMMON} PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/release) - list(APPEND TARGET_LIBS ${TARGET_LIB_COMMON}) - # ---------------------------------------------------------------------------- - -@@ -79,7 +86,7 @@ - target_include_directories(${TARGET_LIB_NMSLIB} PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include $ENV{JAVA_HOME}/include $ENV{JAVA_HOME}/include/${JVM_OS_TYPE} ${CMAKE_CURRENT_SOURCE_DIR}/external/nmslib/similarity_search/include) - set_target_properties(${TARGET_LIB_NMSLIB} PROPERTIES SUFFIX ${LIB_EXT}) - set_target_properties(${TARGET_LIB_NMSLIB} PROPERTIES POSITION_INDEPENDENT_CODE ON) -- set_target_properties(${TARGET_LIB_NMSLIB} PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/release) -+ set_target_properties(${TARGET_LIB_NMSLIB} PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/release) - - list(APPEND TARGET_LIBS ${TARGET_LIB_NMSLIB}) - endif () -@@ -130,7 +137,7 @@ - target_include_directories(${TARGET_LIB_FAISS} PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include $ENV{JAVA_HOME}/include $ENV{JAVA_HOME}/include/${JVM_OS_TYPE} ${CMAKE_CURRENT_SOURCE_DIR}/external/faiss) - set_target_properties(${TARGET_LIB_FAISS} PROPERTIES SUFFIX ${LIB_EXT}) - set_target_properties(${TARGET_LIB_FAISS} PROPERTIES POSITION_INDEPENDENT_CODE ON) -- set_target_properties(${TARGET_LIB_FAISS} PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/release) -+ set_target_properties(${TARGET_LIB_FAISS} PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/release) - - list(APPEND TARGET_LIBS ${TARGET_LIB_FAISS}) - endif () -@@ -138,51 +145,7 @@ - # --------------------------------------------------------------------------- - - # --------------------------------- TESTS ----------------------------------- --if (${CONFIG_ALL} STREQUAL ON OR ${CONFIG_TEST} STREQUAL ON) -- # Reference - https://crascit.com/2015/07/25/cmake-gtest/ -- configure_file(CMakeLists.txt.in googletest-download/CMakeLists.txt) -- execute_process(COMMAND "${CMAKE_COMMAND}" -G "${CMAKE_GENERATOR}" . -- WORKING_DIRECTORY "${CMAKE_BINARY_DIR}/googletest-download" -- ) -- execute_process(COMMAND "${CMAKE_COMMAND}" --build . -- WORKING_DIRECTORY "${CMAKE_BINARY_DIR}/googletest-download" -- ) -- set(gtest_force_shared_crt ON CACHE BOOL "" FORCE) -- -- add_subdirectory("${CMAKE_BINARY_DIR}/googletest-src" -- "${CMAKE_BINARY_DIR}/googletest-build" EXCLUDE_FROM_ALL -- ) -- add_executable( -- jni_test -- tests/faiss_wrapper_test.cpp -- tests/nmslib_wrapper_test.cpp -- tests/test_util.cpp) -- -- target_link_libraries( -- jni_test -- gtest_main -- gmock_main -- faiss -- NonMetricSpaceLib -- OpenMP::OpenMP_CXX -- ${TARGET_LIB_FAISS} -- ${TARGET_LIB_NMSLIB} -- ${TARGET_LIB_COMMON} -- ) -- -- target_include_directories(jni_test PRIVATE -- ${CMAKE_CURRENT_SOURCE_DIR}/tests -- ${CMAKE_CURRENT_SOURCE_DIR}/include -- $ENV{JAVA_HOME}/include -- $ENV{JAVA_HOME}/include/${JVM_OS_TYPE} -- ${CMAKE_CURRENT_SOURCE_DIR}/external/faiss -- ${CMAKE_CURRENT_SOURCE_DIR}/external/nmslib/similarity_search/include -- ${gtest_SOURCE_DIR}/include -- ${gmock_SOURCE_DIR}/include) - -- -- set_target_properties(jni_test PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/bin) --endif () - # --------------------------------------------------------------------------- - - # -------------------------------- INSTALL ---------------------------------- diff --git a/scripts/windowsScript.ps1 b/scripts/windowsScript.ps1 index c271c5219..eee64046a 100644 --- a/scripts/windowsScript.ps1 +++ b/scripts/windowsScript.ps1 @@ -6,16 +6,12 @@ git submodule update --init -- jni/external/nmslib git submodule update --init -- jni/external/faiss -git apply patches/windows/CMakeLists.patch --verbose - -# Validating if the CMakeLists patch is applied -echo "Validating if the CMakeLists patch is applied" -type jni/CMakeLists.txt | Select-String "Windows" - -# Replace '_MSC_VER' with '__MINGW32__' +# _MSC_VER is a predefined macro which defines the version of Visual Studio Compiler +# As we are using x86_64-w64-mingw32-gcc compiler we need to replace this macro with __MINGW32__ (Get-Content jni/external/faiss/faiss/impl/index_read.cpp).replace('_MSC_VER', '__MINGW32__') | Set-Content jni/external/faiss/faiss/impl/index_read.cpp (Get-Content jni/external/faiss/faiss/impl/index_write.cpp).replace('_MSC_VER', '__MINGW32__') | Set-Content jni/external/faiss/faiss/impl/index_write.cpp +# is a Unix header and is not available on Windows. So, adding condition to include it if not running on Windows # Replace '#include ' with # #ifndef __MINGW32__ # #include