Skip to content

Added CMake Support #2

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

darren-harton
Copy link

I added basic CMake support so that other projects may use CMake's externalproject_add and find_package to use this library. Tests are not being built, but that can be added if requested.

Testing:

i7-9750H, 6 cores, 12 threads
Ubuntu 20.04.5 LTS
gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0
CMake 3.16.3

I compiled another project using your header files as dependencies. A sample project can be provided if requested.

Copy link
Owner

@Mandrenkov Mandrenkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @darren-harton, I really appreciate the contribution! I'm not really a CMake expert myself, but I have no issue with adopting it as the official way to build the library. 👍

Before merging, there are a few things which should be addressed. These are:

  1. Running cmake . generates a bunch of build artifacts in the root project directory. Ideally, these should be generated inside a build folder to avoid polluting the filesystem.
  2. The files generates by CMake are not captured in the .gitignore but they definitely should be! We should be able to copy over this one with some minor adjustments.
  3. If we're going to delete the Makefile outright, we'll need a way to run the tests as well. Otherwise, they may as well be dead code, right?
  4. The README instructions should need to updated for CMake. I would recommend replacing the current Makefile recipes with CMake invocations to keep things accessible.

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