-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Revamp nav2_behavior_tree CMakeLists.txt to use modern idioms. #4485
Revamp nav2_behavior_tree CMakeLists.txt to use modern idioms. #4485
Conversation
@clalancette, your PR has failed to build. Please check CI outputs and resolve issues. |
70c389f
to
48b1b70
Compare
@clalancette, your PR has failed to build. Please check CI outputs and resolve issues. |
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. |
Are these changes backportable to Jazzy or only rolling? |
They are backportable to Jazzy, assuming that BehaviorTree/BehaviorTree.CPP#826 is released into Jazzy (it has not yet been, as far as I can tell). |
This pull request is in conflict. Could you fix it @clalancette? |
@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? |
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.
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 |
48b1b70
to
c285b10
Compare
Got it! Your help is much appreciated 🫡
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.
Must be - Iron is pretty close or essentially the same as |
@clalancette, your PR has failed to build. Please check CI outputs and resolve issues. |
As a not very scientific number, locally I went from a compile time for all of navigation2 of 10 minutes 32 seconds on |
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. |
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 :-) |
This pull request is in conflict. Could you fix it @clalancette? |
Fixed the merge conflict 👍 |
@clalancette, your PR has failed to build. Please check CI outputs and resolve issues. |
This pull request is in conflict. Could you fix it @clalancette? |
f296ef5
to
abf323a
Compare
@clalancette, your PR has failed to build. Please check CI outputs and resolve issues. |
This commit does a number of things: 1. 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. 2. Moves the include directory down one level to include/${PROJECT_NAME}, which is best practice in ROS 2 since Humble. 3. Makes sure to export nav2_behavior_tree as a CMake target, so downstream users of it can use that target. 4. Removes the use of boost. To do this, we had to introduce our own version of the "split_string" method. 5. 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. Signed-off-by: Chris Lalancette <[email protected]>
abf323a
to
7bd5722
Compare
All right, so this now builds against the latest Rolling. Unfortunately, it looks like the |
Its failing in our CI too for the last ~week or so, I'm not sure why that popped up out of no where, but that's not related to this PR. We can merge this now I think and no worse for ware. Are you OK with that? |
Yep, that sounds good to me. Thanks! |
Basic Info
Description of contribution in a few bullet points
This commit does a number of things:
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: