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

Do not associate test library with export #61

Merged
merged 2 commits into from
Nov 8, 2021

Conversation

jacobperron
Copy link
Member

Otherwise, downstream targets will try to link against it.
This can cause issues linking because the test component library is not exporting dependencies (e.g. test_msgs).

Otherwise, downstream targets will try to link against it.
This can cause issues linking because the test component library is not exporting dependencies (e.g. test_msgs).

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron jacobperron added the bug Something isn't working label Nov 5, 2021
@jacobperron
Copy link
Member Author

hmm, looks like this patch doesn't work. I guess it only appeared to work locally because I forgot to clean my build folder.

Seems like there's just not a convenient way to write test components without installing them (probably something we could improve in rclcpp_components). For example, rclcpp_components works around the issue by not calling rclcpp_components_register_node and mocking an ament index:

https://github.com/ros2/rclcpp/blob/e0e96681d9f0f5bd1f220b3d3107a612dad8321b/rclcpp_components/CMakeLists.txt#L83-L111

🙁

This helps avoid linking issues downstream. Long term, it would probably be best to avoid installing
the library entirely, but writing tests becomes more complex.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron jacobperron changed the title Do not install test component library Do not associate test library with export Nov 5, 2021
@jacobperron
Copy link
Member Author

Removing the association with the export fixes the issue for me (c86d28e).

@@ -158,7 +158,6 @@ if(BUILD_TESTING)
EXECUTABLE test_component)

install(TARGETS ${PROJECT_NAME}_test_component_lib
EXPORT export_${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.

I don't know why this was originally exported, but it could also be exported as export_${PROJECT_NAME}_test or something to prevent accidental linking against? Although to be fair, there isn't a reason to install (or export) the test library.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, installing or exporting the library sounds like a bad idea.
The workaround that rclcpp_components is using looks better, though this seems fine to solve the CI issues.

@jacobperron
Copy link
Member Author

I'm not sure why the GitHub workflow is suddenly using Python 2.7...
With Rpr passing, I'm going to merge this. I'll play around with the GitHub workflow separately.

@jacobperron jacobperron merged commit 348fd60 into main Nov 8, 2021
@delete-merged-branch delete-merged-branch bot deleted the jacob/uninstall_test_component branch November 8, 2021 18:53
@jacobperron
Copy link
Member Author

IIUC, the GitHub workflow requires some changes to rosidl_parser to become available. It should hopefully resolve itself once a Rolling sync to testing happens: https://build.ros2.org/job/Rrel_sync-packages-to-testing_focal_amd64/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants