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

Fix Eigen Plugins compilation when linking to grid_map_core #475

Draft
wants to merge 3 commits into
base: rolling
Choose a base branch
from

Conversation

Ryanf55
Copy link
Collaborator

@Ryanf55 Ryanf55 commented Jul 31, 2024

Purpose

Here's a proposed patch for #382. Because the original PR for adding the plugins through CMake and ament is not well documented, I really don't know how this will affect others. Please try this PR out. It will stay open for a while until I get a reasonable number of approvals it doesn't break anything.

Test instructions

  • Check this branch out
  • colcon build --packages-up-to issue382

And try building your own packages that link to grid_map_core.

@@ -5,8 +5,7 @@ project(grid_map_core)
find_package(ament_cmake REQUIRED)
find_package(grid_map_cmake_helpers REQUIRED)

## Define Eigen addons.
include(cmake/${PROJECT_NAME}-extras.cmake)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This needs to be moved down till after the library is created. You can't call target_compile_definitions on a target until the target exists.

)
ament_package()
#ament_package(CONFIG_EXTRAS
# cmake/${PROJECT_NAME}-extras.cmake
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This breaks if we leave it in - any tips on how to test the plugins are working in another package?

@nhewitt99
Copy link

Just adding my $0.02 that this PR does fix the Eigen linker issues when using grid_map_core for me, thank you! I can't speak to its potential impact on other plugins, though.

@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Aug 22, 2024

Just adding my $0.02 that this PR does fix the Eigen linker issues when using grid_map_core for me, thank you! I can't speak to its potential impact on other plugins, though.

Thanks for sharing!

@StefanFabian
Copy link
Contributor

My $0.02: This is in line with what I proposed. I would only move the code from the extas file to the main cmakelists and remove the extras file.
If it is not installed, there's no reason to make it an extra file, and with the naming, it could be confusing.

Generally, setting these Eigen compile time variables in the public interface of a library is IMHO bad style as it limits or at least makes it a lot more complicated for people to add their own extensions and makes it incompatible with any other library that does the same (of which I hope there are none but still).
If it's only used in the cpp files, you could set them as PRIVATE and retain the functionality without leaking into downstream packages.

@doisyg
Copy link

doisyg commented Jan 9, 2025

Thanks for this! I am not sure about the proper solution but this is what I have been end up using to be able to build and use properly grid_map_ros in external packages (in Jazzy):

botsandus@3ce1c30

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants