Fix build with yaml-cpp installed system-wise

[mjbogusz]

EDIT: Reduced the scope of the PR. Now it only fixes using system-wise installed yaml-cpp's INCLUDE_DIR which resulted in passing -isystem /usr/include to compiler.

Below is the comment for original scope of this PR.

---

This fixes issues mentioned in #152:

• Passing -isystem /usr/include to compiler (resulting in fatal error: stdlib.h: No such file or directory) if yaml-cpp is installed system-wise
• Not using system-wise installed yaml-cpp (thus requiring to additionally download and compile)
• Checking for variadic macro compiler flag (unrecognized command line option ‘-Wno-gnu-zero-variadic-macro-arguments’)

I can split this PR into two (yaml-cpp and variadic macros) if that's the preferred workflow.

[racko]

Is rviz_yaml_cpp_vendor_LOCAL_YAML actually available when rviz_yaml_cpp_vendor has been installed (e.g. via a package manager)? I don't think so, though I don't have a way to prove it. A quick grep shows:

$ grep -R rviz_yaml_cpp_vendor_LOCAL_YAML /home/racko/ros2_ws/install_isolated/rviz_yaml_cpp_vendor      
/home/racko/ros2_ws/install_isolated/rviz_yaml_cpp_vendor/share/rviz_yaml_cpp_vendor/cmake/rviz_yaml_cpp_vendor-extras.cmake:if(${rviz_yaml_cpp_vendor_LOCAL_YAML})
/home/racko/ros2_ws/install_isolated/rviz_yaml_cpp_vendor/share/rviz_yaml_cpp_vendor/cmake/rviz_yaml_cpp_vendor-extras.cmake:if(${rviz_yaml_cpp_vendor_LOCAL_YAML})

So in the install space rviz_yaml_cpp_vendor_LOCAL_YAML is only read, but never initialized.

Of course one could change rviz_yaml_cpp_vendor-extras.cmake.in to figure out the value of rviz_yaml_cpp_vendor_LOCAL_YAML based on the paths returned by find_package(yaml-cpp ...)?

It might actually be better to use the modern-cmake target provided by yaml_cpp instead of YAML_CPP_LIBRARIES and YAML_CPP_INCLUDE_DIR:

/usr/lib/cmake/yaml-cpp/yaml-cpp-targets.cmake, lines 47-48:

# Create imported target yaml-cpp
add_library(yaml-cpp SHARED IMPORTED)

/usr/lib/cmake/yaml-cpp/yaml-cpp-targets-release.cmake:

...
# Import target "yaml-cpp" for configuration "Release"
set_property(TARGET yaml-cpp APPEND PROPERTY IMPORTED_CONFIGURATIONS RELEASE)
set_target_properties(yaml-cpp PROPERTIES
  IMPORTED_LOCATION_RELEASE "/usr/lib/libyaml-cpp.so.0.5.3"
  IMPORTED_SONAME_RELEASE "libyaml-cpp.so.0.5"
  )
...

We can see that ${YAML_CPP_CMAKE_DIR}/../../../include is not added to INTERFACE_INCLUDE_DIRECTORIES (or anywhere else for that matter).

[mjbogusz]

You're right on availability of rviz_yaml_cpp_vendor_LOCAL_YAML. This can be however easily fixed by wrapping it in @-s in that .cmake.in file (${@rviz_yaml_cpp_vendor_LOCAL_YAML@}). This way it should be substituted inside ament_package() which I believe calls configure_file() internally. I'll update the PR.

System-installed yaml-cpp does not have to set additional include directories, as it's placed in the global /usr/include anyway. However we do have to when using local compilation.

[mjbogusz]

I've updated the PR.

Now the generated install/share/rviz_yaml_cpp_vendor/cmake/rviz_yaml_cpp_vendor-extras.cmake file looks like this:

(...)
if(0)
  set(rviz_yaml_cpp_vendor_INCLUDE_DIRS ${YAML_CPP_INCLUDE_DIR})
endif()

[wjwwood]

You can't make this assumption. If I install yaml cpp to ~/local and add that to my CMAKE_PREFIX_PATH this assumption won't hold.

Also, it doesn't buy you anything to special case this.

[mjbogusz]

This in fact has fixed the fatal error: stdlib.h: No such file or directory error - if /usr/include, which system-wise installed yaml-cpp reports in YAML_CPP_INCLUDE_DIR, is passed to compiler as -isystem it breaks everything.

What would you suggest then?

[wjwwood]

I would suggest that you have a special case (this version of yaml-cpp on Linux for example) and in that case iterate over the headers and remove/change the /usr/include instance. Assuming the include directory is not needed when we don't build it locally isn't a safe assumption to make.

is passed to compiler as -isystem it breaks everything.

Separate question, why is the content of YAML_CPP_INCLUDE_DIR getting passed after -isystem rather than -I? Is something using include_directories(SYSTEM ...) somewhere?

[mjbogusz]

I've finally got time to dig into the issue a bit more. It's quite a whack-a-mole...

• The issue originates in rviz_yaml_cpp_vendor/rviz_yaml_cpp_vendor-extras.cmake.in, which sets _INCLUDE_DIRS to ${YAML_CPP_INCLUDE_DIR} aka /usr/include if system-wise yaml-cpp is available
• Vendor package's _INCLUDE_DIRS is then passed around to other packages using it
• From there it's being passed deeper and deeper into ament's internal cmake macros, functions etc
• One thing I've managed to find: ament's normalize_path does not (apparently) unwrap paths like /usr/lib64/cmake/yaml-cpp/../../../include (it would probably be automatically skipped then)
• It ends up in all dependent packages' generated flags.make with -isystem with a bunch of other directories (why? I haven't found out yet)

For now, I've simply disabled the if(NOT yaml-cpp_FOUND) check in the .cmake.in file. It's a bit of a workaround but it allows rviz to build and it reduces the scope of this PR.

I'll open a separate issue for optionally using system-wise yaml-cpp and another one for ament's normalize_path (if I confirm the erroneous behaviour).

[mjbogusz]

This might also be a fix/workaround for #224.

[mikaelarguedas]

@wjwwood FYI: I put the bouncy milestone on this as this is currently making the builds fail on bionic due to higher gcc version

[mikaelarguedas]

After discussing offline with @wjwwood we are going to merge this as is and always use the custom build of yaml-cpp for now.

Follow-up work has been ticketed at #269

[mikaelarguedas]

• Linux
• Linux-aarch64
• macOS
• Windows

[mikaelarguedas]

merging this..
@wjwwood FYI

[mikaelarguedas]

thanks @mjbogusz for the investigation and the contribution!