search

cdalitz / hclust-cpp

C++ fast hierarchical clustering algorithms
Other
79 stars 16 forks source link

Added cmake. #6

Closed keksklauer4 closed 1 year ago

cdalitz commented 1 year ago

As you already closed teh pull request, I wonder whether you still suggest inclusion of the cmake support. Although hclust-cpp is primarily meant as a library, a cmake-file might be useful to make clear what c++ standard is used (setting it to C++20 would be prohibitive for many projects).

Would you still like it to be include and changed according to the actual requirements?

keksklauer4 commented 1 year ago

Hey @cdalitz , to be honest, I didn't actually want to create this PR just yet (which is why I closed it).

Although it is very likely not CMake best practice, I typically include external libraries directly using CMake, i. e. by including the project as a git submodule and then adding the CMake as a subdirectory in my project's CMake.

This works pretty well most of the time and in this you just have to then link against the library and add the include directory.

The advantages are that you don't need to install a library on every machine you build on, it should work on both Linux and Windows and it needs close to zero work to include the library.

Regarding the C++20 standard: I didn't have the time to check the required version which is why I set it to C++20. Which version would suffice?

If you'd like, I could adjust the CMake a bit, test it further (it worked both on Linux and on Windows but I would test the inclusion as a subproject on Windows as well), add a short paragraph in the readme and then reopen the pr.

cdalitz commented 1 year ago

The standard is C++ 98. Moreover, your CMakefile works with cmake 3.5, so the minimum cmake version should be reduced to 3.5.

When calling your CMakeLists.txt file, I obtain the following warning, but I guess that this is on purpose?

You have called ADD_LIBRARY for library hclust without any source files. This typically indicates a problem with your CMakeLists.txt file

If this ADD_LIBRARY warning is ok, I can upload the CMakeLists.txt to giithub. But this should also be documented in README.md, especially how this file can be utilized to include hclust as a library. Any suggestions?