-
Notifications
You must be signed in to change notification settings - Fork 123
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
Fixing issue with Eigen in Ubuntu Jammy on ARM #378
Merged
Merged
Changes from 2 commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 is actually a bit trickier than I thought.
This package itself has no direct dependency on Eigen, but it includes files that eventually include Eigen, and so we end up with the same build error.
However, because we don't directly depend on Eigen here, the variable
Eigen3_VERSION
isn't even declared, so we'll never enter this block.Does anyone know if there's a way for packages that we include to export their build settings so that downstream packages pick them up? Ideally, any package that depends on, e.g.,
fuse_core
would automatically get the-Wno-class-memaccess
flag.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.
https://docs.ros.org/en/api/catkin/html/dev_guide/generated_cmake_api.html#catkin_package
In the docs for the
catkin_package()
command:Based on that description, I would have expected dependent packages would have had access to the
Eigen3_FOUND
and similar variables after callingfind_package(catkin REQUIRED COMPONENTS fuse_core)
.But it does say the string case used in
catkin_package(DEPENDS)
must match the case of the CMake variables. Infuse_core
I have a depend onEIGEN3
all caps:https://github.com/locusrobotics/fuse/blob/devel/fuse_core/CMakeLists.txt#L29
Is that what the problem is? Just a case mismatch between
fuse_core
and this new if-block?Barring that, using the
CFG_EXTRAS
parameter might be another way to make this work. Iffuse_core
exports some build flags usingCFG_EXTRAS
, dependent packages should pick that up, right? If their CMake files are written correctly. I think.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.
No, the all-caps version is empty even in packages that use Eigen directly. I had to print out all the available variables to figure that one out.
I'll try your other suggestion, thanks!
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.
So maybe the
fuse_core
package is wrong, and it should beEigen3
instead ofEIGEN3
?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
fuse_core
is exporting the correct dependency:https://github.com/ros/geometry2/blob/6df9e94363061a24ba890a6ff29771a96b0d756b/tf2_eigen/CMakeLists.txt#L34
https://github.com/cra-ros-pkg/robot_localization/blob/af30f3bc0592d84fcc06bdbdf6063c126f834493/CMakeLists.txt#L43
However, the
rolling
branch of this package uses this when exportingament
dependencies:fuse/fuse_core/CMakeLists.txt
Line 118 in ff41ec0