Fixes for compiling with clang on macOS (arm64) by srmainwaring · Pull Request #3831 · ros-navigation/navigation2

srmainwaring · 2023-09-22T12:40:16Z

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 <[email protected]>

mergify · 2023-09-22T13:13:47Z

@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).

Signed-off-by: Rhys Mainwaring <[email protected]>

…oolean Signed-off-by: Rhys Mainwaring <[email protected]>

Signed-off-by: Rhys Mainwaring <[email protected]>

SteveMacenski · 2023-09-22T15:07:51Z

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 · 2023-09-22T15:11:29Z

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 · 2023-09-22T15:57:21Z

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 · 2023-09-23T04:37:01Z

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

srmainwaring · 2023-09-23T10:41:33Z

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 <[email protected]>

SteveMacenski · 2023-09-25T16:32:44Z

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 <[email protected]>

codecov · 2023-09-27T13:26:14Z

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.

SteveMacenski

Overall LGTM!

Signed-off-by: Rhys Mainwaring <[email protected]>

…onstructor Signed-off-by: Rhys Mainwaring <[email protected]>

srmainwaring · 2023-09-28T09:24:24Z

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 · 2023-09-28T09:36:34Z

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.

* amcl: declare void parameter for functions with no args Signed-off-by: Rhys Mainwaring <[email protected]> * amcl: remove unused variables Signed-off-by: Rhys Mainwaring <[email protected]> * behavior_tree: address clang compilation issues Signed-off-by: Rhys Mainwaring <[email protected]> * behaviors: add missing override specifier Signed-off-by: Rhys Mainwaring <[email protected]> * bt_navigator: add missing override specifier Signed-off-by: Rhys Mainwaring <[email protected]> * collision_monitor: address clang compilation issues Signed-off-by: Rhys Mainwaring <[email protected]> * collision_monitor: remove unused variables Signed-off-by: Rhys Mainwaring <[email protected]> * constrained_smoother: address clang compilation issues Signed-off-by: Rhys Mainwaring <[email protected]> * controller: address clang compilation issues Signed-off-by: Rhys Mainwaring <[email protected]> * costmap_2d: add missing override specifier Signed-off-by: Rhys Mainwaring <[email protected]> * costmap_2d: address clang compilation issues Signed-off-by: Rhys Mainwaring <[email protected]> * costmap_2d: fix link issue for order_layer Signed-off-by: Rhys Mainwaring <[email protected]> * dwb_controller: fix clang compile issue, replace ulong with uint32_t Signed-off-by: Rhys Mainwaring <[email protected]> * map_server: replace std::experimental::filesystem Signed-off-by: Rhys Mainwaring <[email protected]> * map_server: remove dependency on stdc++fs Signed-off-by: Rhys Mainwaring <[email protected]> * waypoint_follower: address clang compilation issues Signed-off-by: Rhys Mainwaring <[email protected]> * waypoint_follower: remove dependency on stdc++fs Signed-off-by: Rhys Mainwaring <[email protected]> * waypoint_follower: replace std::experimental::filesystem Signed-off-by: Rhys Mainwaring <[email protected]> * smoother: address clang compilation issues Signed-off-by: Rhys Mainwaring <[email protected]> * smoother: remove unused variables Signed-off-by: Rhys Mainwaring <[email protected]> * system_tests: remove dependency on stdc++fs Signed-off-by: Rhys Mainwaring <[email protected]> * rotation_shim_controller: update percentage arg in setSpeedLimit to boolean Signed-off-by: Rhys Mainwaring <[email protected]> * planner: remove unused variables Signed-off-by: Rhys Mainwaring <[email protected]> * costmap_2d: address clang compilation issues Signed-off-by: Rhys Mainwaring <[email protected]> * mppi_controller: replace use of auto as function param with templates Signed-off-by: Rhys Mainwaring <[email protected]> * mppi_controller: address clang compilation issues Signed-off-by: Rhys Mainwaring <[email protected]> * costmap_2d: resolve clang issue with std::pair non-const copy Signed-off-by: Rhys Mainwaring <[email protected]> * smac_planner: suppress unused parameter warnings Signed-off-by: Rhys Mainwaring <[email protected]> * behavior_tree: fix code style Signed-off-by: Rhys Mainwaring <[email protected]> * costmap_2d: fix code style Signed-off-by: Rhys Mainwaring <[email protected]> * mppi_controller: revert reindexing of for-loop Signed-off-by: Rhys Mainwaring <[email protected]> * smoother: remove commented code Signed-off-by: Rhys Mainwaring <[email protected]> * system_tests: remove commented target link library Signed-off-by: Rhys Mainwaring <[email protected]> * collision_monitor: revert parameter type to vector and use explicit constructor Signed-off-by: Rhys Mainwaring <[email protected]> --------- Signed-off-by: Rhys Mainwaring <[email protected]> Signed-off-by: enricosutera <[email protected]>

* amcl: declare void parameter for functions with no args Signed-off-by: Rhys Mainwaring <[email protected]> * amcl: remove unused variables Signed-off-by: Rhys Mainwaring <[email protected]> * behavior_tree: address clang compilation issues Signed-off-by: Rhys Mainwaring <[email protected]> * behaviors: add missing override specifier Signed-off-by: Rhys Mainwaring <[email protected]> * bt_navigator: add missing override specifier Signed-off-by: Rhys Mainwaring <[email protected]> * collision_monitor: address clang compilation issues Signed-off-by: Rhys Mainwaring <[email protected]> * collision_monitor: remove unused variables Signed-off-by: Rhys Mainwaring <[email protected]> * constrained_smoother: address clang compilation issues Signed-off-by: Rhys Mainwaring <[email protected]> * controller: address clang compilation issues Signed-off-by: Rhys Mainwaring <[email protected]> * costmap_2d: add missing override specifier Signed-off-by: Rhys Mainwaring <[email protected]> * costmap_2d: address clang compilation issues Signed-off-by: Rhys Mainwaring <[email protected]> * costmap_2d: fix link issue for order_layer Signed-off-by: Rhys Mainwaring <[email protected]> * dwb_controller: fix clang compile issue, replace ulong with uint32_t Signed-off-by: Rhys Mainwaring <[email protected]> * map_server: replace std::experimental::filesystem Signed-off-by: Rhys Mainwaring <[email protected]> * map_server: remove dependency on stdc++fs Signed-off-by: Rhys Mainwaring <[email protected]> * waypoint_follower: address clang compilation issues Signed-off-by: Rhys Mainwaring <[email protected]> * waypoint_follower: remove dependency on stdc++fs Signed-off-by: Rhys Mainwaring <[email protected]> * waypoint_follower: replace std::experimental::filesystem Signed-off-by: Rhys Mainwaring <[email protected]> * smoother: address clang compilation issues Signed-off-by: Rhys Mainwaring <[email protected]> * smoother: remove unused variables Signed-off-by: Rhys Mainwaring <[email protected]> * system_tests: remove dependency on stdc++fs Signed-off-by: Rhys Mainwaring <[email protected]> * rotation_shim_controller: update percentage arg in setSpeedLimit to boolean Signed-off-by: Rhys Mainwaring <[email protected]> * planner: remove unused variables Signed-off-by: Rhys Mainwaring <[email protected]> * costmap_2d: address clang compilation issues Signed-off-by: Rhys Mainwaring <[email protected]> * mppi_controller: replace use of auto as function param with templates Signed-off-by: Rhys Mainwaring <[email protected]> * mppi_controller: address clang compilation issues Signed-off-by: Rhys Mainwaring <[email protected]> * costmap_2d: resolve clang issue with std::pair non-const copy Signed-off-by: Rhys Mainwaring <[email protected]> * smac_planner: suppress unused parameter warnings Signed-off-by: Rhys Mainwaring <[email protected]> * behavior_tree: fix code style Signed-off-by: Rhys Mainwaring <[email protected]> * costmap_2d: fix code style Signed-off-by: Rhys Mainwaring <[email protected]> * mppi_controller: revert reindexing of for-loop Signed-off-by: Rhys Mainwaring <[email protected]> * smoother: remove commented code Signed-off-by: Rhys Mainwaring <[email protected]> * system_tests: remove commented target link library Signed-off-by: Rhys Mainwaring <[email protected]> * collision_monitor: revert parameter type to vector and use explicit constructor Signed-off-by: Rhys Mainwaring <[email protected]> --------- Signed-off-by: Rhys Mainwaring <[email protected]>

srmainwaring added 3 commits September 22, 2023 12:32

amcl: declare void parameter for functions with no args

4a1c9e7

Signed-off-by: Rhys Mainwaring <[email protected]>

amcl: remove unused variables

15d0af1

Signed-off-by: Rhys Mainwaring <[email protected]>

behavior_tree: address clang compilation issues

9585c05

Signed-off-by: Rhys Mainwaring <[email protected]>

srmainwaring marked this pull request as draft September 22, 2023 13:38

srmainwaring added 24 commits September 22, 2023 15:46

behaviors: add missing override specifier

159441d

Signed-off-by: Rhys Mainwaring <[email protected]>

bt_navigator: add missing override specifier

8b9e770

Signed-off-by: Rhys Mainwaring <[email protected]>

collision_monitor: address clang compilation issues

32e5b99

Signed-off-by: Rhys Mainwaring <[email protected]>

collision_monitor: remove unused variables

12e86c2

Signed-off-by: Rhys Mainwaring <[email protected]>

constrained_smoother: address clang compilation issues

fe36e8c

Signed-off-by: Rhys Mainwaring <[email protected]>

controller: address clang compilation issues

921c813

Signed-off-by: Rhys Mainwaring <[email protected]>

costmap_2d: add missing override specifier

186e552

Signed-off-by: Rhys Mainwaring <[email protected]>

costmap_2d: address clang compilation issues

6c88f53

Signed-off-by: Rhys Mainwaring <[email protected]>

costmap_2d: fix link issue for order_layer

0a970c1

Signed-off-by: Rhys Mainwaring <[email protected]>

dwb_controller: fix clang compile issue, replace ulong with uint32_t

2cce5fb

Signed-off-by: Rhys Mainwaring <[email protected]>

map_server: replace std::experimental::filesystem

34c8576

Signed-off-by: Rhys Mainwaring <[email protected]>

map_server: remove dependency on stdc++fs

f1c2a5f

Signed-off-by: Rhys Mainwaring <[email protected]>

waypoint_follower: address clang compilation issues

4d64a04

Signed-off-by: Rhys Mainwaring <[email protected]>

waypoint_follower: remove dependency on stdc++fs

9f1ef97

Signed-off-by: Rhys Mainwaring <[email protected]>

waypoint_follower: replace std::experimental::filesystem

fd26b67

Signed-off-by: Rhys Mainwaring <[email protected]>

smoother: address clang compilation issues

554ed32

Signed-off-by: Rhys Mainwaring <[email protected]>

smoother: remove unused variables

5ba61c3

Signed-off-by: Rhys Mainwaring <[email protected]>

system_tests: remove dependency on stdc++fs

71cbc3a

Signed-off-by: Rhys Mainwaring <[email protected]>

rotation_shim_controller: update percentage arg in setSpeedLimit to b…

22da9c3

…oolean Signed-off-by: Rhys Mainwaring <[email protected]>

planner: remove unused variables

897e2e0

Signed-off-by: Rhys Mainwaring <[email protected]>

costmap_2d: address clang compilation issues

e81c6a6

Signed-off-by: Rhys Mainwaring <[email protected]>

mppi_controller: replace use of auto as function param with templates

8eeb47c

Signed-off-by: Rhys Mainwaring <[email protected]>

mppi_controller: address clang compilation issues

c3e485b

Signed-off-by: Rhys Mainwaring <[email protected]>

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

1ec29db

Signed-off-by: Rhys Mainwaring <[email protected]>

SteveMacenski reviewed Sep 22, 2023

View reviewed changes

Comment thread nav2_collision_monitor/test/collision_monitor_node_test.cpp Outdated

Comment thread nav2_amcl/src/pf/pf.c

srmainwaring force-pushed the prs/pr-main-clang branch from 2ed8fd9 to 1ec29db Compare September 22, 2023 15:44

smac_planner: suppress unused parameter warnings

f5c479f

Signed-off-by: Rhys Mainwaring <[email protected]>

srmainwaring added 2 commits September 27, 2023 13:21

behavior_tree: fix code style

16dabba

Signed-off-by: Rhys Mainwaring <[email protected]>

costmap_2d: fix code style

3df21f7

Signed-off-by: Rhys Mainwaring <[email protected]>

srmainwaring marked this pull request as ready for review September 27, 2023 13:27

srmainwaring requested a review from SteveMacenski September 27, 2023 13:27

SteveMacenski requested changes Sep 27, 2023

View reviewed changes

srmainwaring added 4 commits September 28, 2023 09:35

mppi_controller: revert reindexing of for-loop

d7a107e

Signed-off-by: Rhys Mainwaring <[email protected]>

smoother: remove commented code

5821f39

Signed-off-by: Rhys Mainwaring <[email protected]>

system_tests: remove commented target link library

6adaf53

Signed-off-by: Rhys Mainwaring <[email protected]>

collision_monitor: revert parameter type to vector and use explicit c…

8ac6406

…onstructor Signed-off-by: Rhys Mainwaring <[email protected]>

srmainwaring requested a review from SteveMacenski September 28, 2023 10:11

SteveMacenski approved these changes Sep 28, 2023

View reviewed changes

SteveMacenski merged commit 42670c0 into ros-navigation:main Sep 28, 2023

srmainwaring deleted the prs/pr-main-clang branch September 28, 2023 21:04

srmainwaring mentioned this pull request Jan 15, 2024

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

Merged

Conversation

srmainwaring commented Sep 22, 2023

Basic Info

Description of contribution in a few bullet points

Description of documentation updates required from your changes

Future work that may be required in bullet points

For Maintainers:

Uh oh!

mergify Bot commented Sep 22, 2023

Uh oh!

SteveMacenski commented Sep 22, 2023

Uh oh!

Uh oh!

Uh oh!

srmainwaring commented Sep 22, 2023

Uh oh!

srmainwaring commented Sep 22, 2023

Uh oh!

SteveMacenski commented Sep 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srmainwaring commented Sep 23, 2023

Uh oh!

SteveMacenski commented Sep 25, 2023

Uh oh!

codecov Bot commented Sep 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

SteveMacenski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

srmainwaring commented Sep 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srmainwaring commented Sep 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SteveMacenski commented Sep 23, 2023 •

edited

Loading

codecov Bot commented Sep 27, 2023 •

edited

Loading

srmainwaring commented Sep 28, 2023 •

edited

Loading