Revamp nav2_behavior_tree CMakeLists.txt to use modern idioms.

clalancette commented 1 week ago

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	Follow-up to #4357
Primary OS tested on	Ubuntu 24.04
Robotic platform tested on	N/A
Does this PR contain AI generated software?	No

Description of contribution in a few bullet points

This commit does a number of things:

Switches to using target_link_libraries everywhere. This gives us finer-grained control over what dependencies are exported to downstream as public, or private. In the particular case of nav2_behavior_tree, this actually doesn't matter too much, but it will help for other packages.
Moves the include directory down one level to include/${PROJECT_NAME}, which is best practice in ROS 2 since Humble.
Makes sure to export nav2_behavior_tree as a CMake target, so downstream users of it can use that target.
Removes the use of boost. To do this, we had to introduce our own version of the "split_string" method.
Moves the test_action_server.hpp file into the main include directory. This is slightly odd, but because downstream packages (opennav_docking_bt) depend on this header file to compile their own tests, it is technically part of the public interface.

Description of documentation updates required from your changes

None needed.

Future work that may be required in bullet points

The rest of the packages in this repository need a similar treatment (PRs will be upcoming).

For Maintainers:

[ ] Check that any new parameters added are updated in docs.nav2.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

mergify[bot] commented 1 week ago

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

mergify[bot] commented 1 week ago

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

clalancette commented 1 week ago

This one needs a release of behaviortree.cpp to be done so we can get the exported target there. I'll contact the maintainers tomorrow and see if we can get a release of that.

SteveMacenski commented 1 week ago

Are these changes backportable to Jazzy or only rolling?

clalancette commented 1 week ago

Are these changes backportable to Jazzy or only rolling?

They are backportable to Jazzy, assuming that https://github.com/BehaviorTree/BehaviorTree.CPP/pull/826 is released into Jazzy (it has not yet been, as far as I can tell).

mergify[bot] commented 1 week ago

This pull request is in conflict. Could you fix it @clalancette?

SteveMacenski commented 1 week ago

@clalancette thanks for all this work, especially this package + Smac Planner + MPPI + System Tests are the largest time hogs so even this alone probably cuts off minutes. It would be great if during these PRs you gave a ballpark of time saved for that package and/or stack. Nothing formal, just something to acknowledge your efforts for.

I was curious to know what capacity you're working on this. Is this something of your own volition to help me / the community / yourself if doing stuff on the rare occasion that Nav2 needs assistance -- or is this something Intrinsic needs bc you're looking at starting to use it?

clalancette commented 1 week ago

Is this something of your own volition to help me / the community / yourself if doing stuff on the rare occasion that Nav2 needs assistance

It is me doing it on my own volition. I've been using Navigation2 as a test bed for rmw_zenoh work, and I noticed that the compile times were high, so I decided to look into it.

It would be great if during these PRs you gave a ballpark of time saved for that package and/or stack. Nothing formal, just something to acknowledge your efforts for.

I can give some numbers as I open PRs, but this is unfortunately a case where we won't see a lot of savings until we get the bulk of it done. That is, most of savings are going to be when the downstream packages can rely on the exported targets, rather than the LIBRARIES/INCLUDE_DIRS.

One other thing I'll mention is that I started this journey on the iron branch, and there, this series made a huge difference (as stated in https://github.com/ros-navigation/navigation2/pull/4357, I went from 22 minutes to 9 minutes locally). As I've been porting to the main branch, I've been seeing less of a difference. I'm not sure if that is because main is just better to start with, or if it's because I haven't yet ported everything over yet.

SteveMacenski commented 1 week ago

Got it! Your help is much appreciated :saluting_face:

as a test bed for rmw_zenoh work

I had some threads with Angelo on this but haven't heard back.. I need to ping them. I wanted to add into our build matrix to run Zenoh once the binaries are available like we have to run Fast/Cyclone on our nightlies. That'll show issues if they exist. Even now, Fast and Cyclone fail on different things.

One other thing I'll mention is that I started this journey on the iron branch, and there, this series made a huge difference (as stated in https://github.com/ros-navigation/navigation2/pull/4357, I went from 22 minutes to 9 minutes locally). As I've been porting to the main branch, I've been seeing less of a difference. I'm not sure if that is because main is just better to start with, or if it's because I haven't yet ported everything over yet.

Must be - Iron is pretty close or essentially the same as main. Probably just not ported yet!

mergify[bot] commented 1 week ago

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

clalancette commented 6 days ago

It would be great if during these PRs you gave a ballpark of time saved for that package and/or stack. Nothing formal, just something to acknowledge your efforts for.

I can give some numbers as I open PRs

As a not very scientific number, locally I went from a compile time for all of navigation2 of 10 minutes 32 seconds on main to 10 minutes 19 seconds with this PR.

SteveMacenski commented 6 days ago

I think just waiting on the BT stuff to sync (and perhaps https://github.com/ros-navigation/navigation2/pull/4485#discussion_r1655183962?)? Otherwise, let me know when we can retrigger CI for this to go on!

clalancette commented 6 days ago

I think just waiting on the BT stuff to sync (and perhaps #4485 (comment)?)? Otherwise, let me know when we can retrigger CI for this to go on!

Yep, exactly. Once those are synced out, we can rerun CI on this and it should pass.

SteveMacenski commented 6 days ago

OK, if there's others you can open while we wait, happy to have a few of these open in parallel. Whatever works for you :-)

mergify[bot] commented 5 days ago

This pull request is in conflict. Could you fix it @clalancette?

SteveMacenski commented 5 days ago

Fixed the merge conflict :+1:

mergify[bot] commented 5 days ago

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

ros-navigation / navigation2