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

Fixing issue with Eigen in Ubuntu Jammy on ARM #378

Merged
merged 3 commits into from
Sep 19, 2024
Merged

Conversation

ayrton04
Copy link
Contributor

On ARM, the use of Eigen 3.4 with g++11 (both the versions in Jammy) causes a warning, which in turn causes the build to fail. Details are here: https://gitlab.com/libeigen/eigen/-/issues/2326. We can suppress the warning for ARM builds by checking for the specific versions of the affected packages and architecture.

If anyone has a more reliable (yet equally succinct) way of testing for ARM, please fill me in.

@ayrton04 ayrton04 requested a review from svwilliams August 27, 2024 12:54
While Eigen isn't included directly here, it is included when we include some headers from fuse_core.
# On ARM, the use of Eigen 3.4 with g++11 (both the versions in Jammy) causes a warning, which in turn causes
# the build to fail. Details are here: https://gitlab.com/libeigen/eigen/-/issues/2326. We can suppress the warning
# for ARM builds here.
if(Eigen3_VERSION VERSION_EQUAL 3.4 AND
Copy link
Contributor Author

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.

Copy link
Contributor

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:

DEPENDS (list of strings) – a list of CMake projects which this project depends on. Since they might not be find_packagable or lack a pkg-config file their INCLUDE_DIRS and LIBRARIES are passed directly. This requires that it has been find_package-ed before and all variables (_FOUND, _INCLUDE_DIRS, etc.) have the same case as this argument.

Based on that description, I would have expected dependent packages would have had access to the Eigen3_FOUND and similar variables after calling find_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. In fuse_core I have a depend on EIGEN3 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. If fuse_core exports some build flags using CFG_EXTRAS, dependent packages should pick that up, right? If their CMake files are written correctly. I think.

Copy link
Contributor Author

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!

Copy link
Contributor

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 be Eigen3 instead of EIGEN3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@svwilliams svwilliams left a comment

Choose a reason for hiding this comment

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

Are you still working out the details?

@ayrton04
Copy link
Contributor Author

Yes, sorry. Haven't had the cycles to look at this. Will pick it back up.

@ayrton04
Copy link
Contributor Author

@svwilliams are you still OK with this?

@svwilliams
Copy link
Contributor

Yep. Do you need the same thing in one of the ROS2 branches as well?

@ayrton04
Copy link
Contributor Author

Probably, but I haven't tried building it in ROS 2 on ARM/Jammy yet. Still, there's nothing in this change that's tied to any version of ROS, so I can add another branch for that.

@ayrton04 ayrton04 merged commit dae8521 into devel Sep 19, 2024
5 checks passed
@ayrton04 ayrton04 deleted the fix-jammy-arm-builds branch September 19, 2024 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants