Regression on ROS 2 Buildfarm for Rolling

[Yadunund]

bwehaviortree_cpp 4.6.2 is failing to build for Rolling on the buildfarm with the error below. This is blocking the announced Rolling sync. We will proceed with the sync which will remove these binaries from Rolling but they will be added once the build is fixed and a new release is bloomed.

23:12:15 -- BTCPP_EXTRA_LIBRARIES: $<BUILD_INTERFACE:ament_index_cpp::ament_index_cpp>;$<BUILD_INTERFACE:/usr/lib/x86_64-linux-gnu/libzmq.so>;$<BUILD_INTERFACE:/usr/lib/x86_64-linux-gnu/libsqlite3.so>
23:12:15 -- BTCPP_LIB_DESTINATION:   lib 
23:12:15 -- BTCPP_INCLUDE_DESTINATION: include 
23:12:15 -- BTCPP_UNIT_TESTS:   ON 
23:12:15 CMake Error at /usr/share/cmake-3.28/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
23:12:15   Could NOT find GTest (missing: GTEST_LIBRARY GTEST_INCLUDE_DIR
23:12:15   GTEST_MAIN_LIBRARY)
23:12:15 Call Stack (most recent call first):
23:12:15   /usr/share/cmake-3.28/Modules/FindPackageHandleStandardArgs.cmake:600 (_FPHSA_FAILURE_MESSAGE)
23:12:15   /usr/share/cmake-3.28/Modules/FindGTest.cmake:270 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
23:12:15   tests/CMakeLists.txt:60 (find_package)

Detailed logs: https://build.ros2.org/view/Rbin_uN64/job/Rbin_uN64__behaviortree_cpp__ubuntu_noble_amd64__binary/61/consoleFull

[Yadunund]

The offending package was likely linking against gtest/ament_cmake_ros transitively but we'll need to be explicit after ros/rosdistro#44971

[sarcasticnature]

I can submit a PR shortly that adds explicit ament_cmake_ros and/or gtest deps, but I'm unsure how to test/verify that the changes will solve the regression.

I'd prefer not to blindly submit a PR, so if @Yadunund (or someone else) could suggest some acceptance criteria that would be great

[kevshin2002]

I believe you just have to build the package and see that it builds.

@sarcasticnature

[sarcasticnature]

@kevshin2002 unfortunately it builds just fine for me w/o the changes on current rolling (as expected) -- I'm assuming there's a ament[_cmake] change that's in the sync that caused the regression.

It looks to me like a transient dep on gtest (as the logs suggest). For example, the package.xml only specifies a dep on ament_cmake_gtest, not gtest itself. The same goes for the ament portions of the cmake

I'll PR something shortly with that in mind

[sarcasticnature]

Ah, hold on, I think the issue is a bit more tricky...

Note that the error is on line 60 of test/CMakeLists.txt (https://github.com/BehaviorTree/BehaviorTree.CPP/blob/4.6.2/tests/CMakeLists.txt#L60)

If this line is being hit, ament_cmake_FOUND and/or BUILD_TESTING are false!

those are both ON/true/1 (whatever) for a vanilla colcon build on my system -- not sure what the setup is for the build farm

[sarcasticnature]

what I assume is happening is that this behavior (using the """wrong""" portion of the if/else) is not new, but upstream changes have made it so that the GTest cmake package is not available to the build (on the build farm) due to it not being explicitly declared in the package.xml.

My suspicion is that ament_cmake_gtest is using a vendored version of gtest... maybe? Or, perhaps more correctly, whatever used to depend on a version of gtest that CMake could find now uses a vendored version (thus invalidating the transient dependency)?

Regardless, I think the correct fix here is to change the if/else to hit the ROS 2 case properly... somehow

[kevshin2002]

Hi @sarcasticnature, i'm not sure if this might be the fix, but could you try adding the two line:

find_package(ament_cmake_ros)
find_package(gtest_vendor)

in cmake/ament_build.cmake?

The root level CMakeLists handles ROS2 build system first before hitting the testing on line 54.

[sarcasticnature]

While that may work, I'm hesitant to throw in dependencies that may not actually be needed -- there's plenty of other packages that don't explicitly depend on those two and still build just fine.

That may be the most expedient solution though, I'm now realizing (based on all the failed CI jobs) that the patch maaaaaaay have some unintended consequences...

[sarcasticnature]

Ok, I think that PR will fix things, but I'm not totally sure (I've tried to test it but can't get 4.6.2 to break as-is).

[cottsay]

I think there's some misunderstanding as to how CMake, CTest, and their related macros work in this package.

The happy path for plain CMake looks like this:

include(CTest)
if(BUILD_TESTING)
  add_executable(...)
  add_test(...)
endif()

The call to include(CTest) will set BUILD_TESTING=ON if it is not already set to something else. This allows the developer to explicitly set -DBUILD_TESTING=OFF on their cmake invocation to disable it if they'd like to.

For ROS packages, the call to find_package(ament_cmake) (or ament_cmake_ros) will also set BUILD_TESTING=ON by default in the same way that include(CTest) does for a non-ROS package.

Colcon doesn't touch BUILD_TESTING implicitly, but it would pass it through if you set it in --cmake-args. For building deb/RPM packages on the buildfarm, we explicitly set -DBUILD_TESTING=OFF. Nobody cared about the test results from the deb builds, so to save time and compute costs we stopped installing dependencies marked as <test_depend> in the package.xml and started passing that flag so that the packages didn't break. This change happened a while back...prior to Iron, maybe sooner.

What changed here is that packages upstream in the rosidl pipeline previously exported a dependency on ament_cmake_ros, which depends on ament_cmake_gtest, and therefore made GTest available. That depenency was dropped. This package was relying on that implicit dependency to supply GTest even when BUILD_TESTING=OFF, which is why it was able to build before despite our best efforts to tell it not to do so. For CMake packages, <test_depend> and the BUILD_TESTING variable work in tandem.

So the fix here is to use BUILD_TESTING properly. Allow either include(CTest) or find_package(ament_cmake) to set the default, and then use the value of BUILD_TESTING to determine whether or not to look for test dependencies and build the tests.

[sarcasticnature]

As I mentioned in your comment on the PR, I don't disagree with that philosophy, but I don't think it's a quick fix without changing some other (perhaps unexpected) things other than just the testing CMakeLists.txt. My goal was to get a quick fix out to address the regression, so I didn't go that route.

I can try and make another PR so that things are 'respecting' BUILD_TESTING, but I'm not going to be able to get to it as quickly. To restate/expand on what I mentioned in the PR comment, the issue is in the else() clause -- right now if BUILD_TESTING is off the 'ROS 2' (ament) method of building is properly skipped, but the tests are attempted to be built anyway in the else (which is there to handle the non-ROS (conan) build case).

The issue with just adding a check for BUILD_TESTING in the else (changing it to an elseif) is that CI seems to utilize the gtest executable but is building with BUILD_TESTING off (as far as I can tell), so the unintended behavior of having the tests build regardless is actually holding things together.

I could very well be wrong about the above (I will have to do some more investigation to be sure), but again I will need more time to look into it

[cottsay]

I'm totally on board with a small change to mitigate the issue. You guys can do whatever you want in your package, my only intent is to explain how the ROS buildfarm, tooling, and standard CMake practices "expect" things to work.

To be clear, though, I don't believe that #961 will unblock the builds on the buildfarm. The error message will go from the previous failure to find_package(GTest) to a failure to find_package(ament_cmake_gtest).

[sarcasticnature]

gotcha, that's good to know and I really appreciate the explanation. I know very little about the build farm so I'm flying a bit blind here.

I guess the short term fix is actually to just include the deps even though they're not needed? perhaps I was a bit too apprehensive of that approach... I would want to fix it eventually though. I'll try and come up with an actual solution tonight