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

Support CMake system #11

Merged
merged 3 commits into from Apr 30, 2019
Merged

Support CMake system #11

merged 3 commits into from Apr 30, 2019

Conversation

heavywatal
Copy link
Contributor

This patch enables users to install clipp with CMake:

cd clipp/
mkdir build
cd build/
cmake -DCMAKE_INSTALL_PREFIX=${HOME}/local ..
make install

and use it from other CMake projects:

# CMakeLists.txt in awesome_project
project(awesome_project CXX)
# ...
find_package(clipp)
# ...
target_link_libraries(${PROJECT_NAME} PRIVATE clipp)

CMake is also useful for building and executing the tests:

cmake -DBUILD_TESTING=1 -DCMAKE_CXX_COMPILER=g++-8 ..
cmake --build . -- -j4
ctest -V

(I have not checked if it works equivalently as run_tests.py, though)

I am concerned that this PR might conflict with your policy. Please feel free to reject this, and I would be fine to keep it in my branch.

@muellan
Copy link
Owner

muellan commented Aug 1, 2018

I can see that CMake would be useful for building the tests. But what is the purpose of the other CMakeLists.txt file? I mean clipp is a header only library. Don't you just include it in your project and only add the translation units and binary libs to your CMakeLists.txt and CMake will be able to figure out the dependencies on headers itself?
Anyway, for now I would like to keep anything that isn't neccessary for the library to work out of the repo. It's such a small project and I don't want to clutter it with files for all the different build systems. But I'm of course willing to change my mind if more users really want it.

@heavywatal
Copy link
Contributor Author

just include it in your project

Yes, that's the quickest way to add single header libraries to a project. But in the long run, it makes it difficult to manage dependencies and follow their updates. Therefore, developers often want to keep external libraries out of their project repositories. For example, I like nlohmann/json, an awesome single-header library, and install it in /usr/local/, ~/local/, or some other locations to use it from several projects. Of course it can be achieved by just git + some scripts, but CMake provides more portable and configurable methods.

Anyway, for now I would like to keep anything that isn't neccessary for the library to work out of the repo. It's such a small project and I don't want to clutter it with files for all the different build systems. But I'm of course willing to change my mind if more users really want it.

I respect your opinion. Let's wait and see coming +1s.

@muellan
Copy link
Owner

muellan commented Aug 1, 2018

Yeah Niels Lohmann's JSON lib is awesome; I've also used it a couple of times.
I agree that if you want to keep (even single-header) libs external the approach you proposed is better.
Personally I like to bundle projects with all their dependent libs so they don't interfere with other projects that use other versions of those libs (it also makes deployment easier). I just had too many bad experiences of projects not compiling/working anymore because some library was updated and the project needed the older version for various reasons. Of course this approach has limits when monsters like Qt are involved.

@m-waka
Copy link

m-waka commented Apr 29, 2019

+1 for CMake support for easier dependency management and finding the (interface) library via find_package(clipp).
But you should not set CMAKE_CXX_STANDARD but go with modern CMake, using target_compile_features(clipp INTERFACE cxx_std_11).

- Replace CMAKE_CXX_STANDARD with target_compile_features()
- Require CMake version 3.8 for the meta flag "cxx_std_11"
- Export namespace
- Support sub_directory(): include_directory, namespace alias
- Update version to 1.2.3
- Fix some commands and variables for testing
@heavywatal
Copy link
Contributor Author

Thank you @m-waka for the suggestion. Updated CMakeLists.txt accordingly.

@muellan muellan closed this Apr 30, 2019
@muellan muellan reopened this Apr 30, 2019
@muellan
Copy link
Owner

muellan commented Apr 30, 2019

Now that at least one other person has looked over it and the CMakeLists looks a lot cleaner I think it is ready to be merged.

@muellan muellan merged commit 2c32b2f into muellan:master Apr 30, 2019
@heavywatal heavywatal deleted the cmake branch April 30, 2019 10:30
@heavywatal heavywatal mentioned this pull request Jul 12, 2019
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.

None yet

3 participants