ament_cmake function to consolidate compile options

[jpsamper2009]

Motivation

• Currently, each ROS 2 package is responsible for setting the compile options for each of its targets, which can lead to two packages having incompatible ABIs, causing runtime issues
• At the same time, for ROS 2 to unify all the compile options and keep them in sync, all packages must be touched and updated, which is an error prone process
• The proposal here is to create an ament_cmake function, let's call it ament_target_compile_options(target_name), which can be called after every add_library or add_executable call to set the compile option for each target

Goals

• Have a single location where one can configure compile options for all ROS 2 packages, and have those settings propagate to all targets

Steps

1. Create an empty ament_target_compile_options
2. Implement ament_add_library/executable (optional - see 3)
3. Either use ament_add_library/executable or add ament_target_compile_options after every add_library/executable call

Open question

• Should the compile options also be added to add_test?

Future work

• Set compile options in this single location, and fix any errors that pop up when compile options are unified

[dirk-thomas]

To clarify which compile options you are referring to (which affect ABI) can you please explicitly enumerate them.

I see a few problems with the described approach:

• Plain CMake packages not using the function would still be subject to ABI incompatibilities.
• In general we don't want to embed ROS specific values like this in ament* packages. Maybe ament_cmake_ros would be a better place. (Obviously that would raise the bar of the first bullet to include packages which use ament_cmake but not ament_cmake_ros.)

CMake toolchain files would be one proper way of ensuring the same compiler options are being used across multiple packages. Others would be passing CMAKE_CXX_STANDARD, CMAKE_CPP_FLAGS, etc. to CMake.

Should the compile options also be added to add_test?

The CMake function add_test doesn't build any code so I don't think compile options are relevant here.

[jpsamper2009]

I like the idea of a toolchain file. I'll look into that and report back.

As for add_test: maybe I was thinking of packages like ament_add_gtest, where an executable is created, but I guess this scenario would be addressed by the toolchain file too

[jpsamper2009]

@dirk-thomas I've looked into using a toolchain file, and I see two main drawbacks:

1. The recommended way of setting a compiler options is with target_set_compile_option
   • Granted, this still leave the issue of plain CMake packages, in which case a toolchain file is still the better solution
2. Having to type out the path to the toolchain file when calling colcon:

colcon build --cmake-args -DCMAKE_TOOLCHAIN_FILE="$(pwd)/path/to/toolchain.cmake"

I was thinking, I could just add the --cmake-args to the colcon defaults.yaml, but then I would have to hard-code the path, and I would have to change it for different workspaces. Or is there a way to have variables (e.g. PWD or current work directory) that can be used in the defaults.yaml?

I also realized I haven't answered the question about which flags affect the ABI. This article gives a good overview of compiler flags and their potential consequences

[dirk-thomas]

The recommended way of setting a compiler options is with target_set_compile_option
Granted, this still leave the issue of plain CMake packages, in which case a toolchain file is still the better solution

That is why a toolchain file is the preferred approach.

Having to type out the path to the toolchain file when calling colcon:

While that is certainly not convenient there is no reason this can't be improved. That would probably fall into the same category as colcon/colcon-core#168.

I could just add the --cmake-args to the colcon defaults.yaml, but then I would have to hard-code the path, and I would have to change it for different workspaces.

The downside of using defaults.yaml is that if you pass custom --cmake-args on the command line they will override your defaults. Using a mixin is probably more appropriate - but that doesn't solve the need for different configurations for different workspaces if it contains workspace specific paths.

I also realized I haven't answered the question about which flags affect the ABI. This article gives a good overview of compiler flags and their potential consequences

That doesn't really answer the question which of the flags you want to use.

[jpsamper2009]

@dirk-thomas At a basic level, we would like to use -ftree-vectorize and -grecord-gcc-switches:

• -ftree-vectorize will affect the ABI
• -grecord-gcc-switches will allow us to write tools that check that the different binaries are built with the same flags, so we can tell, for example, that a given package didn't add a problematic flag like -fsanitize=address

[jpsamper2009]

@dirk-thomas Would it be viable to extend colcon to allow variables in the defaults.yaml or metadata files?

[dirk-thomas]

Would it be viable to extend colcon to allow variables in the defaults.yaml or metadata files?

Of course 👍

[dirk-thomas]

Closing due to no activity. It also sounds like the direction is to address this on the build tool level instead.