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

Feature/add cmake support #161

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

Conversation

kobi-ca
Copy link

@kobi-ca kobi-ca commented Jul 24, 2019

Added cmake support.
Did not touch xcode/msvc since I dont have access to this.

gnu makefile - modified but did not try it out (will give it a shot - it should be good)

@mikeroe please review as well.

Kobi Cohen-Arazi added 5 commits July 18, 2019 10:46
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
@kobi-ca
Copy link
Author

kobi-ca commented Jul 24, 2019

change includes install of the headers and also generate .cmake to be used by other cmake projects.

)
target_link_libraries(concurrentqueue INTERFACE Threads::Threads rt)

include(CMakePackageConfigHelpers)
Copy link
Author

Choose a reason for hiding this comment

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

line 21 to end of file is all about install and generate of .cmake for header only lib

@@ -31,15 +31,15 @@ benchmarks: bin/benchmarks$(EXT)

bin/unittests$(EXT): ../concurrentqueue.h ../blockingconcurrentqueue.h ../tests/unittests/unittests.cpp ../tests/unittests/mallocmacro.cpp ../tests/common/simplethread.h ../tests/common/simplethread.cpp ../tests/common/systemtime.h ../tests/common/systemtime.cpp ../tests/corealgos.h ../tests/unittests/minitest.h makefile
test -d bin || mkdir bin
g++ -std=c++11 -Wall -pedantic-errors -Wpedantic -Wconversion $(OPTS) -fno-elide-constructors ../tests/common/simplethread.cpp ../tests/common/systemtime.cpp ../tests/unittests/unittests.cpp -o bin/unittests$(EXT) $(LD_OPTS)
Copy link
Author

Choose a reason for hiding this comment

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

TODO - need to test gnu make with the new includes

@@ -29,7 +29,7 @@
#include <windows.h>
#endif

#include "../../concurrentqueue.h"
Copy link
Author

Choose a reason for hiding this comment

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

these changes required to conform to the / syntax of other std libraries released for public and installed under //...

@cameron314
Copy link
Owner

cameron314 commented Jul 24, 2019

If you want to add cmake support for Linux that's fine, but:

  • It should be optional (the existing files should not need to be changed/moved)
  • The cmake files should ideally be under build/ somewhere (EDIT: I understand that cmake likes to litter its files across each folder -- this is OK since it seems unavoidable)

@kobi-ca
Copy link
Author

kobi-ca commented Jul 25, 2019

Hi

Thanks for your feedback
Cmake is optional.
The reason I changed the location of the files was to be aligned to how standard open source libs behaves in terms of look and feel.
When someone installs the headers on the machine the include should be:
Libname/header

I can move the cmake to the build dir. It's going to be a bit less conforming to how other cmake prjs behave but that's minor I think.

@cameron314
Copy link
Owner

I understand the layout is non-standard, but the intention was for it to be a header-only library in order to avoid mucking with build systems :-) That's why the two headers are in the root. Everything else is just gravy.

But I appreciate the pull request, sorry for being a bit abrupt earlier. I'm just picky.

@kobi-ca
Copy link
Author

kobi-ca commented Jul 25, 2019 via email

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