Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CMake install targets #446

Merged
merged 2 commits into from
May 7, 2023
Merged

Add CMake install targets #446

merged 2 commits into from
May 7, 2023

Conversation

moritz-h
Copy link
Contributor

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

Other changes:

  • Raise the minimum CMake version to 3.0. Testing CMake 2.8.12.2 on CentOS 7 generated errors already before doing these changes.
  • Make the CMake project() name consistent with the library name (change hnsw_lib to hnswlib).
  • Add option HNSWLIB_EXAMPLES to allow enabling/disabling tests and examples explicitly. Option defaults to previous behavior checking CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME

@moritz-h
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

@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:

  • The new option HNSWLIB_EXAMPLES defaults to previous behaviour, therefore users could fully ignore it without anything changed.
  • All install rules do not change anything on the existing build. It is now possible to run make install or cmake --build . --target install to install the project. This was not possible before, but nothing is changed if users do not explicitly use the install target.
  • The target hnswlib has now a namespaced alias hnswlib::hnswlib, but that does not change the original target and users can choose which to use.

@moritz-h
Copy link
Contributor Author

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
Copy link
Member

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

@dyashuni
Copy link
Contributor

dyashuni commented May 7, 2023

@moritz-h Thank you, looks good

@dyashuni dyashuni merged commit dccd4f9 into nmslib:develop May 7, 2023
@moritz-h moritz-h deleted the cmake branch May 7, 2023 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants