-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Feature/add cmake support #187
Feature/add cmake support #187
Conversation
First commit with CMakeLists.txt and tests CMakeLists.txt enabled
added tests to tests/CMakeLists.txt libtbb, benchmark, unittests and fuzztests
Adapt makefiles, cpp to match new include/concurrentqueue include path
typo in CMake file
…a/concurrentqueue into feature/add-cmake-support
This is the cleanest cmake support I've seen yet :-) |
unittests/unittests.cpp | ||
unittests/mallocmacro.cpp | ||
common/simplethread.cpp | ||
common/systemtime.cpp |
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.
As I understand, it doesn't build c_api files as a separated library, so the current approach will have the error like:
/usr/bin/ld: unittests.cpp:(.text._ZN10moodycamel20ConcurrentQueueTests13c_api_enqueueEv[_ZN10moodycamel20ConcurrentQueueTests13c_api_enqueueEv]+0x56): undefined reference to `moodycamel_cq_destroy'
Because of that, it's important to add c_api files to UNITTESTS_SRC
, isn't it? (or add a target to build c_api library)
set(UNITTESTS_SRC
unittests/unittests.cpp
unittests/mallocmacro.cpp
common/simplethread.cpp
common/systemtime.cpp
../c_api/concurrentqueue.cpp
../c_api/blockingconcurrentqueue.cpp
)
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.
I don't think the c_api contribution existed back then.
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.
Oh, okay, in this case it is a necessary to add c_api support to CMake build system.
) | ||
add_executable(unittests ${UNITTESTS_SRC}) | ||
target_link_libraries(unittests PRIVATE concurrentqueue) | ||
add_test(NAME unittests COMMAND unittests) |
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.
unittests
command has --disable-prompt
flag, I think it will be good to add such flag here to remove Press enter to continue
at the end of tests.
There have been a few issues/pr's here related to adding CMake/Package support and it's definitely a great way to make this package more acceptable.
I understand you're not willing to add Conan in this repo (#74), but looks like Cmake is doable given:
fits into ais relatively concise Feature/add cmake support #161build
directoryI looked at both #93 and #161 and #161 seems like cleaner implementation. Both PR's are stale and have been for a long time, so I figured I'd refresh them here.