Major revamp of the ament_cmake documentation.

[clalancette]

Rearrange the document to more closely follow the order a typical CMakeLists.txt is arranged in. While doing that, update the documentation to match our current best practices, and add in additional advice for building/installing executables as opposed to libraries.

@Ryanf55 FYI. I'm very interested to hear your feedback about this revamp.

[Ryanf55]

Overall, the changes look good. A few touch-ups here and there. Great improvement.

[Ryanf55]

Perhaps consider target_sources? I actually really like this by adding a CMakeLists in src and include folders to make the top level one more digestable. However, neither splitting the CMake code nor using target_sources is common the ROS core.

[clalancette]

We don't use this in the core currently, so I don't think I'll add this here.

[Ryanf55]

Could be useful to explain why we are still recommending ament_target_dependencies

[clalancette]

It's a hard one to explain, to be honest. The fact is that you can do almost everything you want with target_link_libraries nowadays, and I've been (slowly) converting the core over to using that over ament_target_dependencies. The one big thing that doesn't work is message packages; you can't depend on sensor_msgs::sensor_msgs, you have to instead depend on ${sensor_msgs_TARGETS}. That's the main reason to still recommend ament_target_dependencies, but I'm not convinced that explanation is necessary/useful here.

[Ryanf55]

The special case for using messages is already covered in other tutorials. I use target_link_libraries for basically every library except message packages. It could be useful to instead acknowledge the fact that both may be used together.

[Ryanf55]

Might want to add the DEFINED keyword because it could be defined, but empty, in which case you end up not setting the standard.
https://github.com/cpp-best-practices/cmake_template/blob/d19d8921771ba582824948f0cc57f849a5c6efd8/CMakeLists.txt#L8

[clalancette]

I ended up not doing this, because this isn't what we do in the core.

Actually, this is one place where what ros2 pkg create does and what the core does differ. In the core, we use:

if(NOT CMAKE_CXX_STANDARD)
  set(CMAKE_CXX_STANDARD 17)
endif()

But in ros2 pkg create, we generate:

target_compile_features(mylib PUBLIC c_std_99 cxx_std_17)  # Require C99 and C++17

I'm not sure which is better, and we don't use DEFINED in either place. I think I'm going to leave this one as-is for now, though we should probably rectify this situation one way or the other.

[Ryanf55]

I'll create another ticket in ros2/ros2.

[Ryanf55]

Converted to ros2/ros2#1467

[Ryanf55]

CMAKE_COMPILER_IS_GNUCXX is deprecated.

Suggested change

	if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
	if(CMAKE_CXX_COMPILER_ID MATCHES "GNU" OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")

[clalancette]

In the core, we use CMAKE_COMPILER_IS_GNUCXX everywhere. This could probably also be changed as part of ros2/ros2#1467, but I think here we should document what we currently use.

[Ryanf55]

Do you have any automatic refactoring and/or PR tools? There's 167 instances of it in ros2 rolling repos, and it looks like a simple sed replacement would work.

[clalancette]

I'm going to go ahead and merge and backport this one. Thanks for all of your feedback.