Am 14.08.19 um 05:09 schrieb John Ralls:
On Aug 13, 2019, at 2:54 PM, Christian Gruber <christian.gru...@posteo.de>
wrote:
Am 13.08.19 um 02:45 schrieb John Ralls:
On Aug 12, 2019, at 3:12 PM, Christian Gruber <christian.gru...@posteo.de>
wrote:
Following my previous thread "[GNC-dev] Contribute to GnuCash development" I
opened a new topic thread about reworking GoogleTest integration.
At first some investigation results on bug 797344
<https://bugs.gnucash.org/show_bug.cgi?id=797344>.
In function gnc_gtest_configure() in file common/cmake_modules/GncAddTest.cmake
the two CMake variables GTEST_INCLUDE_DIR and GMOCK_INCLUDE_DIR are set as
follows:
find_path(GTEST_INCLUDE_DIR gtest/gtest.h HINTS ${GTEST_ROOT}/include
${GMOCK_ROOT}/gtest/include /usr/include)
find_path(GMOCK_INCLUDE_DIR gmock/gmock.h HINTS ${GMOCK_ROOT}/include
/usr/include)
This means, as long as GTEST_ROOT and GMOCK_ROOT are defined and refer to a
valid GoogleTest repository, header files gtest.h and gmock.h are found there
and the two variables GTEST_INCLUDE_DIR and GMOCK_INCLUDE_DIR will refer to the
include directories within this GoogleTest repository.
In contrast to that the four CMake variables GTEST_MAIN_LIB, GTEST_SHARED_LIB,
GMOCK_MAIN_LIB and GMOCK_SHARED_LIB are set in the same function
gnc_gtest_configure() as follows:
find_library(GTEST_MAIN_LIB gtest_main)
find_library(GTEST_SHARED_LIB gtest)
find_library(GMOCK_MAIN_LIB gmock_main)
find_library(GMOCK_SHARED_LIB gmock)
This means, libraries are always searched in the default CMake search paths.
Therefore any preinstalled GoogleTest libraries will be found this way.
This explains the behaviour described in my bug report.
Now let's come to my ideas regarding rework of GoogleTest integration. After
further studying CMake files I have two general questions at first.
1. Why are library targets "gtest" and "gmock" defined in GnuCash build system
instead of importing them from GoogleTest build results?
In file common/test-core/CMakeLists.txt these two targets are defined via
add_library(). The same is done within GoogleTest build system in files
googletest/CMakeLists.txt and googlemock/CMakeLists.txt. So part of the CMake
build system from GoogleTest repository is copied into GnuCash build system.
My idea is to build and install GoogleTest completely independent from GnuCash
using its own build system and then importing targets via find_package() into
GnuCash build system. GoogleTest build system provides CMake configuration
files after installation ready for importing targets into other projects.
CMake function find_package() is able to find package GTest via CMake configuration
files, i.e. prebuilt from source code repository, as well as preinstalled libraries
from distro using internal CMake module FindGTest
<https://cmake.org/cmake/help/v3.5/module/FindGTest.html>. Beginning from CMake
version 3.5 module FindGTest provides imported targets GTest::GTest and GTest::Main.
This would avoid mixing of libraries and header files from different locations.
Additionally the user doesn't have to define extra environment variables
GTEST_ROOT and GMOCK_ROOT anymore. Instead the user can configure CMake search
path using CMake variable CMAKE_PREFIX_PATH, if desired.
And the user can choose, which GoogleTest installation should be used by
influencing the CMake search path. So the requirement, that one can decide
whether to use preinstalled GoogleTest libraries from distro or GoogleTest
installation prebuilt from sources would be fulfilled.
2. Why is library target "gtest" not used directly instead of variables
GTEST_LIB, GTEST_INCLUDE_DIR?
Several test applications are defined via function gnc_add_test() by passing a
list of sources, include directories and libraries to that function. This
function gnc_add_test() is defined in file
common/cmake_modules/GncAddTest.cmake and passes the list of libraries
(TEST_LIBS_VAR_NAME) after resolving the variable names to CMake function
target_link_libraries().
My idea is to add library target "gtest" directly to that list of libraries
instead of variable GTEST_LIB, which is possible since CMake function
target_link_libraries() can handle libraries as well as CMake targets. The advantage is,
that you don't have to handle include directories separately on your own. When passing
CMake targets to function target_link_libraries() CMake does all the stuff for you.
Include directories defined for that target are added automatically, i.e.
target_include_directories() doesn't have to be invoked to add GTEST_INCLUDE_DIR.
A further advantage is, that all test application targets depend on library target "gtest". And if library
target "gtest" is not an imported target, it would be rebuilt automatically if necessary, when building test
applications. Currently after any change to GoogleTest sources, e.g. checking out another version, you have to rebuilt
GoogleTest libraries at first via "make gtest" for instance before you can rebuilt test applications via
"make check" for instance. And if you forget that, you get a mix of GoogleTest header files and libraries
from different versions.
Unfortunatelly in case of an imported target (see my first question), automatic
rebuilt is not easily possible. In this case you have to define some extra
custom command to invoke external GoogleTest build system.
These two questions are important to be answered before proceeding. Are there
any considerations or constraints, which have to be taken into account, e.g.
backwards compatibility?
Last there is another question.
3. Why has GTEST_SRC to be added to the list of sources passed to function
gnc_add_test()?
Variable GTEST_SRC is set to ${GTEST_SRC_DIR}/src/gtest_main.cc in function
gnc_gtest_configure() in file common/cmake_modules/GncAddTest.cmake. But as far
as I can see this source file is already compiled and archived within library
libgtest_main.a, which is also contained in list of libraries passed to
function gnc_add_test() via variable GTEST_LIB.
Christian,
A little history: We started using Googletest 1.7.0 in 2014, when our build
system was based on autotools. The autotools GTest integration was built from
the recommendations of the GTest Readme file of the day
(https://github.com/google/googletest/tree/release-1.7.0). When we begun using
CMake I wrote gnc_gtest_configure mostly as a port from the autotools
configuration before I'd learned much about CMake.
1. Building Googletest from inside the GnuCash build system ensures that it is
built with the same compiler flags as GnuCash itself. C++ used to break
spectacularly if that wasn't done. It's gotten much better about it in recent
years though it's still among the top 10 complaints reported by the standard
committee. It would be my preference to always build directly from source this
way but some Linux bistro packagers insist that we must use the included
package; in some cases those packages provide a shared library instead of a
static one, something that the Googletest developers specifically recommend
against (or at least did back in v1.7).
2. The GTEST_LIBS and GTEST_INCLUDE_DIRS are holdovers from the old autotools
build system. You're correct that using the CMake target would be better.
The only problem with FindGTest is that there's no corresponding FindGMock, see
https://gitlab.kitware.com/cmake/cmake/issues/17365.
3. Because that v1.7.0 Readme doesn't say anything about building
libgtest_main.a, it just said to build gtest-all.cc into libgtest.a. That
readme also warns against even looking at the provided configure.ac, which does
build libgtest_main.a, and says to use the compile-by-hand instructions. So I
did that, promptly got symbol not found errors for the functions in
gtest_main.cc, and simply added that to the source.
1. Ok, I understand. That means you not only want GoogleTest to be built from source code. You also
want GoogleTest to be built inside GnuCash build system to ensure identical compiler flags. In this
case my proposal is not sufficient. Another approach to avoid copying CMake code could be to add
GoogleTest build system to GnuCash build system via CMake function add_subdirectory(). Looking into
GoogleTest Readme I see, that this is proposed there as well (see "Using CMake" >
"Incorporating Into An Existing CMake Project"):
"If you want to use gtest in a project which already uses CMake, then a more robust
and flexible approach is to build gtest as part of that project directly. This is done by
making the GoogleTest source code available to the main build and adding it using CMake's
`add_subdirectory()` command. This has the significant advantage that the same compiler
and linker settings are used between gtest and the rest of your project, so issues
associated with using incompatible libraries (eg debug/release), etc. are avoided."
Furthermore this Readme recommends four different methods to provide source
code:
* Download the GoogleTest source code manually and place it at a known
location. This is the least flexible approach and can make it more
difficult
to use with continuous integration systems, etc.
* Embed the GoogleTest source code as a direct copy in the main project's
source tree. This is often the simplest approach, but is also the hardest
to
keep up to date. Some organizations may not permit this method.
* Add GoogleTest as a git submodule or equivalent. This may not always be
possible or appropriate. Git submodules, for example, have their own set of
advantages and drawbacks.
* Use CMake to download GoogleTest as part of the build's configure step. This
is just a little more complex, but doesn't have the limitations of the
other
methods.
What do you think?
2. Regarding GMock you're right. But is GMock already used by any GnuCash test?
Searching the code for variable GMOCK_LIB shows no results except in file
common/cmake_modules/GncAddTest.cmake, where it is set.
3. I see, the Readme doesn't really tell you much about the two libraries
libgtest.a and libgtest_main.a and their differences. Looking into the CMake
code (googletest/CMakeLists.txt) brings more clarity. Library libgtest.a
(target gtest) is defined as follows:
cxx_library(gtest "${cxx_strict}" src/gtest-all.cc)
It is built from source file gtest-all.cc. Library libgtest_main.a (target
gtest_main) is defined as follows:
cxx_library(gtest_main "${cxx_strict}" src/gtest_main.cc)
target_link_libraries(gtest_main PUBLIC gtest)
It is built from source file gtest-main.cc. That means when linking
libgtest_main.a into any test application, you shouldn't need to additionally
add gtest_main.cc to the source files of the application.
Furthermore target gtest is set as a dependency to target gtest_main. That
means, when adding gtest_main as a dependency to any application, target gtest
is automatically added as well. So you don't need to add both targets, but
choose what you want, depending on whether your test application provides its
own main function or not. Library libgtest.a (target gtest) is linked in both
cases.
So I would say, passing GTEST_SRC to function gnc_add_test() can be omitted.
I think our distro packagers would object to any option other than relying on
tools outside of the build system to provide the googletest sources and maybe a
prebuilt static lib. It's a one-off for the casual builder and easily scripted
for everyone else. Building gtest is also sufficiently trivial that it's not
all that interesting to call out to guests own CMakeLists.txt instead of just
building the two libraries in ours. OTOH it's possible that might change at
some point, so
Ok, so you want to keep build process as it is at the moment. In this
case at least bug 797344 should be fixed. I'll provide a PR. And I
recommend to use library target gtest directly instead of variables
GTEST_LIB, GTEST_INCLUDE_DIR as stated above. The same I recommend for
library target gmock. I'll add this to the same PR.
Finally I recommend to add a few more notes on the Wiki page
https://wiki.gnucash.org/wiki/Google_Test. For me not familiar with the
GnuCash build system it was not obvious at the beginning, that gtest is
built inside GnuCash build system, because this is a little bit unusual.
It could for instance be explained, how to build gtest and gmock from
inside GnuCash build system via "make gtest" and "make gmock" and that
this way one can test the correct CMake configuration for GnuCash
regarding variables GTEST_ROOT and GMOCK_ROOT. The Wiki page only
explains how to "test the installation" from inside the GoogleTest build
system and tells that this is "not used in practice". But this doesn't
really test the CMake configuration for GnuCash.
I don't think anyone's actually tried doing a mock with GMock yet. There are
hand-rolled ones in the QOF tests, but they use the old Glib test facility.
Most of the C++ work so far has been at the lowest levels so that the C++
classes don't have any mockable dependencies. That will change when we get to
redoing the engine as we'll want to use mock for at least the backend. We don't
want to remove GMock from the dependencies.
I agree about libgtest_main.a. Would you like to make a PR to remove all of the
inclusion of gtest_main.cc and GTEST_SRC?
I opened PR https://github.com/Gnucash/gnucash/pull/552.
Regards,
John Ralls
Regards,
Christian
_______________________________________________
gnucash-devel mailing list
gnucash-devel@gnucash.org
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
