Skip to content

Make MPPI Controller and Critics Apple Clang Friendly#5455

Merged
SteveMacenski merged 3 commits intoros-navigation:mainfrom
idesign0:main
Aug 14, 2025
Merged

Make MPPI Controller and Critics Apple Clang Friendly#5455
SteveMacenski merged 3 commits intoros-navigation:mainfrom
idesign0:main

Conversation

@idesign0
Copy link
Copy Markdown
Contributor

  • Added Apple Clang-specific handling in CMakeLists.txt to avoid -fconcepts flag, which is unsupported.
  • Enabled C++20 for Apple Clang to support Concepts natively.

Signed-off-by: Dhruv Patel <dhruvpatel2991998@gmail.com>
@codecov
Copy link
Copy Markdown

codecov bot commented Aug 13, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
see 4 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread nav2_mppi_controller/CMakeLists.txt Outdated
Signed-off-by: Dhruv Patel <dhruvpatel2991998@gmail.com>
@idesign0
Copy link
Copy Markdown
Contributor Author

idesign0 commented Aug 14, 2025

@SteveMacenski

with recent changes, Only mppi_critics target will require C++20. CMake will automatically add -std=c++20 (or the equivalent) just for this target, leaving all other targets at their current C++ standard. This is safer and avoids impacting the rest of the build. had to update cmake_minimum_required() version(3.8) for this to work. additionally, added Apple condition so changes only target macOS.

@idesign0 idesign0 requested a review from SteveMacenski August 14, 2025 12:05
Copy link
Copy Markdown
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.

Is there no way around this other than changing the cpp version? That's a kind of extreme move IMO

@@ -1,4 +1,4 @@
cmake_minimum_required(VERSION 3.5)
cmake_minimum_required(VERSION 3.8)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you search + replace in the CMakeLists to update them all to 3.8? Consistency is nice :-)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Can you search + replace in the CMakeLists to update them all to 3.8? Consistency is nice :-)

Sure 👍
i will add them in To-do. I’ll do the change when I switch to working with Rolling on macOS if its fine, since my current work and build checks are happening on Humble right now. (will begin soon).

@idesign0
Copy link
Copy Markdown
Contributor Author

Is there no way around this other than changing the cpp version? That's a kind of extreme move IMO

right now i couldnt find any. let me try for some more time if i can find a another way arround.

Copy link
Copy Markdown
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.

Can you update the 3.5 to 3.8 here? CI will pick up on rolling if there are any issues. I don't like to leave things in inconsistent states, though I appreciate your desire to test on Humble first :-)

@idesign0
Copy link
Copy Markdown
Contributor Author

Can you update the 3.5 to 3.8 here? CI will pick up on rolling if there are any issues. I don't like to leave things in inconsistent states, though I appreciate your desire to test on Humble first :-)

sure i will be on it.

Signed-off-by: Dhruv Patel <dhruvpatel2991998@gmail.com>
@idesign0 idesign0 requested a review from SteveMacenski August 14, 2025 17:50
@SteveMacenski SteveMacenski merged commit 1ecf27c into ros-navigation:main Aug 14, 2025
14 of 16 checks passed
bkoensgen pushed a commit to bkoensgen/navigation2 that referenced this pull request Aug 29, 2025
…#5455)

* fix: adjust C++ standard and compiler flags for Apple Clang

Signed-off-by: Dhruv Patel <dhruvpatel2991998@gmail.com>

* Update: Only mppi_critics target will require C++20

Signed-off-by: Dhruv Patel <dhruvpatel2991998@gmail.com>

* cmake_minimum_required(VERSION 3.8) update

Signed-off-by: Dhruv Patel <dhruvpatel2991998@gmail.com>

---------

Signed-off-by: Dhruv Patel <dhruvpatel2991998@gmail.com>
silanus23 pushed a commit to silanus23/navigation2 that referenced this pull request Sep 18, 2025
…#5455)

* fix: adjust C++ standard and compiler flags for Apple Clang

Signed-off-by: Dhruv Patel <dhruvpatel2991998@gmail.com>

* Update: Only mppi_critics target will require C++20

Signed-off-by: Dhruv Patel <dhruvpatel2991998@gmail.com>

* cmake_minimum_required(VERSION 3.8) update

Signed-off-by: Dhruv Patel <dhruvpatel2991998@gmail.com>

---------

Signed-off-by: Dhruv Patel <dhruvpatel2991998@gmail.com>
BCKSELFDRIVEWORLD pushed a commit to BCKSELFDRIVEWORLD/navigation2 that referenced this pull request Sep 23, 2025
…#5455)

* fix: adjust C++ standard and compiler flags for Apple Clang

Signed-off-by: Dhruv Patel <dhruvpatel2991998@gmail.com>

* Update: Only mppi_critics target will require C++20

Signed-off-by: Dhruv Patel <dhruvpatel2991998@gmail.com>

* cmake_minimum_required(VERSION 3.8) update

Signed-off-by: Dhruv Patel <dhruvpatel2991998@gmail.com>

---------

Signed-off-by: Dhruv Patel <dhruvpatel2991998@gmail.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