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

CMake support #192

Merged
merged 10 commits into from
Feb 11, 2015
Merged

CMake support #192

merged 10 commits into from
Feb 11, 2015

Conversation

jollyroger
Copy link
Contributor

This is my try to add CMake support for RapidJSON and close #82. It was tested and is working for:

  • Debian Linux (w/ libgtest-dev installed and w/ gtest as submodule)
  • Windows XP ( VS2010, Cygwin); Building on MSYS/MinGW give many errors on gtest compilation it seems the last supported version of gtest for MSYS is 1.6.0, although it is not enough for RapidJSON to build using this version since there is no method EqFailure (see https://gist.github.com/jollyroger/45f29f4769cc41c7251f)

Some features available in this pull request:

  • In-source and out-of-source builds are supported
  • Separate options to build tests, examples and documentation
  • Automatic documentation building (if Doxygen is available and respective option is set)
  • Smart Google Test Toolkit lookup w/ build options check against Win32/Unix
  • Pkgconfig and CMake package lookup and version support (if installed with make install)
  • Running tests with ctest (using make test of ctest -V to get verbose output as before)
  • Compilation flags support provided in CMake Build(issue #82) #126 and Add initial CMake build support. #185

As small addition: I fixed linking errors in the documentation.

@spl
Copy link
Contributor

spl commented Nov 6, 2014

Thanks, @jollyroger. I'll test it on Mac OS X either this weekend or early next week.

@spl
Copy link
Contributor

spl commented Nov 10, 2014

The readme.md needs to be updated to replace the build instructions for premake with instructions for cmake.

@spl
Copy link
Contributor

spl commented Nov 10, 2014

Is there a reason to have in-source builds? If it would simplify anything, I would suggest supporting only out-of-source builds.

@spl
Copy link
Contributor

spl commented Nov 10, 2014

I don't know the proper way to run cmake, so I just did this:

$ cmake .
-- The CXX compiler identification is AppleClang 5.1.0.5030040
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Found Doxygen: /usr/local/bin/doxygen (found version "1.8.8")
-- Found GTestSrc: [...]/rapidjson/thirdparty/gtest
-- The C compiler identification is AppleClang 5.1.0.5030040
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Found PythonInterp: /usr/local/bin/python (found version "2.7.8")
-- Looking for include file pthread.h
-- Looking for include file pthread.h - found
-- Looking for pthread_create
-- Looking for pthread_create - found
-- Found Threads: TRUE
-- Configuring done
-- Generating done
-- Build files have been written to: [...]/rapidjson

Is that correct? It pollutes the top-level directory with more files, including DartConfiguration.tcl which is not .gitignored.

@spl
Copy link
Contributor

spl commented Nov 10, 2014

I see that there's now a Makefile, so I run make and get this error:

[ 53%] Building CXX object test/perftest/CMakeFiles/perftest.dir/misctest.cpp.o
In file included from [...]/rapidjson/test/perftest/misctest.cpp:21:
[...]/rapidjson/test/perftest/perftest.h:56:10: fatal error: 'gtest/gtest.h' file not found
#include "gtest/gtest.h"
         ^
1 error generated.
make[2]: *** [test/perftest/CMakeFiles/perftest.dir/misctest.cpp.o] Error 1
make[1]: *** [test/perftest/CMakeFiles/perftest.dir/all] Error 2
make: *** [all] Error 2

@spl
Copy link
Contributor

spl commented Nov 10, 2014

Since make test is mentioned above, I thought I'd try it. But it doesn't seem to actually build the tests, only tries to run them:

$ make clean
$ make test
Running tests...
Test project [...]/rapidjson
    Start 1: perftest
Could not find executable [...]/rapidjson/test/perftest/perftest
Looked in the following places:
[...]/rapidjson/test/perftest/perftest
[...]/rapidjson/test/perftest/perftest
[...]/rapidjson/test/perftest/Release/perftest
[...]/rapidjson/test/perftest/Release/perftest
[...]/rapidjson/test/perftest/Debug/perftest
[...]/rapidjson/test/perftest/Debug/perftest
[...]/rapidjson/test/perftest/MinSizeRel/perftest
[...]/rapidjson/test/perftest/MinSizeRel/perftest
[...]/rapidjson/test/perftest/RelWithDebInfo/perftest
[...]/rapidjson/test/perftest/RelWithDebInfo/perftest
[...]/rapidjson/test/perftest/Deployment/perftest
[...]/rapidjson/test/perftest/Deployment/perftest
[...]/rapidjson/test/perftest/Development/perftest
[...]/rapidjson/test/perftest/Development/perftest
[...]/rapidjson/test/perftest/perftest
[...]/rapidjson/test/perftest/perftest
[...]/rapidjson/test/perftest/Release/perftest
[...]/rapidjson/test/perftest/Release/perftest
[...]/rapidjson/test/perftest/Debug/perftest
[...]/rapidjson/test/perftest/Debug/perftest
[...]/rapidjson/test/perftest/MinSizeRel/perftest
[...]/rapidjson/test/perftest/MinSizeRel/perftest
[...]/rapidjson/test/perftest/RelWithDebInfo/perftest
[...]/rapidjson/test/perftest/RelWithDebInfo/perftest
[...]/rapidjson/test/perftest/Deployment/perftest
[...]/rapidjson/test/perftest/Deployment/perftest
[...]/rapidjson/test/perftest/Development/perftest
[...]/rapidjson/test/perftest/Development/perftest
Unable to find executable: [...]/rapidjson/test/perftest/perftest
1/2 Test #1: perftest .........................***Not Run   0.00 sec
    Start 2: unittest
Could not find executable [...]/rapidjson/test/unittest/unittest
Looked in the following places:
[...]/rapidjson/test/unittest/unittest
[...]/rapidjson/test/unittest/unittest
[...]/rapidjson/test/unittest/Release/unittest
[...]/rapidjson/test/unittest/Release/unittest
[...]/rapidjson/test/unittest/Debug/unittest
[...]/rapidjson/test/unittest/Debug/unittest
[...]/rapidjson/test/unittest/MinSizeRel/unittest
[...]/rapidjson/test/unittest/MinSizeRel/unittest
[...]/rapidjson/test/unittest/RelWithDebInfo/unittest
[...]/rapidjson/test/unittest/RelWithDebInfo/unittest
[...]/rapidjson/test/unittest/Deployment/unittest
[...]/rapidjson/test/unittest/Deployment/unittest
[...]/rapidjson/test/unittest/Development/unittest
[...]/rapidjson/test/unittest/Development/unittest
[...]/rapidjson/test/unittest/unittest
[...]/rapidjson/test/unittest/unittest
[...]/rapidjson/test/unittest/Release/unittest
[...]/rapidjson/test/unittest/Release/unittest
[...]/rapidjson/test/unittest/Debug/unittest
[...]/rapidjson/test/unittest/Debug/unittest
[...]/rapidjson/test/unittest/MinSizeRel/unittest
[...]/rapidjson/test/unittest/MinSizeRel/unittest
[...]/rapidjson/test/unittest/RelWithDebInfo/unittest
[...]/rapidjson/test/unittest/RelWithDebInfo/unittest
[...]/rapidjson/test/unittest/Deployment/unittest
[...]/rapidjson/test/unittest/Deployment/unittest
[...]/rapidjson/test/unittest/Development/unittest
[...]/rapidjson/test/unittest/Development/unittest
Unable to find executable: [...]/rapidjson/test/unittest/unittest
2/2 Test #2: unittest .........................***Not Run   0.00 sec

0% tests passed, 2 tests failed out of 2

Total Test time (real) =   0.00 sec

The following tests FAILED:
      1 - perftest (Not Run)
      2 - unittest (Not Run)
Errors while running CTest
make: *** [test] Error 8

@jollyroger
Copy link
Contributor Author

@spl, I'll update README.md. Meanwhile here is a short example of the in-source and out-of-source builds:

  • In-source: cd project ; cmake . ; make ; make test. This will pollute your source directory with lots of files as youy already saw.
  • Out-of-source: cd project ; mkdir build ; cd build ; cmake .. ; make test. This will hold all build files inside a separate build directory. It doesn't matter where it was created.

Could you please run make in the following way:

VERBOSE=1 make

This will expose all information about commands being run.

There's a weird error you've spotted. According to your previous message with output of cmake, the gtest sources were found in thirdparty/gtest. But your compile command shows that gtest was not found. This could possibly mean that:

  • the GTEST_INCLUDE_DIR wasn't actually included
  • something happened to gtest sources just before make was run (git submodule update --init ?)

Could you please share last 15-20 lines of the failed build with VERBOSE=1?

Concerning tests: they are built during normal make run. AFAIK cmake does not compile tests during make test as this is just an alias for ctest command. So, there's no reason to run make test unless the build finished successfully.

@spl
Copy link
Contributor

spl commented Nov 11, 2014

@jollyroger Thanks for the explanation of the in-source vs. out-of-source builds. I see now that it's not a difference in the configuration but in how cmake is used.

Is there an easy way to remove the extra build-related files after an in-source build? make clean doesn't remove those files.

That's really a shame that make test doesn't build as well as run the tests. Is it possible to change this? For example, there's this SO issue.

Here is the verbose output (... replaces uninteresting output):

$ VERBOSE=1 make
...
Linking CXX static library libgtest_main.a
cd /.../rapidjson/googletest && /usr/local/Cellar/cmake/3.0.2/bin/cmake -P CMakeFiles/gtest_main.dir/cmake_clean_target.cmake
cd /.../rapidjson/googletest && /usr/local/Cellar/cmake/3.0.2/bin/cmake -E cmake_link_script CMakeFiles/gtest_main.dir/link.txt --verbose=1
/usr/bin/ar cr libgtest_main.a  CMakeFiles/gtest_main.dir/src/gtest_main.cc.o
/usr/bin/ranlib libgtest_main.a
/usr/local/Cellar/cmake/3.0.2/bin/cmake -E cmake_progress_report /.../rapidjson/CMakeFiles  5
[ 50%] Built target gtest_main
/Applications/Xcode.app/Contents/Developer/usr/bin/make -f test/perftest/CMakeFiles/perftest.dir/build.make test/perftest/CMakeFiles/perftest.dir/depend
cd /.../rapidjson/test/perftest/CMakeFiles/perftest.dir/DependInfo.cmake --color=
/Applications/Xcode.app/Contents/Developer/usr/bin/make -f test/perftest/CMakeFiles/perftest.dir/build.make test/perftest/CMakeFiles/perftest.dir/build
/usr/local/Cellar/cmake/3.0.2/bin/cmake -E cmake_progress_report /.../rapidjson/CMakeFiles 7
[ 53%] Building CXX object test/perftest/CMakeFiles/perftest.dir/misctest.cpp.o
cd /.../rapidjson/test/perftest/misctest.cpp
In file included from /.../rapidjson/test/perftest/misctest.cpp:21:
/.../rapidjson/test/perftest/perftest.h:56:10: fatal error: 'gtest/gtest.h' file not found
#include "gtest/gtest.h"
         ^
1 error generated.
make[2]: *** [test/perftest/CMakeFiles/perftest.dir/misctest.cpp.o] Error 1
make[1]: *** [test/perftest/CMakeFiles/perftest.dir/all] Error 2
make: *** [all] Error 2

It's not including the gtest includes directory. If I add -I/.../rapidjson/thirdparty/gtest/include to the /usr/bin/c++ flags, it builds successfully.

I see there's a difference with the libgtest.a build:

Linking CXX static library libgtest.a
cd /.../rapidjson/googletest && /usr/local/Cellar/cmake/3.0.2/bin/cmake -P CMakeFiles/gtest.dir/cmake_clean_target.cmake
cd /.../rapidjson/googletest && /usr/local/Cellar/cmake/3.0.2/bin/cmake -E cmake_link_script CMakeFiles/gtest.dir/link.txt --verbose=1
/usr/bin/ar cr libgtest.a CMakeFiles/gtest.dir/src/gtest-all.cc.o
/usr/bin/ranlib libgtest.a
/usr/local/Cellar/cmake/3.0.2/bin/cmake -E cmake_progress_report /.../rapidjson/CMakeFiles 4
[ 46%] Built target gtest
/Applications/Xcode.app/Contents/Developer/usr/bin/make -f googletest/CMakeFiles/gtest_main.dir/build.make googletest/CMakeFiles/gtest_main.dir/depend
cd /.../rapidjson && /usr/local/Cellar/cmake/3.0.2/bin/cmake -E cmake_depends "Unix Makefiles" /.../rapidjson /.../rapidjson/thirdparty/gtest /.../rapidjson /.../rapidjson/googletest /.../rapidjson/googletest/CMakeFiles/gtest_main.dir/DependInfo.cmake --color=
/Applications/Xcode.app/Contents/Developer/usr/bin/make -f googletest/CMakeFiles/gtest_main.dir/build.make googletest/CMakeFiles/gtest_main.dir/build
/usr/local/Cellar/cmake/3.0.2/bin/cmake -E cmake_progress_report /.../rapidjson/CMakeFiles 5
[ 50%] Building CXX object googletest/CMakeFiles/gtest_main.dir/src/gtest_main.cc.o
cd /.../rapidjson/googletest && /usr/bin/c++ -I/.../rapidjson/include -I/.../rapidjson/thirdparty/gtest/include -I/.../rapidjson/thirdparty/gtest -DGTEST_HAS_PTHREAD=1 -o CMakeFiles/gtest_main.dir/src/gtest_main.cc.o -c /.../rapidjson/thirdparty/gtest/src/gtest_main.cc

In this case, -I/.../rapidjson/thirdparty/gtest/include is included. And so is -I/.../rapidjson/thirdparty/gtest – not sure if that is needed.

# for include directory separately from source directory.
FIND_PATH(GTEST_INCLUDE_DIR
NAMES gtest/gtest.h
PATH_SUFFIES include
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo? I'm guessing it should be PATH_SUFFIXES.

@jollyroger
Copy link
Contributor Author

@spl yes, that's a typo. Nice catch!

However if GTEST_INCLUDE_DIR was not found, you'd normally get an error during first cmake run. Can you please check the contents of the CMakeCache.txt in the top build directory and tell what is the value of GTEST_INCLUDE_DIR variable?

Concerning the answers on StackOverflow: both have problems. The accepted answer uses different taget name (like make check instead of make test) and requires more CMake instructions to add tests. The second one will supporess compiler output e.g. all warnings and errors. They'll appear in LastTest.log but not in stdout/stderr. And honestly, I just can't decide which one is worse.

 * Support for both in-source and out-of-source builds
 * Set library version to 0.12 to map Debian package
 * Add separate options to build tests, examples and documentation
 * Add pkgconfig lookup support (if installed with `make install`)
 * Add CMake lookup support (if isntalled with `make install`)
 * Add Google Test Source lookup
 * Add CTest support for running tests (use `make test` or `ctest -V`)
@jollyroger
Copy link
Contributor Author

@spl, I have rebased my branch against recent master, here are some changes:

  • fixed typo you found in CMakeModules/FindGTestSrc.cmake
  • added DartConfiguration.tcl to .gitignore
  • added notes on building with CMake
  • binaries are now create in a single bin directory like before

@spl
Copy link
Contributor

spl commented Nov 12, 2014

I believe the instructions should use cmake .. instead of cmake ../rapidjson:

$ cd build
$ cmake ../rapidjson
CMake Error: The source directory "/.../rapidjson/rapidjson" does not exist.
Specify --help for usage, or press the help button on the CMake GUI.
$ cmake ..
-- The CXX compiler identification is AppleClang 5.1.0.5030040
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Found Doxygen: /usr/local/bin/doxygen (found version "1.8.8")
-- Found GTestSrc: /.../rapidjson/thirdparty/gtest
-- The C compiler identification is AppleClang 5.1.0.5030040
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Found PythonInterp: /usr/local/bin/python (found version "2.7.8")
-- Looking for include file pthread.h
-- Looking for include file pthread.h - found
-- Looking for pthread_create
-- Looking for pthread_create - found
-- Found Threads: TRUE
-- Configuring done
-- Generating done
-- Build files have been written to: /.../rapidjson/build

My build was successful this time, and make test passed.

The CMakeCache.txt contains the correct paths:

$ grep GTEST CMakeCache.txt
GTEST_INCLUDE_DIR:PATH=/.../rapidjson/thirdparty/gtest/include
GTEST_SOURCE_DIR:PATH=/.../rapidjson/thirdparty/gtest

Thanks for the updates, @jollyroger!

@jollyroger
Copy link
Contributor Author

@spl, I didn't use this approach here because there's already build directory in the repository, and it is under version control. It's because there's no any pros to run out-of-source build in a separate directory if it will pollute git repo with lots of unversioned files anyway. That's why I proposed in readme to create build directory outside project dir:

$ mkdir ../build
$ cd ../build
$ cmake ../rapidjson
$ make
$ ctest -V
$ sudo make install

There is no problem in your approach if there would be no build directory under version control in rapidjson repository.

@spl
Copy link
Contributor

spl commented Nov 12, 2014

@jollyroger Fair enough. A couple of thoughts on that:

  1. The language in the readme.md is not clear. It says:

    Create directory called `build` nearby rapidjson source directory
    

    This does not exactly say where to create build. “Nearby” is too vague. If the recommendation is to create a directory outside the project directory, that should be explicit. Your comment explains it better than the readme.md.

  2. I think it would be better to suggest a directory inside the rapidjson project directory for a couple of reasons:

    1. It does not add unnecessary directories in an area that may already have other non-RapidJSON-related directories.
    2. It can be a directory that is already .gitignored so that it doesn't show up on git status.
  3. Are we planning to support multiple build systems, e.g. Premake and CMake? If not, than the Premake files in build can go, build/travis-doxygen.sh can be moved (say, to .travis-doxygen.sh to go with .travis.yml), and we can use build as the build directory. Alternatively, we just use a different directory name.

@pah
Copy link
Contributor

pah commented Nov 12, 2014

Regarding (3.) in @spl's last comment, I would suggest to update .travis.yml to use the new Cmake build system already in this branch. This requires changes to

before_install:

  • drop codegear/release PPA (only needed for premake4)
  • install cmake instead of premake4

before_script

  • properly call cmake for $CONF (debug/release) and $BITS (32/64)

script:

  • properly call make to build current configuration
  • run tests

@spl
Copy link
Contributor

spl commented Nov 12, 2014

@pah Good catch!

@jollyroger
Copy link
Contributor Author

@pah, @spl, I'm already working on CI in a separate travis branch: https://github.com/jollyroger/rapidjson/blob/travis/.travis.yml
And here are the first results: https://travis-ci.org/jollyroger/rapidjson/builds/40779912

However I wonder what is the default behaviour for running the tests. Current travis tests disable perftest for release builds and disable objdump check for debug builds. I'm sure most users will desire to run all available tests. I could add a separate option to enable all tests by default but make travis builds behave as before. Sounds ok?

@pah, I've got question about current build procedure: why didn't you install doxygen with apt-get install?

Also, cmake already builds doxygen documentation which makes a lot of code in travis-doxygen.sh unnecessary. Since there's no Doxyfile directly in the source directory (it's generated with CMake from Doxyfile.in; this is required to build documentation in the desired directory) this script requires changes as well. I could try to make this a separate CMake target to replace this script completely.

@pah
Copy link
Contributor

pah commented Nov 12, 2014

Current travis tests disable perftest for release builds and disable objdump check for debug builds.

Actually, it's the other way round for the current Travis configuration. And running perftest on non-optimized builds seems to be unreasonable. The objdump test makes no sense on release builds, as there are no symbols in the resulting binaries (and the test will always "succeed").

For the defaults, I'm fine with running all tests locally.

Why didn't you install doxygen with apt-get install?

Because the version currently available on the Travis machines via APT is not recent enough for the Markdown support we're relying on to generate the detailed user documentation.

Also, cmake already builds doxygen documentation which makes a lot of code in travis-doxygen.sh unnecessary.

No, it only makes the doxygen_run() call obsolete. The remainder of the script is used to generate and upload the documentation to the GitHub pages automatically whenever the master branch has been updated. It's very specific to the Travis setup and should not be added to the default CMake configuration.

@jollyroger
Copy link
Contributor Author

Some updates on travis configuration (see latest commits). Here is an example of successfull travis build:
https://travis-ci.org/jollyroger/rapidjson/jobs/41430238

Also, currently it is possible to run unittests with valgrind enabled. However there is another way to acieve this using ctest's dashboards: http://stackoverflow.com/a/9843594/435302 . So, which approach is better?

  • writing separate tests with valgrind invoked directly (requires adding each test into CMakeLists.txt)
  • typing separate command to run memory check whenever needed using ctest's aproach

@oddkiva
Copy link

oddkiva commented Jan 27, 2015

Hello,

what's the current status of this update? I like this comprehensive PR. I am eager to have this merged into the master branch.
The only missing thing is the compilation flags: would @thebusytypist and @drewnoakes (cf. #126 and #185) be interested in being involved in this PR? I have limited experience with setting compiler flags in CMake but would be happy to help a bit.

For now, I did my own flavor of CMake build system for my urgent needs (cf. https://github.com/davidok8/rapidjson/compare/maint-replace-premake-by-cmake).

@miloyip miloyip mentioned this pull request Feb 11, 2015
miloyip added a commit that referenced this pull request Feb 11, 2015
@miloyip miloyip merged commit 09118fa into Tencent:master Feb 11, 2015
@drewnoakes
Copy link
Contributor

Does the merging of this PR make #82 and #192 obsolete?

@thebusytypist
Copy link
Contributor

Hi @jollyroger,
I have some suggestions on the install process.

Conventionally if a software is going to be installed into a non-default prefix, only CMAKE_INSTALL_PREFIX variable have to be altered. And all files(headers, documents, libs, and binaries) are going to be placed under that prefix.

However, in current build of RapidJSON, I need to modify INCLUDE_INSTALL_DIR, LIB_INSTALL_DIR and DOC_INSTALL_DIR. And the variable CMAKE_INSTALL_PREFIX is actually not used. (Since all XXX_INSTALL_DIRs are CACHEed variables.)

It is OK if this is exactly what you intend for. And it would be better to remind users to change these variables for a custom prefix.

If you instead want to put all those files into the prefix specified by user,
I suggest to not use CACHEed variables.

That is, use

SET(LIB_INSTALL_DIR "${CMAKE_INSTALL_PREFIX}/lib")

rather than

SET(LIB_INSTALL_DIR "${CMAKE_INSTALL_PREFIX}/lib" CACHE STRING "Directory where lib will install")

In this way all XXX_INSTALL_DIR variables won't be displayed for users and hence cannot be edited.

@jollyroger jollyroger deleted the cmake branch April 21, 2015 09:12
@jollyroger
Copy link
Contributor Author

@thebusytypist, I'm sorry for the late reply. Thank you for your well-thought comment.

Yes, I intended this behaviour. It covers the common case when install prefix is set up during the first cmake run. This will calculate all other directoris according to their default values that depend on CMAKE_INSTALL_PREFIX. Though this won't be done if you want to set CMAKE_INSTALL_PREFIX later at some point(with an additional cmake run) and that's what makes your claim valid. However, please consider another case, when you have a few custom XXX_INSTALL_DIR variables that don't exactly match the default rules. In this case you would need to set these variables during every cmake run. Your proposed code will regenerate all dependant variables during every cmake run and that's what I wanted to avoid.

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.

Migrate Premake to CMAKE
7 participants