Skip to content

Fix bug when packages with have multiple imported configurations#318

Merged
sloretz merged 1 commit intomasterfrom
ament_cmake__get_recursive_properties_for_all_configurations
Aug 3, 2021
Merged

Fix bug when packages with have multiple imported configurations#318
sloretz merged 1 commit intomasterfrom
ament_cmake__get_recursive_properties_for_all_configurations

Conversation

@sloretz
Copy link
Copy Markdown
Contributor

@sloretz sloretz commented Feb 12, 2021

Some packages have multiple configurations, but this code was assuming
there would only be one. This results in a CMake error about and
incorrect number of arguments passed to get_target_property().

This fixes it by grabbing data from all configurations.

From --trace-expand output

get_target_property(imported_implib rcpputils::rcpputils IMPORTED_IMPLIB_DEBUG;NOCONFIG )
CMake Error at :58 (get_target_property):
  :47 (ament_get_recursive_properties)
  src/qt_gui_cpp_shiboken/CMakeLists.txt:39 (ament_get_recursive_properties)

Signed-off-by: Shane Loretz sloretz@openrobotics.org
Signed-off-by: Shane Loretz sloretz@osrfoundation.org

Some packages have multiple configurations, but this code was assuming
there would only be one. This results in a CMake error about and
incorrect number of arguments passed to get_target_property().

From `--trace-expand` output

```
get_target_property(imported_implib rcpputils::rcpputils IMPORTED_IMPLIB_DEBUG;NOCONFIG )
CMake Error at :58 (get_target_property):
  :47 (ament_get_recursive_properties)
  src/qt_gui_cpp_shiboken/CMakeLists.txt:39 (ament_get_recursive_properties)
```

Signed-off-by: Shane Loretz <sloretz@openrobotics.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Feb 12, 2021

Edit oops, closed wrong PR

Ah oops, already fixed in ros-visualization/qt_gui_core#239

Guess I need to update my branches to galactic-devel instead

@sloretz sloretz closed this Feb 12, 2021
@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Feb 12, 2021

reopening because I closed the wrong PR :(

@sloretz sloretz reopened this Feb 12, 2021
Copy link
Copy Markdown

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me, pending green CI

@hidmic
Copy link
Copy Markdown

hidmic commented Jul 23, 2021

@sloretz do you still want to merge this PR?

@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Jul 26, 2021

CI (build: --packages-above-and-dependencies ament_cmake_target_dependencies test: --packages-above ament_cmake_target_dependencies)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@hidmic
Copy link
Copy Markdown

hidmic commented Aug 3, 2021

That Windows warning seems unrelated. I think this is ready to go @sloretz.

@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Aug 3, 2021

Ah, thanks! I forgot about this one.

@sloretz sloretz merged commit 5a5b57c into master Aug 3, 2021
@delete-merged-branch delete-merged-branch bot deleted the ament_cmake__get_recursive_properties_for_all_configurations branch August 3, 2021 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants