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

Bugfix use old style include directories for filters #442

Merged
merged 2 commits into from
Feb 18, 2024

Conversation

Ryanf55
Copy link
Collaborator

@Ryanf55 Ryanf55 commented Feb 17, 2024

Purpose

Use old-style CMake variables as a patch till this upstream issue makes it into humble binaries: ros/filters#70

Cause

https://github.com/ANYbotics/grid_map/pull/404/files#diff-217de7f0d7eec88dce2cada8716bd30748cf9ad20d1211c56501a2acb56666bbL28

This PR forgot to add the include directories from filters, probably because there were no targets. I'm not sure how it builds on rolling and not humble. This patch can be replaced with a proper target_link_libraries call soon.

Effect

It's blocking CI from passing in this PR: https://github.com/ANYbotics/grid_map/actions/runs/7943206475/job/21687293385?pr=440#step:6:25

Upstream variables

$ cat /opt/ros/rolling/share/filters/cmake/ament_cmake_export_include_directories-extras.cmake 
# generated from ament_cmake_export_include_directories/cmake/ament_cmake_export_include_directories-extras.cmake.in

set(_exported_include_dirs "${filters_DIR}/../../../include")

# append include directories to filters_INCLUDE_DIRS
# warn about not existing paths
if(NOT _exported_include_dirs STREQUAL "")
  find_package(ament_cmake_core QUIET REQUIRED)
  foreach(_exported_include_dir ${_exported_include_dirs})
    if(NOT IS_DIRECTORY "${_exported_include_dir}")
      message(WARNING "Package 'filters' exports the include directory '${_exported_include_dir}' which doesn't exist")
    endif()
    normalize_path(_exported_include_dir "${_exported_include_dir}")
    list(APPEND filters_INCLUDE_DIRS "${_exported_include_dir}")
  endforeach()
endif()

You can see the variable we need to use is filters_INCLUDE_DIRS

Copy link

@srmainwaring srmainwaring left a comment

Choose a reason for hiding this comment

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

LGTM. Verified on macOS.

@Ryanf55 Ryanf55 merged commit e332abb into ANYbotics:rolling Feb 18, 2024
2 checks passed
@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Feb 18, 2024

@Mergifyio backport iron

Copy link

mergify bot commented Feb 18, 2024

backport iron

✅ Backports have been created

@Ryanf55 Ryanf55 deleted the bugfix-use-old-style-for-filters branch February 18, 2024 18:47
@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Feb 18, 2024

@Mergifyio backport humble

Copy link

mergify bot commented Feb 18, 2024

backport humble

✅ Backports have been created

Ryanf55 added a commit that referenced this pull request Feb 18, 2024
Bugfix use old style include directories for filters (backport #442)
Ryanf55 added a commit that referenced this pull request Feb 18, 2024
Bugfix use old style include directories for filters (backport #442)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants