Conversation
|
I think I missed something here. Isnt the grouping exactly the same as in http://ci.ros2.org/job/ci_linux/3515/testReport/ ? |
The old test report lists the tests with |
Oh yeah my bad, I was expecting all of them to be under a single rclpy folder and I just compared the number of folders and not the names |
mikaelarguedas
left a comment
There was a problem hiding this comment.
Changes look good to me, I think it's worth waiting a Windows CI to complete before merging.
Few question/nitpicks below
| WORKING_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}" | ||
| PYTHON_EXECUTABLE "${PYTHON_EXECUTABLE}" | ||
| APPEND_ENV AMENT_PREFIX_PATH=${ament_index_build_path} | ||
| PYTHONPATH=${CMAKE_CURRENT_BINARY_DIR} |
There was a problem hiding this comment.
PYTHONPATH is a value to the multi-value argument APPEND_ENV. Therefore I used the extra one level indentation. If anyone would like to change this please feel free to do so.
| LIBRARY_OUTPUT_DIRECTORY${_build_type} "${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}" | ||
| RUNTIME_OUTPUT_DIRECTORY${_build_type} "${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}" | ||
| LIBRARY_OUTPUT_DIRECTORY${_build_type} "${CMAKE_CURRENT_BINARY_DIR}/test_${PROJECT_NAME}" | ||
| RUNTIME_OUTPUT_DIRECTORY${_build_type} "${CMAKE_CURRENT_BINARY_DIR}/test_${PROJECT_NAME}" |
There was a problem hiding this comment.
It looks a bit weird to me that if I build without build_testing all my rclpy libraries go to this test_rclpy folder. Then they'll be install at the right place at the end of the day so I guess it's fine
There was a problem hiding this comment.
The directory can have any name except rclpy. I used the test_ prefix since it is being used during testing. But any other name (which isn't likely to collide with other packages) would be fine.
|
|
||
| def create_node(node_name, *, namespace=None): | ||
| # imported locally to avoid loading extensions on module import | ||
| from rclpy.node import Node |
There was a problem hiding this comment.
Is delaying the import a necessary change with the new approach, or an additional feature?
There was a problem hiding this comment.
It is necessary since the node module already imports the singleton module. But we need to delay the instantiation of the singletons until we had a chance to override the _import functions.
|
This seems to have a warning on windows. But Jenkins displays this job weirdly and when you click on the warning it brings you to a page saying 0 warnings o_O |
|
A retriggered Windows build looks fine: |
DRAFT: Native service client
Follow up of ament/ament_cmake#116 (comment)
See http://ci.ros2.org/job/ci_linux/3522/testReport/ with the "nicer" grouping.
Ready for review.