search

ros-navigation / navigation2

ROS 2 Navigation Framework and System
https://nav2.org/
Other
2.34k stars 1.21k forks source link

Revamp CMake to make compilation significantly faster #4357

Open clalancette opened 1 month ago

clalancette commented 1 month ago

Basic Info

Info Please fill out this column
Ticket(s) this addresses N/A
Primary OS tested on Ubuntu 22.04
Robotic platform tested on N/A
Does this PR contain AI generated software? No

Description of contribution in a few bullet points

This very large PR deserves a long explanation, so please bear with me.

I was recently using navigation2 along with the turtlebot3 simulation on ROS 2 Iron, and I noticed that compiling navigation2 took ages. In particular, it looks like compiling nav2_system_tests is extremely slow.

That led me to look into how things are being compiled. Besides being slow, I noticed that navigation2 isn't using many of what we consider the "modern" CMake practices in ROS 2. Thus, this PR implements the following things:

  1. Make sure to generate CMake "targets" in all packages. We do this so that downstream packages only need to link against the exact target that they need, and the individual targets only export the things that are PUBLIC. This is the main source of the speedup of compilation in this PR. The way this is done is by using the EXPORT command to install, and also calling ament_export_targets to generate the appropriate CMake glue.
  2. Removal of ament_target_dependencies in favor of target_link_libraries. ament_target_dependencies was created in the days before target_link_libraries, and as such doesn't have as many features. In particular, ament_target_dependencies works on the concept of packages, while target_link_libraries works on the concept of "targets", which can have arbitrary PUBLIC and PRIVATE linked libraries and header files. I'll note that this switch in focus does have a downside, in that the dependencies that we link against are targets while the dependencies we export (via ament_export_dependencies) are package names.
  3. Switch to installing header files in include/${PROJECT_NAME}/${PROJECT_NAME}. This was a change we pushed in Humble, and ensures that overlays work in all situations. You can see more about why we did this in http://docs.ros.org/en/humble/Releases/Release-Humble-Hawksbill.html#c-headers-are-installed-in-a-subdirectory and https://github.com/ros2/ros2/issues/1150 .
  4. Remove include_directories in favor of target_include_directories. This is best practice nowadays to more precisely control exactly what include directories are used.
  5. Cleanup of dependencies in general, adding them where needed and removing them if not needed.
  6. Cleanup of package.xml files, ensuring that we use depend as needed (which implies build_export_depend as well as build_depend and exec_depend).
  7. Addition of the proper #include in C++ and header files. Not only is this cleaner, it also makes it much easier to determine which dependencies should be PUBLIC, and which ones PRIVATE.

With all of these cleanups in place, on my local machine compilation went from over 22 minutes to 9 minutes.

This PR targets the iron branch, as that is what I was using for my development. But I can retarget this to another branch depending on the development practices for this repository.

Note that I do not expect this PR to go in as-is (which is why it is a draft). I didn't even run any tests on this yet, and it requires both https://github.com/ros/bond_core/pull/94 and https://github.com/BehaviorTree/BehaviorTree.CPP/pull/826 before it will even compile. But I wanted to get the reaction of the developers here to the "shape" of what this will look like, and get early feedback. If this is something that the maintainers don't want, then I'll just close this. If the maintainers are OK with how this generally looks, then I'll probably open a separate PR per package, implementing the changes.

Thoughts?

Description of documentation updates required from your changes

Future work that may be required in bullet points

For Maintainers:

mergify[bot] commented 1 month ago

@clalancette, all pull requests must be targeted towards the main development branch. Once merged into main, it is possible to backport to @iron, but it must be in main to have these changes reflected into new distributions.

SteveMacenski commented 1 month ago

First of all; thanks!

I certainly don't mind the update (I know this will make @Ryanf55 very happy), I just want the stack to be consistent; which this does since you touch all the packages! Happy for faster compiling and more modern cmake if we can get the full stack over to it.

clalancette commented 1 month ago

I certainly don't mind the update (I know this will make @Ryanf55 very happy), I just want the stack to be consistent; which this does since you touch all the packages! Happy for faster compiling and more modern cmake if we can get the full stack over to it.

Just to be totally clear on this point; I didn't convert all of navigation2 here, though I did do most of it. If we want to go down this route, then I'll continue working on this, as there are probably 6 or so packages left to do.

SteveMacenski commented 1 month ago

The change is definitely welcome!

mergify[bot] commented 1 month ago

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

ruffsl commented 1 month ago

With all of these cleanups in place, on my local machine compilation went from over 22 minutes to 9 minutes.

Thanks so much @clalancette! Build time were getting out of hand, but my hate for CMake left me despondent.

SteveMacenski commented 1 month ago

The changes from boost filesystem to std::filesystem were a good change too.

It wasn't in the version of C++ when that part of the code was written :smiling_face_with_tear: But yes, kill boost whereever possible!

@clalancette move forward with this as you see fit, I'm not a backseat driver. I'll review / respond as you're ready

clalancette commented 1 month ago

@clalancette move forward with this as you see fit, I'm not a backseat driver. I'll review / respond as you're ready

Thanks! I've been actually working on some tooling for this in the background, which is why I've been kind of quiet here. I'm on vacation next week, but I'll pick up work on that tooling (and updating this PR) once I'm back.