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

Fix ament_target_dependencies accepting non-packages #180

Merged
merged 1 commit into from Jul 25, 2019

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Jul 19, 2019

This quotes a cmake variable to avoid some weird behavior that causes a check to always evaluate to false, even for non-packages.

If package_name has been found then if(NOT ${${package_name}_FOUND}) becomes if(NOT 1), and the condition is false as intended.

If package_name is not a package name then ${package_name}_FOUND isn't a variable, so ${${package_name}_FOUND} is an empty string. Since the string is unquoted it gets expanded into zero arguments, and the actual condition becomes if(NOT) which evaluates to false too.

Thanks to @aaronchongth for finding the cmake issue: https://gitlab.kitware.com/cmake/cmake/issues/19379

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@ivanpauno
Copy link
Contributor

and the actual condition becomes if(NOT) which evaluates to false too.

That's fun 🤣.

@sloretz
Copy link
Contributor Author

sloretz commented Jul 19, 2019

CI (build all, test ament_cmake_target_dependencies)

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

@sloretz
Copy link
Contributor Author

sloretz commented Jul 24, 2019

CI for this and referenced PRs

  • Building all
  • Testing:
    • ament_cmake_target_dependencies
    • examples_rclcpp_minimal_composition
    • rclcpp_components
    • rttest rosidl_generator_c
    • rosidl_generator_py
    • rosidl_typesupport_connext_c
    • rosidl_typesupport_fastrtps_c
    • rviz_rendering_tests
  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@sloretz
Copy link
Contributor Author

sloretz commented Jul 25, 2019

CI again with feedback and linter fixes

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

Edit: CI looks ok, just

cmake/rclcpp_components_register_node.cmake:33: Line ends in whitespace [whitespace/eol]

Which appears to be caused by ros2/rclcpp#784

@sloretz sloretz merged commit 99d316a into master Jul 25, 2019
@delete-merged-branch delete-merged-branch bot deleted the sloretz/fix-not-packags-accepted branch July 25, 2019 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants