Skip to content
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.

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

avoid custom working directory #147

Merged
merged 1 commit into from
Nov 15, 2017

Conversation

dirk-thomas
Copy link
Member

@dirk-thomas dirk-thomas commented Nov 14, 2017

Follow up of ament/ament_cmake#116 (comment)

See http://ci.ros2.org/job/ci_linux/3522/testReport/ with the "nicer" grouping.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Ready for review.

@dirk-thomas dirk-thomas added the in review Waiting for review (Kanban column) label Nov 14, 2017
@dirk-thomas dirk-thomas self-assigned this Nov 14, 2017
@mikaelarguedas
Copy link
Member

I think I missed something here. Isnt the grouping exactly the same as in http://ci.ros2.org/job/ci_linux/3515/testReport/ ?
On the bright side I can now access the test results for all these tests so that's the most important improvement 👍

@dirk-thomas
Copy link
Member Author

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 rclpy.src.ros2.rclpy.rclpy.test* while the new one lists them as rclpy.test*.

@mikaelarguedas
Copy link
Member

The old test report lists the tests with rclpy.src.ros2.rclpy.rclpy.test* while the new one lists them as rclpy.test*.

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

Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

Changes look good to me, I think it's worth waiting a Windows CI to complete before merging.
Few question/nitpicks below

PYTHON_EXECUTABLE "${PYTHON_EXECUTABLE}"
APPEND_ENV AMENT_PREFIX_PATH=${ament_index_build_path}
PYTHONPATH=${CMAKE_CURRENT_BINARY_DIR}
Copy link
Member

Choose a reason for hiding this comment

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

weird indent ?

Copy link
Member Author

Choose a reason for hiding this comment

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

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}"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Is delaying the import a necessary change with the new approach, or an additional feature?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@mikaelarguedas
Copy link
Member

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

@dirk-thomas
Copy link
Member Author

A retriggered Windows build looks fine: Build Status

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants