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: RAPIDJSON_BUILD_THIRDPARTY_GTEST option added. #309

Closed
wants to merge 1 commit into from
Closed

CMake: RAPIDJSON_BUILD_THIRDPARTY_GTEST option added. #309

wants to merge 1 commit into from

Conversation

shindo
Copy link
Contributor

@shindo shindo commented Apr 19, 2015

This option specifies whether to build GTest from thirdparty/gtest submodule or not.

test/CMakeLists.txt reworked to build GTest from sources (submodule or installed on the system).

Actually GTest almost always should be built from thirdparty submodule, but one may want to use installed GTest sources (UNIX-only, right?).

The problem with FindGTestSrc.cmake was that it searches for sources and headers separately, and it's possible to find thirdparty submodule's sources and installed headers which are of different GTest version (eg. on Ubuntu Lucid GTest packages do not provide source, just headers and built libraries).

With RAPIDJSON_BUILD_THIRDPARTY_GTEST option set ON we use submodule's sources and headers only.
If this option is not set we search for sources and headers with hint pathes set to default UNIX values.

This option specifies whether to build GTest from thirdparty/gtest submodule
or not.

test/CMakeLists.txt reworked to build GTest from sources (submodule or
installed on the system).
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 0f39c72 on shindo:pull-gtest into 8d39282 on miloyip:master.

@miloyip miloyip added the build label Apr 20, 2015
@miloyip
Copy link
Collaborator

miloyip commented Apr 20, 2015

I would like to invite @jollyroger to review on this.

@shindo
Copy link
Contributor Author

shindo commented Apr 20, 2015

Sure, it would be great if someone will review this.

I see several options here:

  • limit GTest sources with thirdparty/gtest (one should checkout this submodule if they want to build/run tests)
  • support custom path to GTest sources (if one has their own downloaded GTest sources)
  • support installed GTest packages (UNIX-only)

I'm not sure if all these options make sense, probably it's enough to use thirdparty/gtest.
This way you can be sure that needed GTest version is used and do not think about older versions.

@jollyroger
Copy link
Contributor

GTest was distributed along with pre-built library up till squeeze in Debian and up till lucid in Ubuntu. It is still distributed as a pre-built library in Fedora although there was a request somewhere to distribute full project source code as recommended by upstream developers (see https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=726021)

Debian squeeze and Ubuntu lucid are both very old. Support for lucid ends in 10 days. Support for squeeze will end in February, 2016 (there's only minor security support left, though all other support was dropped on the 31th of May, 2014). Thus I don't think there's a problem with lucid support (I dare you try and build RapidJSON with libgtest-dev version 1.3.0 distributed with lucid or with 1.5.0 distributed with squeeze).

I wrote FindGTestSrc.cmake to allow package maintainers to use system-wide installed libgtest-dev by default if itis installed. This could be a bit harsh and you may want to use thirdparty/gtest if not stated otherwise. This is where your option is useful. Anyway, maintainers are used to pass variables in order to get result they desire.

You make a valid point saying that there is a possibility to find sources and headers in different places and that was done precisely because Debian/Ubuntu distributions install headers and sources in different places(as stated in the comments to the FindGTestSrc.cmake). But mixing search for GTest back into main CMakeLists.txt is a very bad choice. Thus I recommend:

  • implement all changes and checking all options in the FindGTestSrc.cmake and add only an option to the main CMakeLists.txt. You can handle your option inside FindGTestSrc.cmake file, you know. Find*.cmake files are created exactly to handle all possible weird and dirty thirdparty software installations and leave other project files clean.
  • Add additional checks for source and header files to match each other, if possible, by checking content, not by path. If not, add an exception for split install configuration in Debian/Ubuntu. We could obviously state here that any custom GTest installation will be full (e.g. file location will be the same as in thirdparty/gtest) and won't be placed in system-wide locations (such as /usr/src and /usr/include).
  • Make thirdparty/gtest a default option if desired.

As a last resort for the users, they could always override path problems by setting variables directly during cmake run.

@jollyroger
Copy link
Contributor

As for options @shindo mentioned:

  • using thirdparty/gtest only is hardly the preferred one
  • custom path to GTest sources could be specified even now by running cmake -DGTEST_SOURCE_DIR=/usr/src/gtest -DGTEST_INCLUDE_DIR=/usr/include ..; in fact you may override any variable in such way
  • support for system-wide installations of GTest is done already, but for modern source installations

@shindo
Copy link
Contributor Author

shindo commented Apr 20, 2015

@jollyroger, thank you!

I didn't think of specifying GTEST_SOURCE_DIR and GTEST_INCLUDE_DIR with -D option, this can actually solve most problems.

I encountered a problem with the current scripts on Lucid when headers were found in /usr/include and sources in thirdparty/gtest.

I'm just not sure if installed libgtest-dev should have higher priority than thirdparty/gtest.

@jollyroger
Copy link
Contributor

If that is the case, then it would be enough to set GTEST_INCLUDE_DIR directly if GTest source code was found only in thirdparty/gtest since old versions of libgtest-dev don't distribute GTest source files at all. This should fix your problem. As for using system-wide gtest as default option, I'd leave this question to @miloyip.

@shindo
Copy link
Contributor Author

shindo commented Apr 21, 2015

Well, yes, I agree with you. But changing internal variables this way looks dirty (still possible, and I'll go with it).

As a user I want to control the behaviour of the build process like:

  • "build gtest from thirdparty", because there's submodule already
  • "build gtest from these sources", just like thirdparty, but from different location
  • "build gtest from system-wide gtest" (UNIX-only)

Of course, with the least possible amount of options to change.

This is just my thoughts and expectations from the build scripts. I'll accept your solution, thanks.

@jollyroger
Copy link
Contributor

@shindo , see my implementation for your request (#312). It implements the 1st and 3dr option. It is not possible however to use custom gtest installation without setting GTEST_SOURCE_DIR variable directly. However this PR allows you to specify only GTEST_SOURCE_DIR since cmake looks for GTEST_INCLUDE_DIR in it at first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants