Don't use (outdated) self-provided gtest version (especially if another version is installed already) about urdfdom

[clalancette]

• If gtest is not installed, don't run the tests

That means that things "silently" pass tests, which I'm not a huge fan of.

• If gtest is not installed, install it via cmake's ExternalProject

I guess I don't see how this is materially different from what we have currently. Either way, we are vendoring gtest.

All of that said, I'm curious what problems you are having. The way that gtest is supposed to work is that you compile it alongside your code, which is exactly what we do here. So the fact that it is available elsewhere should have no affect either way on this. Some more information or reproduction steps would be helpful.

[chhtz]

Sorry for the late reply, I was not in office yesterday. A bit about my setup, I have a collection of libraries checked into subfolders of /home/user/workspace which locally install to /home/user/workspace/install (i.e., that is the CMAKE_INSTALL_PREFIX) -- some other library depends on googletest which is why this gets installed to .../include/gtest/...
I don't have any problem if I make sure that the version I install matches the version provided by urdfdom, but if I update the version I get the following errors (in this case manually running make -C build install VERBOSE=1 inside the urdfdom folder -- only the first errors copied):

[ 73%] Building CXX object urdf_parser/test/CMakeFiles/gtest.dir/gtest/src/gtest-all.cc.o
cd /home/user/workspace/control/urdfdom/build/urdf_parser/test && /usr/lib/ccache/c++   -I/home/user/workspace/install/include -I/home/user/workspace/control/urdfdom/urdf_parser/test/gtest/include -I/home/user/workspace/control/urdfdom/urdf_parser/test/gtest -I/home/user/workspace/control/urdfdom/urdf_parser/test  -fvisibility=hidden -O3 -DNDEBUG   -Wall -Wextra -Wpedantic -o CMakeFiles/gtest.dir/gtest/src/gtest-all.cc.o -c /home/user/workspace/control/urdfdom/urdf_parser/test/gtest/src/gtest-all.cc
In file included from /home/user/workspace/install/include/gtest/gtest-message.h:57,
                 from /home/user/workspace/install/include/gtest/gtest-assertion-result.h:46,
                 from /home/user/workspace/install/include/gtest/gtest.h:63,
                 from /home/user/workspace/control/urdfdom/urdf_parser/test/gtest/src/gtest-all.cc:38:
/home/user/workspace/control/urdfdom/urdf_parser/test/gtest/src/gtest-internal-inl.h: In constructor ‘testing::internal::GTestFlagSaver::GTestFlagSaver()’:
/home/user/workspace/install/include/gtest/internal/gtest-port.h:2274:26: error: ‘FLAGS_gtest_death_test_use_fork’ was not declared in this scope; did you mean ‘testing::testing::FLAGS_gtest_death_test_use_fork’?
 2274 | #define GTEST_FLAG(name) FLAGS_gtest_##name
      |                          ^~~~~~~~~~~~
/home/user/workspace/control/urdfdom/urdf_parser/test/gtest/src/gtest-internal-inl.h:168:28: note: in expansion of macro ‘GTEST_FLAG’
  168 |     death_test_use_fork_ = GTEST_FLAG(death_test_use_fork);
      |                            ^~~~~~~~~~
/home/user/workspace/install/include/gtest/internal/gtest-port.h:2274:26: note: ‘testing::testing::FLAGS_gtest_death_test_use_fork’ declared here
 2274 | #define GTEST_FLAG(name) FLAGS_gtest_##name
      |                          ^~~~~~~~~~~~
/home/user/workspace/install/include/gtest/internal/gtest-port.h:2326:26: note: in expansion of macro ‘GTEST_FLAG’
 2326 |   GTEST_API_ extern bool GTEST_FLAG(name); \
      |                          ^~~~~~~~~~
/home/user/workspace/control/urdfdom/urdf_parser/test/gtest/src/gtest-internal-inl.h:74:1: note: in expansion of macro ‘GTEST_DECLARE_bool_’
   74 | GTEST_DECLARE_bool_(death_test_use_fork);
      | ^~~~~~~~~~~~~~~~~~~
/home/user/workspace/control/urdfdom/urdf_parser/test/gtest/src/gtest-internal-inl.h: In destructor ‘testing::internal::GTestFlagSaver::~GTestFlagSaver()’:
/home/user/workspace/install/include/gtest/internal/gtest-port.h:2274:26: error: ‘FLAGS_gtest_death_test_use_fork’ was not declared in this scope; did you mean ‘testing::testing::FLAGS_gtest_death_test_use_fork’?
 2274 | #define GTEST_FLAG(name) FLAGS_gtest_##name
      |                          ^~~~~~~~~~~~
/home/user/workspace/control/urdfdom/urdf_parser/test/gtest/src/gtest-internal-inl.h:192:5: note: in expansion of macro ‘GTEST_FLAG’
  192 |     GTEST_FLAG(death_test_use_fork) = death_test_use_fork_;
      |     ^~~~~~~~~~
/home/user/workspace/install/include/gtest/internal/gtest-port.h:2274:26: note: ‘testing::testing::FLAGS_gtest_death_test_use_fork’ declared here
 2274 | #define GTEST_FLAG(name) FLAGS_gtest_##name
      |                          ^~~~~~~~~~~~
/home/user/workspace/install/include/gtest/internal/gtest-port.h:2326:26: note: in expansion of macro ‘GTEST_FLAG’
 2326 |   GTEST_API_ extern bool GTEST_FLAG(name); \
      |                          ^~~~~~~~~~
/home/user/workspace/control/urdfdom/urdf_parser/test/gtest/src/gtest-internal-inl.h:74:1: note: in expansion of macro ‘GTEST_DECLARE_bool_’
   74 | GTEST_DECLARE_bool_(death_test_use_fork);
      | ^~~~~~~~~~~~~~~~~~~

This issue looks very similar to this issue: google/googletest#4061 -- i.e., having a library which self-provides an outdated version of gtest.

I don't know what gtest really recommends on how to integrate it, e.g. this page recommends fetching it via FetchContent:
https://google.github.io/googletest/quickstart-cmake.html
This page also describes how to find it via pkg-config:
https://google.github.io/googletest/pkgconfig.html

Overall, this issue is not critical for me, I can for now fix my "global" gtest version to 1.11.0 (which appears to be the version you use) -- or I disable BUILD_TESTING for urdfdom in my workspace.

[chhtz]

What would also resolve this (for me) and should not harm anyone's build (I assume), is to add a BEFORE to the include_directories( at the start of urdfdom/urdf_parser/test/CMakeLists.txt (this makes sure that the self-provided gtest is prioritized over any other version which may be installed somewhere).

[chhtz]

@clalancette Any further opinion on this? Any solution would be fine for me (if necessary, I can also provide more information on how to reproduce the issue -- and provide a PR showing that adding BEFORE resolves it).

I could even live with having this closed as invalid and workaround it locally.

[clalancette]

@clalancette Any further opinion on this? Any solution would be fine for me (if necessary, I can also provide more information on how to reproduce the issue -- and provide a PR showing that adding BEFORE resolves it).

Yes, more information on how to reproduce would be much appreciated, since I'm still struggling to understand how this could be a problem.

[chhtz]

I failed to produce a stand-alone MRE. It looks like I have a fork of console_bridge which adds ${CMAKE_INSTALL_PREFIX}/include directly to the INCLUDE_DIRECTORIES property during find_package(console_bridge REQUIRED) instead of defining console_bridge_INCLUDE_DIRS.

I'll probably workaround this locally then ...

[chhtz]

In case someone wants to reproduce the issue, you can run the following in a previously empty folder using bash:

mkdir install && export CMAKE_PREFIX_PATH=${PWD}/install && export CMAKE_MODULE_PATH=${PWD}/install
git clone https://github.com/ros/console_bridge.git
git clone https://github.com/google/googletest.git
git clone https://github.com/ros/urdfdom
(mkdir console_bridge/build && cd console_bridge/build && cmake -DCMAKE_INSTALL_PREFIX=../../install .. && make install -j)
(mkdir googletest/build && cd googletest/build && cmake -DCMAKE_INSTALL_PREFIX=../../install .. && make install -j)
### fake a non-standard find_package(console_bridge):
sed -i "54 i include_directories(\${console_bridge_INCLUDE_DIRS})" urdfdom/CMakeLists.txt
(mkdir urdfdom/build && cd urdfdom/build && cmake -DCMAKE_INSTALL_PREFIX=../../install .. && make install -j)