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

Fixes for compiling with clang on macOS (arm64) #3831

Merged
merged 34 commits into from
Sep 28, 2023

Conversation

srmainwaring
Copy link
Contributor


Basic Info

Info Please fill out this column
Ticket(s) this addresses #3828
Primary OS tested on MacOS (arm64 and intel)
Robotic platform tested on Gazebo / ArduPilot simulation of Iris copter

Description of contribution in a few bullet points

  • Changes fix compiler errors when building on macOS 13.4 with clang (version details below).
  • Broken out into changes per submodule / type of error to aid dropping changes that may not be desired (e.g. replacement of std::experimental::filesystem with std::filesystem).
  • Primary development and testing against ROS 2 Humble and the nav2 humble branch.
  • This PR rebased onto main and built against ROS Rolling (the port of ROS Rolling to macOS is ongoing, so cannot fully test all nav2 libraries on macOS until that is complete).
% clang --version
Apple clang version 14.0.3 (clang-1403.0.22.14.1)
Target: arm64-apple-darwin22.5.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

Description of documentation updates required from your changes

  • No functional changes.

Future work that may be required in bullet points

  • There are additional changes required to support macOS - these require more work to make them compatible with other platforms so have not been included at this stage.

For Maintainers:

  • Check that any new parameters added are updated in navigation.ros.org
  • Check that any significant change is added to the migration guide
  • Check that any new features OR changes to existing behaviors are reflected in the tuning guide
  • Check that any new functions have Doxygen added
  • Check that any new features have test coverage
  • Check that any new plugins is added to the plugins page
  • If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
@mergify
Copy link
Contributor

mergify bot commented Sep 22, 2023

@srmainwaring, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

@srmainwaring srmainwaring marked this pull request as draft September 22, 2023 13:38
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
…oolean

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
@SteveMacenski
Copy link
Member

The build failure looks like an API difference in Humble vs rolling. Its a trivial change, but one that would need to be made to compile

@srmainwaring
Copy link
Contributor Author

The build failure looks like an API difference in Humble vs rolling.

I'm working through these now and will force push the changes once I've got a clean build. A couple of libraries in Rolling on macOS are proving tricky. Will shift back from draft once it's clear.

@srmainwaring
Copy link
Contributor Author

What is the preferred treatment for unused parameters?

 float NodeLattice::getHeuristicCost(
   const Coordinates & node_coords,
   const Coordinates & goal_coords,
   [[maybe_unused]] const nav2_costmap_2d::Costmap2D * costmap)
 {

or

 float NodeLattice::getHeuristicCost(
   const Coordinates & node_coords,
   const Coordinates & goal_coords,
   const nav2_costmap_2d::Costmap2D * /*costmap*/)
 {

@SteveMacenski
Copy link
Member

SteveMacenski commented Sep 23, 2023

Second one. The C++98 in me will die kicking and screaming

@srmainwaring
Copy link
Contributor Author

There's one error in the tests - it looks like a code style conflict from a test I edited, but still trying to interpret the CI message.

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
@SteveMacenski
Copy link
Member

I see 8 failing messages and if you click on each in the "Tests" tab, it shows you exactly the failure and where it is (and in some cases, what to do to fix it explicitly)

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
@codecov
Copy link

codecov bot commented Sep 27, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (1efc96c) 64.15% compared to head (8ac6406) 90.37%.
Report is 3 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #3831       +/-   ##
===========================================
+ Coverage   64.15%   90.37%   +26.21%     
===========================================
  Files         413      413               
  Lines       18241    18239        -2     
===========================================
+ Hits        11703    16483     +4780     
+ Misses       6538     1756     -4782     
Files Coverage Δ
nav2_amcl/src/pf/pf.c 91.66% <ø> (+91.66%) ⬆️
nav2_amcl/src/pf/pf_vector.c 100.00% <100.00%> (+100.00%) ⬆️
...tree/include/nav2_behavior_tree/bt_action_node.hpp 84.87% <100.00%> (+15.12%) ⬆️
...clude/nav2_behavior_tree/bt_cancel_action_node.hpp 80.00% <100.00%> (ø)
...vior_tree/plugins/condition/is_stuck_condition.cpp 98.11% <100.00%> (+3.77%) ⬆️
...nclude/nav2_behaviors/plugins/drive_on_heading.hpp 92.64% <100.00%> (+92.64%) ⬆️
.../nav2_bt_navigator/navigators/navigate_to_pose.hpp 100.00% <100.00%> (+100.00%) ⬆️
nav2_controller/src/controller_server.cpp 89.19% <100.00%> (+76.45%) ⬆️
...clude/nav2_costmap_2d/denoise/image_processing.hpp 98.06% <100.00%> (+<0.01%) ⬆️
nav2_costmap_2d/src/clear_costmap_service.cpp 42.00% <ø> (+12.00%) ⬆️
... and 6 more

... and 185 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@srmainwaring srmainwaring marked this pull request as ready for review September 27, 2023 13:27
Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

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

Overall LGTM!

nav2_system_tests/src/planning/CMakeLists.txt Outdated Show resolved Hide resolved
nav2_smoother/test/test_savitzky_golay_smoother.cpp Outdated Show resolved Hide resolved
nav2_mppi_controller/test/utils_test.cpp Outdated Show resolved Hide resolved
nav2_amcl/src/pf/pf_vector.c Show resolved Hide resolved
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
…onstructor

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
@srmainwaring
Copy link
Contributor Author

srmainwaring commented Sep 28, 2023

This is definitely a vector https://navigation.ros.org/configuration/packages/collision_monitor/configuring-collision-monitor-node.html

Yes, and on inspection from the line below my change I can see the data is a vector as well. This was the clang error:

[ 97%] Building CXX object test/CMakeFiles/collision_monitor_node_test.dir/collision_monitor_node_test.cpp.o
/Users/rhys/Code/ros2/rolling/ros2-nav/src/navigation2/nav2_collision_monitor/test/collision_monitor_node_test.cpp:1026:61: error: braces around scalar initializer [-Werror,-Wbraced-scalar-init]
  cm_->declare_parameter("polygons", rclcpp::ParameterValue({"Stop",}));
                                                            ^~~~~~~~~
1 error generated.

Not sure why it's not resolving the list to a std::vector<std::string> and calling the matching explicit constructor for rclcpp::ParameterValue. The correction in 8ac6406 forces the desired ctor.

@srmainwaring
Copy link
Contributor Author

btw - to build on macOS (and fully close #3828) there is a change needed to the nav2_mppi_controller CMakeLists.txt to disable some of the compiler settings (AVX2, FMA, etc). I haven't included those here and will post in a follow up PR.

@SteveMacenski SteveMacenski merged commit 42670c0 into ros-navigation:main Sep 28, 2023
7 checks passed
@srmainwaring srmainwaring deleted the prs/pr-main-clang branch September 28, 2023 21:04
enricosutera pushed a commit to enricosutera/navigation2 that referenced this pull request May 19, 2024
* amcl: declare void parameter for functions with no args

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* amcl: remove unused variables

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* behavior_tree: address clang compilation issues

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* behaviors: add missing override specifier

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* bt_navigator: add missing override specifier

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* collision_monitor: address clang compilation issues

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* collision_monitor: remove unused variables

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* constrained_smoother: address clang compilation issues

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* controller: address clang compilation issues

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* costmap_2d: add missing override specifier

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* costmap_2d: address clang compilation issues

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* costmap_2d: fix link issue for order_layer

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* dwb_controller: fix clang compile issue, replace ulong with uint32_t

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* map_server: replace std::experimental::filesystem

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* map_server: remove dependency on stdc++fs

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* waypoint_follower: address clang compilation issues

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* waypoint_follower: remove dependency on stdc++fs

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* waypoint_follower: replace std::experimental::filesystem

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* smoother: address clang compilation issues

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* smoother: remove unused variables

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* system_tests: remove dependency on stdc++fs

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* rotation_shim_controller: update percentage arg in setSpeedLimit to boolean

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* planner: remove unused variables

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* costmap_2d: address clang compilation issues

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* mppi_controller: replace use of auto as function param with templates

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* mppi_controller: address clang compilation issues

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* costmap_2d: resolve clang issue with std::pair non-const copy

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* smac_planner: suppress unused parameter warnings

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* behavior_tree: fix code style

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* costmap_2d: fix code style

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* mppi_controller: revert reindexing of for-loop

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* smoother: remove commented code

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* system_tests: remove commented target link library

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* collision_monitor: revert parameter type to vector and use explicit constructor

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

---------

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: enricosutera <enricosutera@outlook.com>
Marc-Morcos pushed a commit to Marc-Morcos/navigation2 that referenced this pull request Jul 4, 2024
* amcl: declare void parameter for functions with no args

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* amcl: remove unused variables

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* behavior_tree: address clang compilation issues

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* behaviors: add missing override specifier

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* bt_navigator: add missing override specifier

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* collision_monitor: address clang compilation issues

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* collision_monitor: remove unused variables

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* constrained_smoother: address clang compilation issues

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* controller: address clang compilation issues

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* costmap_2d: add missing override specifier

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* costmap_2d: address clang compilation issues

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* costmap_2d: fix link issue for order_layer

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* dwb_controller: fix clang compile issue, replace ulong with uint32_t

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* map_server: replace std::experimental::filesystem

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* map_server: remove dependency on stdc++fs

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* waypoint_follower: address clang compilation issues

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* waypoint_follower: remove dependency on stdc++fs

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* waypoint_follower: replace std::experimental::filesystem

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* smoother: address clang compilation issues

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* smoother: remove unused variables

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* system_tests: remove dependency on stdc++fs

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* rotation_shim_controller: update percentage arg in setSpeedLimit to boolean

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* planner: remove unused variables

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* costmap_2d: address clang compilation issues

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* mppi_controller: replace use of auto as function param with templates

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* mppi_controller: address clang compilation issues

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* costmap_2d: resolve clang issue with std::pair non-const copy

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* smac_planner: suppress unused parameter warnings

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* behavior_tree: fix code style

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* costmap_2d: fix code style

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* mppi_controller: revert reindexing of for-loop

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* smoother: remove commented code

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* system_tests: remove commented target link library

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

* collision_monitor: revert parameter type to vector and use explicit constructor

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>

---------

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
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.

2 participants