-
Notifications
You must be signed in to change notification settings - Fork 4
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
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this 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:
- 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. - The files generates by CMake are not captured in the
.gitignorebut they definitely should be! We should be able to copy over this one with some minor adjustments. - 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?
- 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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I added basic CMake support so that other projects may use CMake's
externalproject_addandfind_packageto use this library. Tests are not being built, but that can be added if requested.Testing:
I compiled another project using your header files as dependencies. A sample project can be provided if requested.