Skip to content

Expose CMake target for use via FetchContent #340

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

Merged
merged 5 commits into from
Oct 3, 2021
Merged

Conversation

LTLA
Copy link
Contributor

@LTLA LTLA commented Sep 12, 2021

This allows other CMake projects to include HNSW via CMake's FetchContent approach, e.g.,

FetchContent_Declare(
  hnswlib
  GIT_REPOSITORY https://github.com/nmslib/hnswlib
  GIT_TAG master
)

FetchContent_MakeAvailable(hnswlib)

target_link_libraries(my_lib PUBLIC hnswlib)

A real-world example is available at https://github.com/LTLA/knncolle/blob/master/extern/CMakeLists.txt.

I moved the test building directives into a conditional that only triggers when actually building Hnswlib. This ensures that downstream projects are not saddled with the need to build the tests when all they want are the headers.

I also took the liberty of making getDataByLabel const, to indicate that it could be safely used in parallel frameworks that do not expect modifications to the index upon query; and I added a missing header for <deque>.

@yurymalkov
Copy link
Member

@LTLA thank you for the PR!
I wonder is minimal 3.14 version of cmake is a real necessity? E.g. I've check my mac and it is 3.10 and a google cloud instance has 3.13.4. Even colab has only 3.12.0.
I guess this can cause compatibility issues.

@LTLA
Copy link
Contributor Author

LTLA commented Sep 16, 2021

That's... a good question. Not sure why I bumped it up to 3.14, maybe just a force of habit. I lowered it back down to 2.6 with no issues for hnswlib itself. I don't think downstream projects will care because they'll set their own minimum requirements.

The only thing to note is that more recent versions of CMake will complain if the minimum version is too low. Probably it could be bumped to 3.* without many problems, but that's not entirely relevant to this PR, so I'll leave that to you.

@yurymalkov
Copy link
Member

@LTLA, thank you! I'll try the change shortly!

@LTLA
Copy link
Contributor Author

LTLA commented Sep 30, 2021

Gentle nudge.

@yurymalkov yurymalkov changed the base branch from master to develop October 1, 2021 05:24
@yurymalkov yurymalkov merged commit 7c63a15 into nmslib:develop Oct 3, 2021
@yurymalkov
Copy link
Member

@LTLA Thank you for the PR!

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.

2 participants