Consider storing compiler flags in a single place and reuse them everywhere

[mikaelarguedas]

Feature request

Feature description

Currently compiler flags are set in each package. This makes adding new flags throughout the stack a lot of effort.

If we could define them in a single place and reuse them in every package it would make updating them easier.

Related to ros2/rcutils#114 and connected PRs

Implementation considerations

We could provide these in ament_cmake this way any ament_cmake package could access them. We would still set them manually on pure CMake packages.

If we want to avoid the coupling between "ROS_DEFAULT_FLAGS" and the build system, we could add them to ament_cmake_ros.

[dirk-thomas]

Imo ament_cmake shouldn't continue that subjective information. ament_cmake_ros sounds better to be.

The main question is if these are opt-in or being set by default (with a way to opt-out?).

[wjwwood]

So after discussing this at the ROS 2 meeting and with @mikaelarguedas, we've come up with a proposal, something like this (in addition to a dependency on ament_cmake_ros):

find_package(ament_cmake_ros REQUIRED)
# a variable is automatically setup, e.g. `ament_cmake_ros_compiler_flags`
# can optionally be pruned with something like
# ament_cmake_ros_remove_compiler_flags(output_var_name input_list EXCLUDE "-W...")
# then can be manually used (allowing for either global or target specific usage)

# For C++ only projects:
add_compile_options(${ament_cmake_ros_compiler_flags})
# For projects with C code (including messages and therefore Python generated code):
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${ament_cmake_ros_compiler_flags}")
# For targets:
target_compile_options(target <INTERFACE|PUBLIC|PRIVATE> ${ament_cmake_ros_compiler_flags})

Please give feedback here if you have feedback on this proposal's approach. I'll start implementing it soon.

[wjwwood]

@mikaelarguedas will this always apply?

https://github.com/ros2/rclcpp/blob/b1af28047c26da0fe5723e3ea95ed1143bc224e5/rclcpp/CMakeLists.txt#L22-L27

[mikaelarguedas]

sounds good to me.

Still thinking about the variable name. As I don't have a better alternative +1 to implement as described 👍

[mikaelarguedas]

mikaelarguedas will this always apply?
https://github.com/ros2/rclcpp/blob/b1af28047c26da0fe5723e3ea95ed1143bc224e5/rclcpp/CMakeLists.txt#L22-L27

Actually that's a good timing to revisit it! From what I remember it was an issue of casting pointers to function pointers. As ros2/rosidl_python#10 should remove retrieving C function pointer from Python objects, we may be able to reenable pedantic on message packages once that PR lands.

Original ticket tracking stricter compiler flags (for cross-reference): #305 (comment)

[mikaelarguedas]

A few things to consider when implementing:

• Compilers:
  How do we want to handle differences between compilers? It could be useful to populat the variable differently based on the detected CMAKE_CXX_COMPILER (or have several variables?)
  • while we currently don't set anything explicitly for MSVC we may pass stricter compiler options on windows in the future
  • Some differences in compiler flags support between GCC and Clang also surfaced recently: Unknown warning option on OSX eProsima/Fast-CDR#22
• Languages:
  • I don't think it ever came up but we may want different flags for C and C++ at some point

[wjwwood]

Ok, let me update the proposal based on:

• have separate compiler flags variables for each compiler
• have a "default" variable that uses the detected compiler
• have one for C and C++, but have them be identical by default
• also handle C and CXX standard with a variable

find_package(ament_cmake_ros REQUIRED)
# automatically creates variables:
#   - ament_cmake_ros_cpp_compiler_flags
#   - ament_cmake_ros_c_compiler_flags
#   - ament_cmake_ros_cpp_compiler_flags_<COMPILER_NAME>
#   - ament_cmake_ros_c_compiler_flags_<COMPILER_NAME>
#   - ament_cmake_ros_cpp_standard
#   - ament_cmake_ros_c_standard

# can optionally prune compiler flags with something like:
# ament_cmake_ros_remove_compiler_flags(output_var_name input_list EXCLUDE "-W...")

# For most C/C++ projects:
add_compile_options(${ament_cmake_ros_cpp_compiler_flags})
# For targets:
target_compile_options(target
  <INTERFACE|PUBLIC|PRIVATE> ${ament_cmake_ros_cpp_compiler_flags})

[dirk-thomas]

I am not sure about "just providing variables" with the compiler flags and wonder if a getter function would be "better".

Users might just copy-n-paste the use of the variable without noticing that they don't have the corresponding find_package() call. In that case the variable is simply not defined and no compiler flags are being set.

If a getter function is used instead this would result in a clear error message from CMake - so getting a "hint" that something is missing.

--

Another question: are all compiler flags from the same type - public or private - or could some be public while some others are private?

[mikaelarguedas]

mikaelarguedas will this always apply?
https://github.com/ros2/rclcpp/blob/b1af28047c26da0fe5723e3ea95ed1143bc224e5/rclcpp/CMakeLists.txt#L22-L27

Actually that's a good timing to revisit it! From what I remember it was an issue of casting pointers to function pointers. As ros2/rosidl_python#10 should remove retrieving C function pointer from Python objects, we may be able to reenable pedantic on message packages once that PR lands.

I confirmed (on Linux) that once that PR lands we can go back to using add_compile_options for all message packages 👍

---

Users might just copy-n-paste the use of the variable without noticing that they don't have the corresponding find_package() call. In that case the variable is simply not defined and no compiler flags are being set.

That's a good point, when discussing this offline yesterday we figured that it would allow a minimal change for all the code by not requiring to add a function / macro call in order to get variables with the flags populated.
And that if the variables are named/namespaced properly it should be obvious where they come from.
But if we think it's likely that people will face this issue I would be fine moving to requiring a call.

[wjwwood]

Ok, I can use a function to get the variable and roll the functionality of the remove function into it (could call that function internally still):

find_package(ament_cmake_ros REQUIRED)
# automatically creates these macros:
#   - ament_cmake_ros_get_cpp_compiler_flags(output_var
#       [FOR_COMPILER <compiler_name>]
#       [EXCLUDE <"-W..."> [...]])
#   - ament_cmake_ros_get_c_compiler_flags(output_var
#       [FOR_COMPILER <compiler_name>]
#       [EXCLUDE <"-W..."> [...]])
#   - ament_cmake_ros_get_cpp_standard(output_var)
#   - ament_cmake_ros_get_c_standard(output_var)
# when the compiler is not specified the detected compiler is used, and
# for unknown compilers no error, just empty var because it is impossible to
# know all compilers ahead of time
# can optionally prune compiler flags with EXCLUDE keyword or with this macro:
#   - ament_cmake_ros_remove_compiler_flags(output_var_name input_list EXCLUDE "-W...")

# For most C/C++ projects:
ament_cmake_ros_get_cpp_compiler_flags(my_cpp_compiler_flags)
add_compile_options(${my_cpp_compiler_flags})
# For targets:
ament_cmake_ros_get_cpp_compiler_flags(my_cpp_compiler_flags)
target_compile_options(target
  <INTERFACE|PUBLIC|PRIVATE> ${my_cpp_compiler_flags})

Please +/-1 this updated proposal.

[dirk-thomas]

How about my question from above:

are all compiler flags from the same type - public or private - or could some be public while some others are private?

[wjwwood]

I don't see a reason for that right now. We could add a PUBLIC_ONLY and PRIVATE_ONLY options to those macros in the future if we have a need for it. I believe add_compile_options doesn't discriminate between the two right now and that will likely be the preferred style for most of our packages at this time.

[dirk-thomas]

... that will likely be the preferred style for most of our packages at this time.

We should definitely use the target specific compile options. The goal is use only target specific options and export targets in the future.

[dirk-thomas]

Please +/-1 this updated proposal.

👍

[mikaelarguedas]

Please +/-1 this updated proposal.

👍

[mjcarroll]

👍