Skip to content

use globally defined varible for cppcheck include dirs#125

Merged
Karsten1987 merged 2 commits intomasterfrom
use_global_cppcheck_include_dirs
Feb 12, 2019
Merged

use globally defined varible for cppcheck include dirs#125
Karsten1987 merged 2 commits intomasterfrom
use_global_cppcheck_include_dirs

Conversation

@Karsten1987
Copy link
Copy Markdown
Contributor

@Karsten1987 Karsten1987 commented Feb 12, 2019

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

@Karsten1987 Karsten1987 added the in review Waiting for review (Kanban column) label Feb 12, 2019
@Karsten1987 Karsten1987 self-assigned this Feb 12, 2019
dirk-thomas
dirk-thomas reviewed Feb 12, 2019
View reviewed changes
ament_cmake_cppcheck/cmake/ament_cmake_cppcheck_lint_hook.cmake Outdated

# Get include paths for added targets
set(_all_include_dirs "")
if(DEFINED AMENT_CMAKE_CPPCHECK_INCLUDE_DIRS)
Copy link
Copy Markdown
Contributor

@dirk-thomas dirk-thomas Feb 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@Karsten1987 Karsten1987 Feb 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

@jacobperron jacobperron Feb 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ament_cmake_cppcheck_EXTERNAL_INCLUDE_DIRS?

@dirk-thomas
Copy link
Copy Markdown
Contributor

dirk-thomas commented Feb 12, 2019

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

@Karsten1987
Copy link
Copy Markdown
Contributor Author

Karsten1987 commented Feb 12, 2019

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
Copy link
Copy Markdown
Contributor

jacobperron commented Feb 12, 2019 •
edited
Loading

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
Copy link
Copy Markdown
Contributor

dirk-thomas commented Feb 12, 2019

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
Copy link
Copy Markdown
Contributor

dirk-thomas commented Feb 12, 2019 •
edited
Loading

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

@Karsten1987 Karsten1987 force-pushed the use_global_cppcheck_include_dirs branch from 650de69 to 104cc2c Compare February 12, 2019 17:27
@Karsten1987
Copy link
Copy Markdown
Contributor Author

Karsten1987 commented Feb 12, 2019 •
edited
Loading

changed to ADDITIONAL_INCLUDE_DIRS. Commit 05c794c is signed.

@Karsten1987
use globally defined varible for cppcheck include dirs
05c794c 
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
@Karsten1987 Karsten1987 force-pushed the use_global_cppcheck_include_dirs branch from 104cc2c to 05c794c Compare February 12, 2019 17:35
@dirk-thomas
Copy link
Copy Markdown
Contributor

dirk-thomas commented Feb 12, 2019

changed to ADDITIONAL_INCLUDE_DIRS. Commit 05c794c is signed.

👍

Please update the docblock to mention the new variable.

@Karsten1987 Karsten1987 mentioned this pull request Feb 12, 2019
Merged
@Karsten1987
Copy link
Copy Markdown
Contributor Author

Karsten1987 commented Feb 12, 2019

updated docblock mentioning how to set the include dirs for cppcheck

@Karsten1987
update docblock
7992753 
Signed-off-by: Karsten Knese <karsten@openrobotics.org>

period
@Karsten1987 Karsten1987 force-pushed the use_global_cppcheck_include_dirs branch from 5d6c058 to 7992753 Compare February 12, 2019 20:00
@Karsten1987
Copy link
Copy Markdown
Contributor Author

Karsten1987 commented Feb 12, 2019

rebased for DCO compliance.

@Karsten1987 Karsten1987 merged commit e08e905 into master Feb 12, 2019
@Karsten1987 Karsten1987 deleted the use_global_cppcheck_include_dirs branch February 12, 2019 20:02
@Karsten1987 Karsten1987 removed the in review Waiting for review (Kanban column) label Feb 12, 2019
@dirk-thomas dirk-thomas added the enhancement New feature or request label Feb 12, 2019
@jacobperron jacobperron mentioned this pull request Mar 1, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jacobperron jacobperron jacobperron left review comments

+1 more reviewer

@dirk-thomas dirk-thomas dirk-thomas approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Karsten1987 Karsten1987

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Karsten1987 @dirk-thomas @jacobperron