Skip to content

Fix testing CMake issue to resolve Rolling regression #961

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Apr 16, 2025
Merged

Fix testing CMake issue to resolve Rolling regression #961

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
584ad95
Fix unit test CMake bug and small typos
sarcasticnature Apr 15, 2025
47de5c0
Fix bug when building with ros when testing is off
sarcasticnature Apr 15, 2025
40bb1ab
Revert commit and eliminate BUILD_TESTING check
sarcasticnature Apr 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,21 +55,21 @@ if ( ament_cmake_FOUND )

add_definitions( -DUSING_ROS2 )
message(STATUS "------------------------------------------")
message(STATUS "BehaviourTree is being built using AMENT.")
message(STATUS "BehaviorTree is being built using AMENT.")
message(STATUS "------------------------------------------")
include(cmake/ament_build.cmake)

elseif( CATKIN_DEVEL_PREFIX OR CATKIN_BUILD_BINARY_PACKAGE)

add_definitions( -DUSING_ROS )
message(STATUS "------------------------------------------")
message(STATUS "BehaviourTree is being built using CATKIN.")
message(STATUS "BehaviorTree is being built using CATKIN.")
message(STATUS "------------------------------------------")
include(cmake/catkin_build.cmake)
set(catkin_FOUND TRUE)
else()
message(STATUS "------------------------------------------")
message(STATUS "BehaviourTree is being built with conan.")
message(STATUS "BehaviorTree is being built with conan.")
message(STATUS "------------------------------------------")
include(cmake/conan_build.cmake)
endif()
Expand Down
2 changes: 1 addition & 1 deletion tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ set(TEST_DEPENDECIES
foonathan::lexy
bt_sample_nodes)

if(ament_cmake_FOUND AND BUILD_TESTING)
if(ament_cmake_FOUND)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the original check is correct

Suggested change
if(ament_cmake_FOUND)
if(ament_cmake_FOUND AND BUILD_TESTING)

The problem here is that gtest was being found and linked transitively via ament_cmake. We'll likely need to explicitly call find_package(ament_cmake_gtest REQUIRED) before we call ament_add_gtest below and add a <test_depend> for the same in package.xml.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean aren't both of those things true already? See line 43 below for your first point and here: https://github.com/BehaviorTree/BehaviorTree.CPP/blob/4.6.2/package.xml#L27 for the second point

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, as I noted in my comments on the issue, it seems that this case of the if statement isn't even being hit on the build farm

#960 (comment)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the right change. -DBUILD_TESTING=OFF is a widely accepted mechanism to say "don't build the tests". On the buildfarm today, we don't install any of the test dependencies and expect packages to respect -DBUILD_TESTING=OFF.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the issue with the way that the CMake is currently structured is that even with -DBUILD_TESTING=OFF the else case is going to be hit and it will (attempt to) build the tests anyway. I don't disagree that it should work that way, but it would require more changes elsewhere to actually respect that.

My first attempt at fixing it 'respected' BUILD_TESTING (...kind of) but that broke the repo's CI because it builds with testing off but still expects the test executable to be present

I'll take a look at your comment in #960 and respond there as well


find_package(ament_cmake_gtest REQUIRED)

Expand Down
Loading