ament_cmake function to consolidate compile options about ament_cmake HOT 8 CLOSED

@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

from ament_cmake.

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.

from ament_cmake.

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

from ament_cmake.

@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

from ament_cmake.

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.

from ament_cmake.

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

from ament_cmake.

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

Of course 👍

from ament_cmake.

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

from ament_cmake.