ros / urdfdom

URDF parser
http://ros.org/wiki/urdf
Other
97 stars 131 forks source link

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

Open chhtz opened 3 months ago

chhtz commented 3 months ago

gtest was previously removed from this repository 923c05a54228185aaf2a26aa33e68b46f7b0b5ba, however got merged back later 5a729c11a6b647947ed156a7582cff59049057c1.

Providing a fixed version of this causes problems if a newer version is already installed. Suggestions:

clalancette commented 3 months ago
  • 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.

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 commented 3 months ago

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: https://github.com/google/googletest/issues/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 commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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)