cmake: No (warning) flags passed during build, several unittests missing

[pah]

After the merge of #192, some issues popped up in the current regression runs:

• All objects are build without any specific compiler flags, instead of the previously passed -march=native -Wall -Wextra -Werror -Weffc++ -Wswitch-default
• Some unittests are not built
  • bigintegertest.cpp
  • stringbuffertest.cpp
  • strtodtest.cpp

I've not reviewed the state of the Cmake support indetail. It might be good to rename travis-doxygen.sh to .travis-doxygen.sh to avoid confusion with this file visible in the main folder.

[miloyip]

Sorry that I should review the PR in more detail first. I just found out there are compilation errors in VS 2013 as well. Do u suggest to revert to PR first?

[jollyroger]

Hi. It seemed there were no such unittests at the time I wrote that PR. These are easily added. As for build flags — while most are harmless, -march flag isn't something I'd like to add because that could be troublesome for package maintainers (to be more precise, Debian maintainers). For example, library built on a i686-capable (packages are built on a modern systems) machine will not run on an i386 target architecture that is supported by Debian due to missing processor instructions.

In case someone wants to build the library himself CFLAGS, CXXFLAGS are supported by cmake out of the box i.e. running CXXFLAGS=-march native cmake .. should be fine.

As for reverting my PR: i'm ok either way. If you revert, I'll rebase my PR on top of current master and add warning flags and unit tests. Otherwise I'll just create another PR. I'm not sure if I could help with VS build though but maybe compilation errors will shed some light.

[miloyip]

Hi @jollyroger . Thank you for your quick response.

I am not familiar with package issues. Will they include binaries for something like RapidJSON? Since the binaries are only unit tests and examples, I think probably should not be distributed as binary. If this is true, then -march should be safe.

I can try to work on the VC warnings/errors tonight.

[jollyroger]

@miloyip , here you can see how previous versions of RapidJSON are distributed: https://packages.debian.org/search?keywords=rapidjson&searchon=names&suite=all&section=all

I'm very sorry, I forgot that it's a source-only library, so you are right, -march flag should be safe.

I'll look on the changes meanwhile and decide what to do with that PR.

[miloyip]

I discovered that in VC2013, gtest uses /MT flag, while unittest uses /MD. This generate linkage errors. Any hint for solving this?

[jollyroger]

I had these Issues when tested against Visual Studio, but I thought I had that thing fixed. there was an option for gtest's cmake to avoid this problem: https://github.com/miloyip/rapidjson/pull/192/files#diff-15547c54d3d4898a882b4ab7b3cee381R7 (test/CMakeLists.txt, line: 7).

Could you try to set this to OFF ?

[miloyip]

I just tried adding set(gtest_force_shared_crt ON) to test/CMakeLists.txt. And it works.

[miloyip]

Besides, shall .gitignore add /build/*?

[jollyroger]

Build directory with exactly this name is not needed anyway: you could build in a totally separate directory (as I did). But as a matter of convenience its ok. It is worth removing all previous lines with /build from .gitignore as well.

[jollyroger]

I fixed all problems @pah mentioned, although I still didn't add the set(gtest_force_shared_crt ON) as @miloyip described. Will see if there's any better way except this workaround. All changes were pushed to the "cmake-update" branch: https://github.com/jollyroger/rapidjson/tree/cmake-update

[jollyroger]

I have to report that this compiler set of options generates valgrind errors for unittest run: -march=native -Wall -Wextra -Werror -Weffc++ -Wswitch-default. Details can be found here: https://travis-ci.org/jollyroger/rapidjson/builds/50360039

[pah]

The Valgrind errors have been worked around previously by forcing -march=native to -march=sse4.2 in .travis.yml, see #171. Quoting from the previous .travis.yml:

#   hack to avoid Valgrind bug (https://bugs.kde.org/show_bug.cgi?id=326469),
#   exposed by merging PR#163 (using -march=native)
  - (cd build/gmake && sed -i 's/march=native/msse4.2/' *.make)

We should update the hack in .travis.yml until a more recent version of Valgrind is available on the Travis CI machines.

[miloyip]

Hi @jollyroger , is there any update on this issue? Are you preparing for a PR? Thanks.

[jollyroger]

I've created PR #244. Please review.

[pah]

Apart from the broken definition on MSVC (see comment above) it looks ok to me.
Don't know, if the changes to .gitignore are needed, though.

[jollyroger]

@pah CMake does not create any temporary files in /thirdparty/lib and /intermediate. Instead only /build directory is needed for out-of-source builds. An additional line with *.a excludes is necessary for ignoring intermediate static library namespacetest.a built in /test/unittest in case of in-source build.