nmslib / hnswlib

Header-only C++/python library for fast approximate nearest neighbors
https://github.com/nmslib/hnswlib
Apache License 2.0
4.12k stars 609 forks source link

Add CMake install targets #446

Closed moritz-h closed 1 year ago

moritz-h commented 1 year ago

Add CMake install targets to allow installing the library using CMake and allow usage with find_package().

Other changes:

moritz-h commented 1 year ago

May I ask is there a tendency if these changes will be ok or not? Installing CMake targets for hnswlib would make packaging of downstream C++ libraries with a transitive dependency on hnswlib a lot easier. But if you don't want to change CMake here, I would at least know that downstream workarounds are the way to go in the long run.

yurymalkov commented 1 year ago

Hi @moritz-h, Thank you for the PR and the reminder! I'm open to merging it, but as I'm not very familiar with CMake, I'm a bit concerned about potential negative consequences for users relying on the previous structure. What do you think are the chances of a broken build in such cases?

moritz-h commented 1 year ago

@yurymalkov The only major breaking change is the updated minimum required CMake version. There should be no changes to users with CMake >= 3.0. But the current CMake in hnswlib master seems to be broken anyway for CMake 2.x, so I assume there are no CMake 2.x users, and this change is not critical. Also CMake 3.0 release was 9 years ago, so should be reasonable to drop 2.x support.

Running CMake 2.8.12.2 in a CentOS 7 Docker container with hnswlib master: ``` CMake Error: Error required internal CMake variable not set, cmake may be not be built correctly. Missing variable is: CMAKE_LANGUAGES_COMPILER_ENV_VAR CMake Error: Error required internal CMake variable not set, cmake may be not be built correctly. Missing variable is: CMAKE_LANGUAGES_COMPILER CMake Error: Could not find cmake module file: /root/hnswlib/build/CMakeFiles/2.8.12.2/CMakeLANGUAGESCompiler.cmake -- The CXX compiler identification is GNU 4.8.5 CMake Error: Could not find cmake module file: CMakeLANGUAGESInformation.cmake -- Check for working CXX compiler: /usr/bin/c++ -- Check for working CXX compiler: /usr/bin/c++ -- works -- Detecting CXX compiler ABI info -- Detecting CXX compiler ABI info - done CMake Error: CMAKE_LANGUAGES_COMPILER not set, after EnableLanguage -- Configuring incomplete, errors occurred! See also "/root/hnswlib/build/CMakeFiles/CMakeOutput.log". ```

Renaming project(hnsw_lib) to project(hnswlib) should be only a cosmetic change. The only edge case I can think of is someone using the variable CMAKE_PROJECT_<PROJECT-NAME>_INCLUDE_BEFORE to inject CMake code. But this is most likely a constructed theoretical argument, I would not assume anyone is using this here, with this very minimal CMake project. Even then, the worst case would be users must update the variable name. My thought was to clean this up to match the name of the library, but could revert it to hnsw_lib if you want to avoid any minimal possible change, even in rare edge cases.

Everything else is just adding features, no change for existing users:

moritz-h commented 1 year ago

I just discovered that the next CMake release 3.27 will print a deprecation warning for projects with compatibility to CMake older than 3.5, see https://cmake.org/cmake/help/git-master/release/dev/deprecate-policy-old.html This is fixed by setting a version range to cmake_minimum_required to explicitly set the project compatible to newer CMake versions.

yurymalkov commented 1 year ago

Cool, thank you! We will merge the PR after @dyashuni will do a quick look at it.

dyashuni commented 1 year ago

@moritz-h Thank you, looks good