Fixed bug where including relative paths would fail to find the correct file

[asasine]

By updating the current_path to the included file's absolute path before recursing, all relative paths are relative to the included file. After recursively including the new tree, the change to current_path is undone.

Added unit tests to verify new behavior. The new unit tests aren't built for catkin or ament because I wasn't sure where to copy the test tree files.

Because the trees are included using relative paths to the test executable, they fail if the current working directory is not the same as the trees/ directory i.e., ./behaviortree_cpp_v3_test works but ./bin/behaviortree_cpp_v3_test fails. I'd like to resolve this prior to merging but not sure how to.

Fixes #324

[asasine]

I was able to make the tests pass regardless of which path the test executable is called in commit 9f9e4ab. It works by adding a gtest environment, which uses argc and argv to get the absolute path of the test executable, and making it accessible to tests through an extern variable.

The new unit tests still aren't built for ROS1 or ROS2. @facontidavide should I add these tests to those environments or is it fine to leave them out? I think it's just a matter of copying the trees/ directory to the proper path, but I don't have any ROS installations to test this with.

[facontidavide]

I am unavailable the next week, but I will try to review and merge this after that

[asasine]

@facontidavide it seems like a recent change in #373 has unintentionally broken tree inclusion. When including a file that has a tree with the main_tree_to_execute attribute, an exception is thrown. The most recent CI run on this branch shows the new tests I added are failing:

C++ exception with description "The attribute [main_tree_to_execute] has been found multiple times. You must specify explicitly the name of the <BehaviorTree> to instantiate." thrown in the test body.

https://github.com/asasine/BehaviorTree.CPP/runs/6429134096?check_suite_focus=true#step:7:203

One thing of note is that this CI scenario doesn't seem to be included in the PR checks.

[asasine]

I added this scenario in PR checks so the regression would be caught in the future. @facontidavide have you had a chance to review this PR and/or fix the regression?

[facontidavide]

The environment trick is very neat. Thanks

[facontidavide]

[ 59%] Building CXX object tests/CMakeFiles/behaviortree_cpp_v3_test.dir/gtest_ports.cpp.o
[ 60%] Building CXX object tests/CMakeFiles/behaviortree_cpp_v3_test.dir/navigation_test.cpp.o
[ 61%] Building CXX object tests/CMakeFiles/behaviortree_cpp_v3_test.dir/gtest_subtree.cpp.o
[ 63%] Building CXX object tests/CMakeFiles/behaviortree_cpp_v3_test.dir/gtest_switch.cpp.o
[ 64%] Building CXX object tests/CMakeFiles/behaviortree_cpp_v3_test.dir/gtest_coroutines.cpp.o
[ 65%] Linking CXX executable behaviortree_cpp_v3_test
Error copying directory from "/tmp/ws/src/behaviortree_cpp/tests/trees" to "/trees".
gmake[2]: *** [tests/CMakeFiles/behaviortree_cpp_v3_test.dir/build.make:327: tests/behaviortree_cpp_v3_test] Error 1
gmake[2]: *** Deleting file 'tests/behaviortree_cpp_v3_test'
gmake[1]: *** [CMakeFiles/Makefile2:373: tests/CMakeFiles/behaviortree_cpp_v3_test.dir/all] Error 2
gmake: *** [Makefile:146: all] Error 2
---
--- stderr: behaviortree_cpp_v3
CMake Warning:
  Manually-specified variables were not used by the project:

    CATKIN_INSTALL_INTO_PREFIX_ROOT
    CATKIN_TEST_RESULTS_DIR


Error copying directory from "/tmp/ws/src/behaviortree_cpp/tests/trees" to "/trees".
gmake[2]: *** [tests/CMakeFiles/behaviortree_cpp_v3_test.dir/build.make:327: tests/behaviortree_cpp_v3_test] Error 1
gmake[2]: *** Deleting file 'tests/behaviortree_cpp_v3_test'
gmake[1]: *** [CMakeFiles/Makefile2:373: tests/CMakeFiles/behaviortree_cpp_v3_test.dir/all] Error 2
gmake: *** [Makefile:146: all] Error 2
---
Failed   <<< behaviortree_cpp_v3 [56.3s, exited with code 2]

[asasine]

The broken build appears to be from a missing ${CMAKE_RUNTIME_OUTPUT_DIRECTORY} variable

[facontidavide]

Atually:

Error copying directory from "/tmp/ws/src/behaviortree_cpp/tests/trees" to "/trees".

This is happening on the Open Robotics CI server. Not sure how to set those variables

[asasine]

It’s set by the root CMakeLists.txt

BehaviorTree.CPP/CMakeLists.txt

Line 255 in c14f25c

set(CMAKE_RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/${BEHAVIOR_TREE_BIN_DESTINATION}" )

[asasine]

@facontidavide I believe this is failing because the Open Robotics CI server is building with ament, BUILD_TESTING might be off, and BUILD_UNIT_TESTS is on by default. Therefore, the ROS2/ament branch of tests/CMakeLists.txt would be skipped, and the pure-CMake block in tests/CMakeLists.txt is executed.

BehaviorTree.CPP/tests/CMakeLists.txt

Lines 53 to 69 in 4fa2177

	elseif(BUILD_UNIT_TESTS)
	enable_testing()
	add_executable(${BEHAVIOR_TREE_LIBRARY}_test ${BT_TESTS})
	target_link_libraries(${PROJECT_NAME}_test ${BEHAVIOR_TREE_LIBRARY}
	bt_sample_nodes gtest gtest_main)
	target_include_directories(${BEHAVIOR_TREE_LIBRARY}_test PRIVATE gtest/include ${GTEST_INCLUDE_DIRS})
	add_custom_command(TARGET ${BEHAVIOR_TREE_LIBRARY}_test POST_BUILD
	COMMAND ${CMAKE_COMMAND} -E copy_directory
	${CMAKE_SOURCE_DIR}/tests/trees
	${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/trees)
	add_test(BehaviorTreeCoreTest ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/${BEHAVIOR_TREE_LIBRARY}_test)
	endif()

Since CMAKE_RUNTIME_OUTPUT_DIRECTORY is never set, ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/trees evaluates to /trees, which cmake would not have valid permissions to write to.

My recommendation is to remove BUILD_UNIT_TESTS in favor of the canonical BUILD_TESTING flag instead. Alternatively, defaulting BUILD_UNIT_TESTS to the value of BUILD_TESTING might work too.

[facontidavide]

I think the problem is not if the test is build or not. If it is built even if BUILD_TESTING is not set, no bug deal.

The problem is really
set(CMAKE_RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/${BEHAVIOR_TREE_BIN_DESTINATION}" )

And the fact that CMAKE_BINARY_DIR is not set.

Have a look at this, I think it is a better way to address the issue: #398

[asasine]

Removing the use of CMAKE_RUNTIME_OUTPUT_DIRECTORY is also good as it's extra bloat but I don't think it's the root problem.

It's safe to rely on CMAKE_BINARY_DIR being set by CMake https://cmake.org/cmake/help/v3.10/variable/CMAKE_BINARY_DIR.html. A bigger issue (imo) is that tests are being built when the caller is requesting them not to be through the canonical flag BUILD_TESTING=OFF, and the code in tests/CMakeLists.txt for building pure-CMake tests assumes the same corresponding block in the top-level CMakeLists.txt is also executed, which it is not. This can be replicated reliably with a colcon build --cmake-args -DBUILD_TESTING=OFF from a ROS2-sourced shell.

---

This entire block

BehaviorTree.CPP/CMakeLists.txt

Lines 250 to 257 in 4fa2177

	else()
	set( BEHAVIOR_TREE_LIB_DESTINATION lib )
	set( BEHAVIOR_TREE_INC_DESTINATION include )
	set( BEHAVIOR_TREE_BIN_DESTINATION bin )
	set(CMAKE_RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/${BEHAVIOR_TREE_BIN_DESTINATION}" )
	set(CMAKE_LIBRARY_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/${BEHAVIOR_TREE_LIB_DESTINATION}" )
	set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/${BEHAVIOR_TREE_BIN_DESTINATION}" )

isn't even executed in the Open Robotics CI build. That build would be calling this branch instead

BehaviorTree.CPP/CMakeLists.txt

Lines 234 to 245 in 4fa2177

	if(ament_cmake_FOUND)
	find_package(ament_index_cpp REQUIRED)
	ament_target_dependencies(${BEHAVIOR_TREE_LIBRARY} PUBLIC ament_index_cpp)
	ament_export_dependencies(ament_index_cpp)
	set( BEHAVIOR_TREE_LIB_DESTINATION lib )
	set( BEHAVIOR_TREE_INC_DESTINATION include )
	set( BEHAVIOR_TREE_BIN_DESTINATION bin )
	ament_export_include_directories(include)
	ament_export_libraries(${BEHAVIOR_TREE_LIBRARY})
	ament_package()

since ament_cmake_FOUND would be true.

The bug in the Open Robotics CI build only crops up because this branch of test/CMakeLists.txt

BehaviorTree.CPP/tests/CMakeLists.txt

Lines 53 to 67 in 4fa2177

	elseif(BUILD_UNIT_TESTS)
	enable_testing()
	add_executable(${BEHAVIOR_TREE_LIBRARY}_test ${BT_TESTS})
	target_link_libraries(${PROJECT_NAME}_test ${BEHAVIOR_TREE_LIBRARY}
	bt_sample_nodes gtest gtest_main)
	target_include_directories(${BEHAVIOR_TREE_LIBRARY}_test PRIVATE gtest/include ${GTEST_INCLUDE_DIRS})
	add_custom_command(TARGET ${BEHAVIOR_TREE_LIBRARY}_test POST_BUILD
	COMMAND ${CMAKE_COMMAND} -E copy_directory
	${CMAKE_SOURCE_DIR}/tests/trees
	${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/trees)
	add_test(BehaviorTreeCoreTest ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/${BEHAVIOR_TREE_LIBRARY}_test)

assumes that the pure CMake branch of the top-level CMakeLists.txt was executed which sets CMAKE_RUNTIME_OUTPUT_DIRECTORY but that's branch isn't executed, the ament_cmake_FOUND one is. ament_cmake branch in the top-level CMakeLists.txt, pure CMake in the tests/CMakeLists.txt.

---

Another option is to restructure the branch in tests/CMakeLists.txt to look like this

if(ament_cmake_FOUND)
  if(BUILD_TESTING)
    # ...
  endif()
elseif(catkin_FOUND)
  if(CATKIN_ENABLE_TESTING)
    # ...
  endif()
else()
  if(BUILD_UNIT_TESTS)
    # ...
  endif()
endif()

or restructure the add_subdirectory(tests) to look like this

if(BUILD_SAMPLES AND ((ament_cmake_FOUND AND BUILD_TESTING) OR (catkin_FOUND AND CATKIN_ENABLE_TESTING) OR (NOT ament_cmake_FOUND AND NOT catkin_FOUND AND BUILD_UNIT_TESTS)))
  add_subdirectory(tests)
endif()