use globally defined varible for cppcheck include dirs

[Karsten1987]

introduces a new variable AMENT_CMAKE_CPPCHECK_INCLUDE_DIRS which can be set externally to configure cppcheck's include dirs.

the new variable can then be used externally in projects as such:

if(BUILD_TESTING)
  find_package(ament_cmake_gmock REQUIRED)
  find_package(ament_lint_auto REQUIRED)
  find_package(test_msgs REQUIRED)
  find_package(rosbag2_test_common REQUIRED)

  set(AMENT_CMAKE_CPPCHECK_INCLUDE_DIRS ${rclcpp_INCLUDE_DIRS})
  ament_lint_auto_find_test_dependencies()

  [...]

connects to the CI warnings found here: ros2/rosbag2#86

[dirk-thomas]

I would suggest to use lowercase for the AMENT_CMAKE_CPPCHECK to match the package name and other variables in style.

[Karsten1987]

now that I think about, maybe a completely different name could be good. Might be confusing with the default c++ like include dirs.
what about ament_cmake_cppcheck_EXTERNAL_DIRS?

[jacobperron]

ament_cmake_cppcheck_EXTERNAL_INCLUDE_DIRS?

[dirk-thomas]

For your example usage of this new API what are you intending to set the variable to? Are these paths actually external?

[Karsten1987]

yes, they are meant to be external. AFAIK, the cmake logic implemented in this file gets all local/internal include dirs and specifically excludes external ones. @jacobperron is my understanding here correct?

[jacobperron]

AFAIK, the cmake logic implemented in this file gets all local/internal include dirs and specifically excludes external ones. @jacobperron is my understanding here correct?

Correct.

---

The affected package is rosbag2_transport (ros2/rosbag2#86). It is using a macro from rclcpp, which cppcheck complains about as an "unknown" macro. By providing cppcheck a reference to the rclcpp header containing the macro, it can resolve the includes and the error goes away.

I expect this issue is likely to affect other packages that similarly try to use the same macros from rclcpp or macros from other libraries outside of the package being linted. I'm not sure if there's a way to handle this as part ament_auto_lint, but in the meantime any affected packages can make use of the variable introduced in this PR.

[dirk-thomas]

I am not convinced that "external" is a good prefix here. There is no reason that the path needs to be external. It can literally point anywhere - e.g. into the packages build directory. Therefore I would suggest a different prefix, e.g.: ADDITIONAL or EXTRA.

[dirk-thomas]

@Karsten1987 also please use "Signed-Off-By" for your commits to pass the DCO check.

[Karsten1987]

changed to ADDITIONAL_INCLUDE_DIRS. Commit 05c794c is signed.

[dirk-thomas]

changed to ADDITIONAL_INCLUDE_DIRS. Commit 05c794c is signed.

👍

Please update the docblock to mention the new variable.

[Karsten1987]

updated docblock mentioning how to set the include dirs for cppcheck

[Karsten1987]

rebased for DCO compliance.