Revamp CMake to make compilation significantly faster #4357
Conversation
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_util, 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. Removes the separate src/CMakeLists.txt. While this
isn't strictly necessary, it makes it a lot easier to do
the rest of the changes in here.
4. Makes sure to export nav2_util as a CMake target, so
downstream users of it can use that target.
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
|
@clalancette, all pull requests must be targeted towards the |
| add_subdirectory(src/motion_model) | ||
| add_subdirectory(src/sensors) | ||
| if(CMAKE_CXX_COMPILER_ID MATCHES "Clang") | ||
| add_compile_options(-Wno-gnu-folding-constant) |
There was a problem hiding this comment.
Can you explain this added for AMCL specifically?
There was a problem hiding this comment.
Yeah, this is a good question. The honest truth is that for reasons I don't totally understand, having the targets in the subdirectory doesn't produce nav2_amcl targets that "worked". In particular, the downstream package linking against it (nav2_system_tests) would fail to properly find the target. Moving this all together into the main CMakeLists.txt seemed to make it work. It bears more investigation into why that is happening.
| PRIVATE | ||
| "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>" | ||
| "$<INSTALL_INTERFACE:include/${PROJECT_NAME}>") | ||
| target_link_libraries(${bt_plugin} PUBLIC |
There was a problem hiding this comment.
Question: Is there much of a speed up by specifying the exact targets vs all the targets in the packages? It is somewhat convenient to be able to specify the dependencies variable and just target the set as a whole without having to think about it much. But if this is a big speed up, that is a worthwhile trade off to me to specify them individually.
There was a problem hiding this comment.
So yeah, you get most of the speed-up by specifying the exact targets, and splitting this into PUBLIC and PRIVATE. That is, anything that is marked as PRIVATE will not be exposed to the downstream targets, thus shortening the list of things they have to link against later on.
Unfortunately, as it stands, ament_target_dependencies does not fully support this (in particular, it doesn't support PRIVATE). We could add it to ament_target_dependencies, but since we are trying to slowly move away from it, I didn't think it was particularly worth it.
While thinking about it, I did come up with another way to do this, which would reduce duplication. Inside of nav2_common, we could have a "map" between a package name and target names. And then we could have a custom cmake macro/function here that, given a target and a list of packages, automatically looked up the mapping between a package name and a target name, and called target_link_libraries. The caller would call something like nav2_link_libraries(TARGET mytarget PACKAGES foo bar). The downside is that it is more CMake "magic", which is is something that people have complained about for years with ament. I also don't know what the impact on compile times will be. Nevertheless, if you think this is interesting, I can look into prototyping it.
|
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. |
| #include "nav2_util/node_utils.hpp" | ||
| #include "rcl_action/action_server.h" | ||
| #include "rclcpp_lifecycle/lifecycle_node.hpp" |
There was a problem hiding this comment.
I'm curious how you identified the headers to remove or the C libs to add in in place of others (here and in other files)? I expect this was a bit trial and error as you were adding in the specific targets within each package, but if there was something more automated you used that would be good for me to have in my toolbox as a future reviewer of changes to this codebase after this PR
There was a problem hiding this comment.
Good question.
It was completely manual for right now. For further work here, I'm going to look into doing something much more automated, because it's not particularly sustainable with the amount of code that exists in this repository.
There was a problem hiding this comment.
Nice, this is a much needed improvement to bring NAV2 in line with the newer conventions in the ament tutorials. This is similar to a few other pull requests I have done in NAV2. Overall, I approve the intent of this work, but cannot do a detailed review of every line of code change. Nice benchmarks on the system tests. Turns out, not linking everything to everything speeds up the build!
The changes from boost filesystem to std::filesystem were a good change too.
| set(library_name ${PROJECT_NAME}_core) | ||
| add_subdirectory(src) | ||
|
|
||
| add_library(${library_name} SHARED |
There was a problem hiding this comment.
Why hard-code SHARED? This breaks the ability for people to compile the util as a static lib, right?
There was a problem hiding this comment.
I carried this over from what existed already. Actually, another good change we could make here would be to switch these packages over to ament_cmake_ros, which automatically handles making libraries shared by default, but also allows using -DBUILD_SHARED=OFF.
| add_subdirectory(src) | ||
|
|
||
| add_library(${library_name} SHARED | ||
| src/costmap.cpp |
There was a problem hiding this comment.
I find it nicer to use target_sources in another CMakeLists in the src directory because then you can just copy/paste the output of ls -1 *.cpp into the CMakeLists, and diffs are isolated to the directories that changed, but I understand you might be trying to keep par with how the ROS core repos do it all at the top-level CMakeLists.
There was a problem hiding this comment.
Yeah, my goal was to keep it mostly the same as the core.
In my opinion, the way we split up packages in ROS 2 actually makes it less interesting to have separate CMakeLists.txt in separate directories. That is, most packages are relatively small, and thus having everything in one CMakeLists.txt at the top-level is viable for most packages. It also removes one of my pet peeves with sub-CMakeLists.txt, where they often rely on "magic" variables inherited from the parent.
All of that said, I can move things back down into the individual sub-directories here, depending on maintainer preference. It isn't a core part of speeding things up.
| @@ -52,8 +52,7 @@ set(dependencies | |||
| set(local_controller_plugin_lib local_controller) | |||
|
|
|||
| add_library(${local_controller_plugin_lib} SHARED src/error_codes/controller/controller_error_plugins.cpp) | |||
| ament_target_dependencies(${local_controller_plugin_lib} ${dependencies}) | |||
| target_compile_definitions(${local_controller_plugin_lib} PUBLIC "PLUGINLIB__DISABLE_BOOST_FUNCTIONS") | |||
| target_link_libraries(${local_controller_plugin_lib} PRIVATE nav2_core::nav2_core nav2_costmap_2d::nav2_costmap_2d_core ${nav_msgs_TARGETS} ${geometry_msgs_TARGETS} pluginlib::pluginlib) | |||
There was a problem hiding this comment.
Some of these I would do a line limit and break up to one dependency per line so it fits on an editor without wrap.
There was a problem hiding this comment.
Yeah, 100%. When I open up separate PRs, I'll definitely do that and run them through the linters.
| ${nav_2d_msgs_TARGETS} | ||
| tf2::tf2 | ||
| ${tf2_geometry_msgs_TARGETS} | ||
| conversions |
There was a problem hiding this comment.
Is there an alias target available or one that could be created to reduce the chances of linker errors due to typos on the library name?
There was a problem hiding this comment.
So this is one oddity of CMake. Any libraries we depend on from this package haven't yet had their targets created, so we can't use the traditional nav_2d_utils::conversions target like we would in downstream packages. Instead, we just have to use conversions.
While we could add in aliases, I think it is actually more confusing because those aliases clash with the eventual targets that would be created. I guess we could create an alias called internal::conversions, though I'm not sure it is worth it. It might be a better idea to rename the library to something more descriptive, though that could also have downstream implications.
There was a problem hiding this comment.
I've gotten ALIAS targets working well in about 120 packages. If you do the ALIAS as ${PROJECT_NAME}::your_target, then the build target name is the same as ament's exported name. From what I've seen, ALIAS targets don't show up in the install interface, and naming them the same is recommended: https://youtu.be/bsXLMQ6WgIk?si=SPk624LzEa2UhLU5&t=3133
I have yet to have issues doing this method, and will routinely create ALIAS targets so my code in test can link to the right library with configure errors if there are typos.
| @@ -19,6 +19,10 @@ | |||
| <depend>nav_2d_msgs</depend> | |||
| <depend>nav2_core</depend> | |||
| <depend>pluginlib</depend> | |||
| <depend>lifecycle_msgs</depend> | |||
There was a problem hiding this comment.
How is bloom missing these on iron and passing the build?
There was a problem hiding this comment.
Yeah, it's a good question.
Most likely, these are being pulled in by something else. In this case, it looks like nav2_util depends on lifecycle_msgs, and so nav2_controller inherits it from there. But since this package directly depends on it, we should technically have this dependency here as well.
There was a problem hiding this comment.
Ah ok. I thought bloom was supposed to catch transitive dependencies. I must be remembering wrong. Good catch!
There was a problem hiding this comment.
Ah ok. I thought bloom was supposed to catch transitive dependencies. I must be remembering wrong. Good catch!
It's complicated. In point of fact, bloom won't check at all that your dependencies are "correct", it will only check that your dependencies can be resolved into system or ROS packages.
The buildfarm goes further, in that it when it goes to build your package, it will start with an absolutely minimal Ubuntu container, and just apt-get install exactly what you put in your package.xml. If a dependency is missing from direct and indirect dependencies, it will definitely find that. But if you happen to pull in a package via an indirect dependency, things will just work. That's what is happening here.
There was a problem hiding this comment.
Thank you, that clears it up. Good finds on catching all the missing ones.
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. |
|
The change is definitely welcome! |
|
This pull request is in conflict. Could you fix it @clalancette? |
|
Here's the list of packages that are left to convert (I'll be opening separate PRs for each of them):
|
|
Closing this one as we've done a lot more work in the series that followed it. |
Basic Info
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_testsis 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:
EXPORTcommand toinstall, and also callingament_export_targetsto generate the appropriate CMake glue.ament_target_dependenciesin favor oftarget_link_libraries.ament_target_dependencieswas created in the days beforetarget_link_libraries, and as such doesn't have as many features. In particular,ament_target_dependenciesworks on the concept of packages, whiletarget_link_librariesworks 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 (viaament_export_dependencies) are package names.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 Overlaying packages using CMake export targets can fail with merge install underlay ros2/ros2#1150 .include_directoriesin favor oftarget_include_directories. This is best practice nowadays to more precisely control exactly what include directories are used.dependas needed (which impliesbuild_export_dependas well asbuild_dependandexec_depend).#includein 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
ironbranch, 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 ros/bond_core#94 and BehaviorTree/BehaviorTree.CPP#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: