Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass include paths to cppcheck #117

Merged
merged 16 commits into from
Jan 10, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 28 additions & 1 deletion ament_cmake_cppcheck/cmake/ament_cmake_cppcheck_lint_hook.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
# See the License for the specific language governing permissions and
# limitations under the License.

find_package(ament_cmake_core REQUIRED)

file(GLOB_RECURSE _source_files FOLLOW_SYMLINKS
"*.c"
"*.cc"
Expand All @@ -24,5 +26,30 @@ file(GLOB_RECURSE _source_files FOLLOW_SYMLINKS
)
if(_source_files)
message(STATUS "Added test 'cppcheck' to perform static code analysis on C / C++ code")
ament_cppcheck()

# Get include paths for added targets
set(_all_include_dirs "")
# BUILDSYSTEM_TARGETS only supported in CMake >= 3.7
if(NOT CMAKE_VERSION VERSION_LESS "3.7.0")
get_directory_property(_build_targets DIRECTORY ${CMAKE_SOURCE_DIR} BUILDSYSTEM_TARGETS)
foreach(_target ${_build_targets})
get_property(_include_dirs
TARGET ${_target}
PROPERTY INCLUDE_DIRECTORIES
)

# Only append include directories that are from the package being tested
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sloretz See #125.

You can pass cppcheck one or more include directories that contain the macros it is trying to resolve.
I'm not satisfied with the solution, but I think anything better would require changes in cppcheck itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks, I missed that PR

# This accomplishes two things:
# 1. Reduces execution time (less include directories to search)
# 2. cppcheck will not check for errors in external packages
Copy link
Contributor

Choose a reason for hiding this comment

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

This fails on Windows in this case: ros2/ros2#942.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand the case that's failing. From the referenced ticket, it looks like the failures are due to headers from external packages not being passed to cppcheck (which is the intended behavior). For this case, we have ament_cmake_cppcheck_ADDITIONAL_INCLUDE_DIRS that can be set (introduced in #125).

Copy link
Contributor

Choose a reason for hiding this comment

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

Without the proper include directories cppcheck warns for unknown macros.

ros2/rclpy#577 sets an additional include dir.

foreach(_include_dir ${_include_dirs})
string(REGEX MATCH "^${CMAKE_SOURCE_DIR}.*" _is_match ${_include_dir})
if(_is_match)
list_append_unique(_all_include_dirs ${_include_dir})
jacobperron marked this conversation as resolved.
Show resolved Hide resolved
endif()
endforeach()
endforeach()
endif()

ament_cppcheck(INCLUDE_DIRS ${_all_include_dirs})
endif()
7 changes: 6 additions & 1 deletion ament_cmake_cppcheck/cmake/ament_cppcheck.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,15 @@
# :type TESTNAME: string
# :param LANGUAGE: the language argument for cppcheck, either 'c' or 'c++'
# :type LANGUAGE: string
# :param INCLUDE_DIRS: an optional list of include paths for cppcheck
# :type INCLUDE_DIRS: list
# :param ARGN: the files or directories to check
# :type ARGN: list of strings
#
# @public
#
function(ament_cppcheck)
cmake_parse_arguments(ARG "" "LANGUAGE;TESTNAME" "" ${ARGN})
cmake_parse_arguments(ARG "" "LANGUAGE;TESTNAME" "INCLUDE_DIRS" ${ARGN})
if(NOT ARG_TESTNAME)
set(ARG_TESTNAME "cppcheck")
endif()
Expand All @@ -38,6 +40,9 @@ function(ament_cppcheck)
set(result_file "${AMENT_TEST_RESULTS_DIR}/${PROJECT_NAME}/${ARG_TESTNAME}.xunit.xml")
set(cmd "${ament_cppcheck_BIN}" "--xunit-file" "${result_file}")
list(APPEND cmd ${ARG_UNPARSED_ARGUMENTS})
if(ARG_INCLUDE_DIRS)
list(APPEND cmd "--include_dirs" "${ARG_INCLUDE_DIRS}")
jacobperron marked this conversation as resolved.
Show resolved Hide resolved
endif()
if(ARG_LANGUAGE)
list(APPEND cmd "--language" "${ARG_LANGUAGE}")
endif()
Expand Down
1 change: 1 addition & 0 deletions ament_cmake_cppcheck/package.xml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
<buildtool_depend>ament_cmake_core</buildtool_depend>
<buildtool_depend>ament_cmake_test</buildtool_depend>

<buildtool_export_depend>ament_cmake_core</buildtool_export_depend>
<buildtool_export_depend>ament_cmake_test</buildtool_export_depend>
<buildtool_export_depend>ament_cppcheck</buildtool_export_depend>

Expand Down
12 changes: 9 additions & 3 deletions ament_cppcheck/ament_cppcheck/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
# limitations under the License.

import argparse
from collections import defaultdict
import multiprocessing
import os
from shutil import which
Expand All @@ -39,6 +40,11 @@ def main(argv=sys.argv[1:]):
help='Files and/or directories to be checked. Directories are searched recursively for '
'files ending in one of %s.' %
', '.join(["'.%s'" % e for e in extensions]))
parser.add_argument(
'--include_dirs',
nargs='*',
help="Include directories for C/C++ files being checked."
"Each directory is passed to cppcheck as '-I <include_dir>'")
parser.add_argument(
'--language',
help="Passed to cppcheck as '--language=<language>', and it forces cppcheck to consider "
Expand Down Expand Up @@ -86,6 +92,8 @@ def main(argv=sys.argv[1:]):
'--xml-version=2']
if args.language:
cmd.extend(['--language={0}'.format(args.language)])
for include_dir in (args.include_dirs or []):
cmd.extend(['-I', include_dir])
if jobs:
cmd.extend(['-j', '%d' % jobs])
cmd.extend(files)
Expand All @@ -105,9 +113,7 @@ def main(argv=sys.argv[1:]):
return 1

# output errors
report = {}
for filename in files:
report[filename] = []
report = defaultdict(list)
dirk-thomas marked this conversation as resolved.
Show resolved Hide resolved
for error in root.find('errors'):
location = error.find('location')
filename = location.get('file')
Expand Down