Account for INTERFACE libraries when getting target include directories by jacobperron · Pull Request #121 · ament/ament_lint

jacobperron · 2019-01-11T01:00:49Z

Fixes #120

CMake does not allow getting the INCLUDE_DIRECTORIES property from
INTERFACE libraries.
Instead, first check if the property exists, if it does not then try to
get the INTERFACE_INCLUDE_DIRECTORIES property.

Note, if INTERFACE_INCLUDE_DIRECTORIES is not defined an empty list is
returned, but we cannot assume the target is not an interface.
This is why the implementation is conditional on INCLUDE_DIRECTORIES
instead.

jacobperron · 2019-01-11T01:02:09Z

CI (linter tests only):

Linux
Linux-aarch64
macOS
Windows

edit: re-trigger with typo fix

jacobperron · 2019-01-11T01:18:32Z

FYI @Karsten1987

dirk-thomas · 2019-01-11T01:21:18Z

ament_cmake_cppcheck/cmake/ament_cmake_cppcheck_lint_hook.cmake

-        string(REGEX MATCH "^${CMAKE_SOURCE_DIR}.*" _is_match ${_include_dir})
-        if(_is_match)
+        # Check if include directory is a subdirectory of the source directory
+        string(REGEX MATCH "^${CMAKE_SOURCE_DIR}.*" _is_subdirectory ${_include_dir})


This regex will also match sibling directories of e.g packages which start with the same name as this directory. So the regex should have a trailing shash before the wildcard.

Also this might need to consider platform specific path separators.

Same below.

This seems to be a way to do it 0472892
Open to suggestions.

Are the returned include dirs always native paths?

My impression was that CMake prefers paths to be stored with forward slashes internally and then converts them on platforms that need it but I can't find the relevant documentation.

Confirmed on a Windows machine that include dirs are cmake paths. I’ll remove the native path command.

nuclearsandwich · 2019-01-11T17:28:07Z

@jacobperron when you run the next CI round for this can you include rosbag2? I think you'll need to instruct colcon to skip packages that require the ros1 bridge.

jacobperron · 2019-01-11T17:38:21Z

when you run the next CI round for this can you include rosbag2? I think you'll need to instruct colcon to skip packages that require the ros1 bridge.

Done:

Linux
Windows

edit: Just building up to rosbag2_test_common

jacobperron · 2019-01-11T18:50:29Z

Regular CI (linter tests only):

Linux
Linux-aarch64
macOS
Windows

dirk-thomas · 2019-01-11T19:42:41Z

ament_cmake_cppcheck/cmake/ament_cmake_cppcheck_lint_hook.cmake

+        # Check if include directory is a subdirectory of the source directory
+        string(REGEX MATCH "^${CMAKE_SOURCE_DIR}/.*" _is_subdirectory ${_include_dir})
+        # Check if include directory is part of a generator expression (e.g. $<BUILD_INTERFACE:...>)
+        string(REGEX MATCH "^\\$<.*:${CMAKE_SOURCE_DIR}/.*>$" _is_genexp_subdirectory "${_include_dir}")


The CMAKE_SOURCE_DIR might contain characters which have special meaning in a regex. Unfortunately afaik there is no function in CMake to escape a string for that use case (see https://gitlab.kitware.com/cmake/cmake/issues/18409).

I think the real world impact of this issue is narrowly scoped enough that the TODO in 600fa4c covers it.

A robust string library could probably get around the need for regexp here. Comparing the initia substring of the include directory to the CMAKE_SOURCE_DIR might be sufficient in the usual case but I don't have any idea yet what to do about the Generator Expressions.

CMake does not allow getting the INCLUDE_DIRECTORIES property from INTERFACE libraries. Instead, first check if the property exists, if it does not then try to get the INTERFACE_INCLUDE_DIRECTORIES property. Note, if INTERFACE_INCLUDE_DIRECTORIES is not defined an empty list is returned, but we cannot assume the target is not an interface. This is why the implementation is conditional on INCLUDE_DIRECTORIES instead. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron · 2019-01-11T20:15:56Z

Looks like a regression, the errors have returned for cppcheck v1.86...

jacobperron · 2019-01-11T21:21:05Z

For some reason ${_include_dirs_defined} is always 0 (false), even though if the property is requested it works as expected:

ament_lint/ament_cmake_cppcheck/cmake/ament_cmake_cppcheck_lint_hook.cmake

Lines 37 to 41 in 63fc82d

    
           get_property(_include_dirs_defined 
        
             TARGET ${_target} 
        
             PROPERTY INCLUDE_DIRECTORIES 
        
             DEFINED 
        
           )

jacobperron · 2019-01-11T21:39:44Z

Through some experimentation, looks like we can use the TYPE property to check if a target is an INTERFACE_LIBRARY. Update to condition incoming...

…roperty to use Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron · 2019-01-11T21:46:51Z

Windows (cppcheck v1.86)

jacobperron · 2019-01-11T21:50:17Z

Test rosbag2_test_common:

Linux:
Windows:

stale

nuclearsandwich · 2019-01-11T22:50:24Z

ament_cmake_cppcheck/cmake/ament_cmake_cppcheck_lint_hook.cmake

-        PROPERTY INCLUDE_DIRECTORIES
-      )
+      # Include directories property is different for INTERFACE libraries
+      get_target_property(_target_type ${_target} TYPE)


🎉 Glad you found a way to do this.

nuclearsandwich

👍 with passing CI for both default and the rosbag2_test_common package.

jacobperron · 2019-01-12T00:03:26Z

For good measure:

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron · 2019-01-12T06:04:55Z

Windows:

jacobperron added the in progress Actively being worked on (Kanban column) label Jan 11, 2019

jacobperron added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jan 11, 2019

dirk-thomas reviewed Jan 11, 2019

View reviewed changes

clalancette mentioned this pull request Jan 11, 2019

ROS 2 Crystal Clemmys Patch Release 1 ros2/ros2#638

Closed

dirk-thomas reviewed Jan 11, 2019

View reviewed changes

jacobperron force-pushed the fix_120 branch from 600fa4c to 63fc82d Compare January 11, 2019 20:06

dirk-thomas approved these changes Jan 11, 2019

View reviewed changes

nuclearsandwich previously approved these changes Jan 11, 2019

View reviewed changes

jacobperron force-pushed the fix_120 branch from 4af33e8 to 15e68d4 Compare January 11, 2019 21:41

Use target type property as a condition on what include directories p…

434dadc

…roperty to use Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron force-pushed the fix_120 branch from 15e68d4 to 434dadc Compare January 11, 2019 21:43

nuclearsandwich reviewed Jan 11, 2019

View reviewed changes

nuclearsandwich approved these changes Jan 11, 2019

View reviewed changes

Increase cppcheck test timeout to 120s

bcca820

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron force-pushed the fix_120 branch from 6cdc701 to bcca820 Compare January 12, 2019 04:53

dirk-thomas approved these changes Jan 12, 2019

View reviewed changes

jacobperron merged commit aa51cab into master Jan 12, 2019

jacobperron deleted the fix_120 branch January 12, 2019 06:40

jacobperron removed the in review Waiting for review (Kanban column) label Jan 12, 2019

Conversation

jacobperron commented Jan 11, 2019

Uh oh!

jacobperron commented Jan 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacobperron commented Jan 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nuclearsandwich commented Jan 11, 2019

Uh oh!

jacobperron commented Jan 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacobperron commented Jan 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacobperron commented Jan 11, 2019

Uh oh!

jacobperron commented Jan 11, 2019

Uh oh!

jacobperron commented Jan 11, 2019

Uh oh!

jacobperron commented Jan 11, 2019

Uh oh!

jacobperron commented Jan 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nuclearsandwich left a comment

Choose a reason for hiding this comment

Uh oh!

jacobperron commented Jan 12, 2019

Uh oh!

jacobperron commented Jan 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jacobperron commented Jan 11, 2019 •

edited

Loading

jacobperron commented Jan 11, 2019 •

edited

Loading