From efa74c491f5e60a86cf35b59b28540f2e56ceedd Mon Sep 17 00:00:00 2001 From: Joris Vaillant Date: Thu, 9 Nov 2023 16:13:06 +0100 Subject: [PATCH 1/9] binding: Fix memory leak in buildGeomFromUrdf and buildGeomFromUrdfString --- bindings/python/parsers/urdf/geometry.cpp | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/bindings/python/parsers/urdf/geometry.cpp b/bindings/python/parsers/urdf/geometry.cpp index 0485468b81..75dcd5c827 100644 --- a/bindings/python/parsers/urdf/geometry.cpp +++ b/bindings/python/parsers/urdf/geometry.cpp @@ -139,10 +139,20 @@ namespace pinocchio template static PyObject* postcall(ArgumentPackage const& args_, PyObject* result) { + // If owner_arg exist, we run bp::return_internal_reference postcall + // result lifetime will be tied to owner_arg lifetime PyObject* patient = bp::detail::get_prev::execute(args_, result); if (patient != Py_None) return bp::return_internal_reference::postcall(args_, result); - return result; + // If owner_arg doesn't exist, then Python will have to manage the result lifetime + bp::extract geom_model_extract(result); + if (geom_model_extract.check()) + { + return bp::to_python_indirect()(geom_model_extract()); + } + // If returned value is not a GeometryModel*, then raise an error + PyErr_SetString(PyExc_RuntimeError, "pinocchio::python::return_value_policy only work on GeometryModel* data type"); + return 0; } }; From bbfd88a5926297c6a77d49248e432b45ed5b3548 Mon Sep 17 00:00:00 2001 From: Joris Vaillant Date: Thu, 9 Nov 2023 17:42:47 +0100 Subject: [PATCH 2/9] ci: setup ctest verbose output --- .github/workflows/ros-ci.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/ros-ci.yml b/.github/workflows/ros-ci.yml index d28749e7cb..4124e83718 100644 --- a/.github/workflows/ros-ci.yml +++ b/.github/workflows/ros-ci.yml @@ -26,6 +26,7 @@ jobs: # target after completion of the regular test target. The output of this step does affect the output of the CI process. # Note, this does not affect projects that do not have pure CMake projects in their upstream_ws. BUILDER: catkin_tools + CTEST_OUTPUT_ON_FAILURE: 1 AFTER_RUN_TARGET_TEST: 'ici_with_unset_variables source /root/target_ws/install/setup.bash && cd /root/target_ws/build/pinocchio && make test' IMMEDIATE_TEST_OUTPUT: 1 runs-on: ubuntu-latest From 9c83f884066bde9ea40fd625dc6518d84aad9f97 Mon Sep 17 00:00:00 2001 From: Joris Vaillant Date: Fri, 10 Nov 2023 10:01:08 +0100 Subject: [PATCH 3/9] Try to output failed tests errors --- .github/workflows/ros-ci.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/workflows/ros-ci.yml b/.github/workflows/ros-ci.yml index 4124e83718..4c3080ac07 100644 --- a/.github/workflows/ros-ci.yml +++ b/.github/workflows/ros-ci.yml @@ -26,8 +26,7 @@ jobs: # target after completion of the regular test target. The output of this step does affect the output of the CI process. # Note, this does not affect projects that do not have pure CMake projects in their upstream_ws. BUILDER: catkin_tools - CTEST_OUTPUT_ON_FAILURE: 1 - AFTER_RUN_TARGET_TEST: 'ici_with_unset_variables source /root/target_ws/install/setup.bash && cd /root/target_ws/build/pinocchio && make test' + AFTER_RUN_TARGET_TEST: 'ici_with_unset_variables source /root/target_ws/install/setup.bash && cd /root/target_ws/build/pinocchio && ctest --output-on-failure' IMMEDIATE_TEST_OUTPUT: 1 runs-on: ubuntu-latest steps: From 6f0a582b503111e100a4b70bc54c81ef0dae3676 Mon Sep 17 00:00:00 2001 From: Joris Vaillant Date: Fri, 10 Nov 2023 16:57:28 +0100 Subject: [PATCH 4/9] cmake: add test to detect memory leak --- unittest/python/CMakeLists.txt | 51 +++++++++++++++++++ ...ndings_build_geom_from_urdf_memorycheck.py | 30 +++++++++++ .../python/memorycheck_unit_test.cmake.in | 17 +++++++ 3 files changed, 98 insertions(+) create mode 100644 unittest/python/bindings_build_geom_from_urdf_memorycheck.py create mode 100644 unittest/python/memorycheck_unit_test.cmake.in diff --git a/unittest/python/CMakeLists.txt b/unittest/python/CMakeLists.txt index 8d02622b16..3128d08c20 100644 --- a/unittest/python/CMakeLists.txt +++ b/unittest/python/CMakeLists.txt @@ -2,6 +2,45 @@ # Copyright (c) 2015-2021 CNRS INRIA # +macro(ADD_PYTHON_MEMORYCHECK_UNIT_TEST NAME SOURCE) + set(TEST_FILE_NAME memorycheck_unit_test_${NAME}.cmake) + set(PYTHON_TEST_SCRIPT "${PROJECT_SOURCE_DIR}/${SOURCE}") + configure_file(memorycheck_unit_test.cmake.in ${TEST_FILE_NAME} @ONLY) + + add_test(NAME ${NAME} + COMMAND ${CMAKE_COMMAND} -P ${TEST_FILE_NAME}) + + set(MODULES "${ARGN}") # ARGN is not a variable + foreach(MODULE_PATH IN LISTS MODULES) + list(APPEND PYTHONPATH "${CMAKE_BINARY_DIR}/${MODULE_PATH}") + endforeach() + + if(DEFINED ENV{PYTHONPATH}) + list(APPEND PYTHONPATH "$ENV{PYTHONPATH}") + endif() + + # get path separator to join those paths + execute_process( + COMMAND "${PYTHON_EXECUTABLE}" "-c" "import os; print(os.pathsep)" + OUTPUT_VARIABLE PATHSEP + OUTPUT_STRIP_TRAILING_WHITESPACE) + + list(REMOVE_DUPLICATES PYTHONPATH) + if(WIN32) + # ensure that severals paths stay together as ENV variable PYTHONPATH when + # passed to python test via PROPERTIES + string(REPLACE ";" "\;" PYTHONPATH_STR "${PYTHONPATH}") + else(WIN32) + string(REPLACE ";" "${PATHSEP}" PYTHONPATH_STR "${PYTHONPATH}") + endif(WIN32) + set(ENV_VARIABLES "PYTHONPATH=${PYTHONPATH_STR}") + if(APPLE) + list(APPEND ENV_VARIABLES "LD_LIBRARY_PATH=$ENV{LD_LIBRARY_PATH}") + list(APPEND ENV_VARIABLES "DYLD_LIBRARY_PATH=$ENV{DYLD_LIBRARY_PATH}") + endif(APPLE) + set_tests_properties(${NAME} PROPERTIES ENVIRONMENT "${ENV_VARIABLES}") +endmacro() + SET(${PROJECT_NAME}_PYTHON_TESTS bindings @@ -73,10 +112,22 @@ IF(urdfdom_FOUND) ) ENDIF(urdfdom_FOUND) +SET(${PROJECT_NAME}_PYTHON_MEMORYCHECK_TESTS + bindings_build_geom_from_urdf_memorycheck + ) + FOREACH(TEST ${${PROJECT_NAME}_PYTHON_TESTS}) ADD_PYTHON_UNIT_TEST("test-py-${TEST}" "unittest/python/${TEST}.py" "bindings/python") ENDFOREACH(TEST ${${PROJECT_NAME}_PYTHON_TESTS}) MAKE_DIRECTORY("${CMAKE_CURRENT_BINARY_DIR}/serialization-data") +IF(MEMORYCHECK_COMMAND) + FOREACH(TEST ${${PROJECT_NAME}_PYTHON_MEMORYCHECK_TESTS}) + ADD_PYTHON_MEMORYCHECK_UNIT_TEST("test-py-memory-${TEST}" + "unittest/python/${TEST}.py" + "bindings/python") + ENDFOREACH() +ENDIF() + ADD_SUBDIRECTORY(pybind11) diff --git a/unittest/python/bindings_build_geom_from_urdf_memorycheck.py b/unittest/python/bindings_build_geom_from_urdf_memorycheck.py new file mode 100644 index 0000000000..b1d1de8ae6 --- /dev/null +++ b/unittest/python/bindings_build_geom_from_urdf_memorycheck.py @@ -0,0 +1,30 @@ +import os +import unittest + +import pinocchio as pin + + +@unittest.skipUnless(pin.WITH_URDFDOM, "Needs URDFDOM") +class TestBuildGeomFromUrdfMemoryCheck(unittest.TestCase): + def setUp(self): + self.current_file = os.path.dirname(str(os.path.abspath(__file__))) + self.model_dir = os.path.abspath( + os.path.join(self.current_file, "../../models/example-robot-data/robots") + ) + self.model_path = os.path.abspath( + os.path.join(self.model_dir, "ur_description/urdf/ur5_robot.urdf") + ) + + def test_load(self): + model = pin.buildModelFromUrdf(self.model_path) + for i in range(2): + pin.buildGeomFromUrdf( + model, + self.model_path, + pin.COLLISION, + package_dirs=self.model_dir, + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/unittest/python/memorycheck_unit_test.cmake.in b/unittest/python/memorycheck_unit_test.cmake.in new file mode 100644 index 0000000000..ce3cde0d53 --- /dev/null +++ b/unittest/python/memorycheck_unit_test.cmake.in @@ -0,0 +1,17 @@ +SET(PYTHON_EXECUTABLE @PYTHON_EXECUTABLE@) +SET(MEMORYCHECK_COMMAND @MEMORYCHECK_COMMAND@) +SET(PYTHON_TEST_SCRIPT @PYTHON_TEST_SCRIPT@) + +execute_process(COMMAND + ${MEMORYCHECK_COMMAND} -- ${PYTHON_EXECUTABLE} ${PYTHON_TEST_SCRIPT} + ERROR_VARIABLE MEMORYCHECK_OUTPUT) + +string(FIND "${MEMORYCHECK_OUTPUT}" "definitely lost: 0 bytes in 0 blocks" DEFINITELY_LOST) +string(FIND "${MEMORYCHECK_OUTPUT}" "indirectly lost: 0 bytes in 0 blocks" INDIRECTLY_LOST) + +if(${DEFINITELY_LOST} GREATER -1 AND ${INDIRECTLY_LOST} GREATER -1) + message(STATUS "${PYTHON_TEST_SCRIPT} is not leaking memory") +else() + message(FATAL_ERROR "Output: ${MEMORYCHECK_OUTPUT}\n" + "${PYTHON_TEST_SCRIPT} is leaking memory\n") +endif() From 52a663ad0d8efcf66fafe7df7ef47a33d5a38b6b Mon Sep 17 00:00:00 2001 From: Joris Vaillant Date: Fri, 10 Nov 2023 17:26:08 +0100 Subject: [PATCH 5/9] python: don't use that had been introduced in Python2.7... --- .github/workflows/ros-ci.yml | 2 +- unittest/python/utils.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ros-ci.yml b/.github/workflows/ros-ci.yml index 4c3080ac07..d28749e7cb 100644 --- a/.github/workflows/ros-ci.yml +++ b/.github/workflows/ros-ci.yml @@ -26,7 +26,7 @@ jobs: # target after completion of the regular test target. The output of this step does affect the output of the CI process. # Note, this does not affect projects that do not have pure CMake projects in their upstream_ws. BUILDER: catkin_tools - AFTER_RUN_TARGET_TEST: 'ici_with_unset_variables source /root/target_ws/install/setup.bash && cd /root/target_ws/build/pinocchio && ctest --output-on-failure' + AFTER_RUN_TARGET_TEST: 'ici_with_unset_variables source /root/target_ws/install/setup.bash && cd /root/target_ws/build/pinocchio && make test' IMMEDIATE_TEST_OUTPUT: 1 runs-on: ubuntu-latest steps: diff --git a/unittest/python/utils.py b/unittest/python/utils.py index f40caf2920..127fd22b3d 100644 --- a/unittest/python/utils.py +++ b/unittest/python/utils.py @@ -15,9 +15,9 @@ def test_se3ToXYZQUAT_XYZQUATToSe3(self): self.assertApprox(pin.SE3ToXYZQUAT(m).T, [1., 2., 3., sqrt(2) / 2, 0, 0, sqrt(2) / 2]) self.assertApprox(pin.XYZQUATToSE3([1., 2., 3., sqrt(2) / 2, 0, 0, sqrt(2) / 2]), m) self.assertApprox(pin.XYZQUATToSE3(np.array([1., 2., 3., sqrt(2) / 2, 0, 0, sqrt(2) / 2])), m) - with self.assertRaisesRegex(ValueError, "Wrong size: .*"): + with self.assertRaises(ValueError): pin.XYZQUATToSE3([1., 2., 3.]) - with self.assertRaisesRegex(ValueError, "Wrong size: .*"): + with self.assertRaises(ValueError): pin.XYZQUATToSE3(np.array([1., 2., 3., sqrt(2) / 2])) def test_isapprox(self): From fa99f9d5a42c687a86fe0688e725462fef060f41 Mon Sep 17 00:00:00 2001 From: Joris Vaillant Date: Fri, 10 Nov 2023 17:38:58 +0100 Subject: [PATCH 6/9] python: don't add new code issue --- unittest/python/bindings_build_geom_from_urdf_memorycheck.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/unittest/python/bindings_build_geom_from_urdf_memorycheck.py b/unittest/python/bindings_build_geom_from_urdf_memorycheck.py index b1d1de8ae6..08df5204e0 100644 --- a/unittest/python/bindings_build_geom_from_urdf_memorycheck.py +++ b/unittest/python/bindings_build_geom_from_urdf_memorycheck.py @@ -17,7 +17,7 @@ def setUp(self): def test_load(self): model = pin.buildModelFromUrdf(self.model_path) - for i in range(2): + for _ in range(2): pin.buildGeomFromUrdf( model, self.model_path, From 8050c7d569d2c251765aaa15cacc8b029343bedb Mon Sep 17 00:00:00 2001 From: Joris Vaillant Date: Mon, 13 Nov 2023 09:28:15 +0100 Subject: [PATCH 7/9] Update unittest/python/CMakeLists.txt Co-authored-by: Justin Carpentier --- unittest/python/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/unittest/python/CMakeLists.txt b/unittest/python/CMakeLists.txt index 3128d08c20..e7ab98b8ac 100644 --- a/unittest/python/CMakeLists.txt +++ b/unittest/python/CMakeLists.txt @@ -1,5 +1,5 @@ # -# Copyright (c) 2015-2021 CNRS INRIA +# Copyright (c) 2015-2023 CNRS INRIA # macro(ADD_PYTHON_MEMORYCHECK_UNIT_TEST NAME SOURCE) From 082aa28bc6da37ca0e2ad13d84bf5c956c0f38bf Mon Sep 17 00:00:00 2001 From: Joris Vaillant Date: Mon, 13 Nov 2023 13:58:52 +0100 Subject: [PATCH 8/9] python: fix spelling --- bindings/python/parsers/urdf/geometry.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bindings/python/parsers/urdf/geometry.cpp b/bindings/python/parsers/urdf/geometry.cpp index 75dcd5c827..04b204554b 100644 --- a/bindings/python/parsers/urdf/geometry.cpp +++ b/bindings/python/parsers/urdf/geometry.cpp @@ -139,8 +139,8 @@ namespace pinocchio template static PyObject* postcall(ArgumentPackage const& args_, PyObject* result) { - // If owner_arg exist, we run bp::return_internal_reference postcall - // result lifetime will be tied to owner_arg lifetime + // If owner_arg exists, we run bp::return_internal_reference postcall + // result lifetime will be tied to the owner_arg lifetime PyObject* patient = bp::detail::get_prev::execute(args_, result); if (patient != Py_None) return bp::return_internal_reference::postcall(args_, result); @@ -151,7 +151,7 @@ namespace pinocchio return bp::to_python_indirect()(geom_model_extract()); } // If returned value is not a GeometryModel*, then raise an error - PyErr_SetString(PyExc_RuntimeError, "pinocchio::python::return_value_policy only work on GeometryModel* data type"); + PyErr_SetString(PyExc_RuntimeError, "pinocchio::python::return_value_policy only works on GeometryModel* data type"); return 0; } }; From 366fca64f4927a28c10af0b54ab8ce241f384420 Mon Sep 17 00:00:00 2001 From: Joris Vaillant Date: Mon, 13 Nov 2023 14:17:00 +0100 Subject: [PATCH 9/9] cmake: Only run memory checks when memory check tool is valgrind --- unittest/python/CMakeLists.txt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/unittest/python/CMakeLists.txt b/unittest/python/CMakeLists.txt index e7ab98b8ac..8cece0327c 100644 --- a/unittest/python/CMakeLists.txt +++ b/unittest/python/CMakeLists.txt @@ -122,12 +122,14 @@ ENDFOREACH(TEST ${${PROJECT_NAME}_PYTHON_TESTS}) MAKE_DIRECTORY("${CMAKE_CURRENT_BINARY_DIR}/serialization-data") -IF(MEMORYCHECK_COMMAND) +IF(MEMORYCHECK_COMMAND AND MEMORYCHECK_COMMAND MATCHES ".*valgrind$") FOREACH(TEST ${${PROJECT_NAME}_PYTHON_MEMORYCHECK_TESTS}) ADD_PYTHON_MEMORYCHECK_UNIT_TEST("test-py-memory-${TEST}" "unittest/python/${TEST}.py" "bindings/python") ENDFOREACH() +ELSE() + MESSAGE(STATUS "Valgrind not found, memory checks are disabled") ENDIF() ADD_SUBDIRECTORY(pybind11)