-
Notifications
You must be signed in to change notification settings - Fork 107
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
Account for INTERFACE libraries when getting target include directories #121
Conversation
FYI @Karsten1987 |
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}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be a way to do it 0472892
Open to suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the returned include dirs always native paths?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed on a Windows machine that include dirs are cmake paths. I’ll remove the native path command.
@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. |
# 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}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
Looks like a regression, the errors have returned for cppcheck v1.86... |
For some reason ament_lint/ament_cmake_cppcheck/cmake/ament_cmake_cppcheck_lint_hook.cmake Lines 37 to 41 in 63fc82d
|
Through some experimentation, looks like we can use the |
…roperty to use Signed-off-by: Jacob Perron <jacob@openrobotics.org>
PROPERTY INCLUDE_DIRECTORIES | ||
) | ||
# Include directories property is different for INTERFACE libraries | ||
get_target_property(_target_type ${_target} TYPE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 Glad you found a way to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 with passing CI for both default and the rosbag2_test_common package.
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
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.