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

forward declare fromMsg to avoid missing symbols in downstream libraries #485

Merged
merged 2 commits into from
Mar 21, 2022

Conversation

jcmonteiro
Copy link
Contributor

@jcmonteiro jcmonteiro commented Dec 10, 2021

This PR fixes rst-tu-dortmund/teb_local_planner#334 and mitigates #242 while a proper solution addressing fromMsg and other template prototype functions (e.g., #427) is not provided.

Why

Without the forward declaration, the symbol will be undefined in a target that depends on tf2 and tf2_geometry_msgs and includes first tf2/utils.h and later tf2_geometry_msgs/tf2_geometry_msgs.hpp since there are inlined functions in tf2/impl/utils.h that make use of fromMsg with the quaternions as input types.

Disclaimer

This solution is avoiding the deeper issue where fromMsg and other template functions are not being specialized. If, for example, fromMsg had been properly specialized in tf2_geometry_msgs.hpp for the types geometry_msgs::msg::Quaternion and tf2::Quaternion, the forward declaration here would be

template<>
void fromMsg(const geometry_msgs::msg::Quaternion & in, tf2::Quaternion & out);

Example

One can validate this solution by building a simple library

// include/library/library.hpp
#pragma once
namespace library {
void function();
}
// src/library.cpp
#include "library/library.hpp"
#include <tf2_geometry_msgs/tf2_geometry_msgs.hpp>
#include <tf2/utils.h>

void library::function()
{
  geometry_msgs::msg::Quaternion quat;
  tf2::impl::toQuaternion(quat);
}
...
add_library(library SHARED src/library.cpp)
...

changing the order of the includes and checking the symbols in liblibrary.so with

nm --demangle <path-to-workspace>/build/library/liblibrary.so | grep fromMsg

When checking the symbol, one should notice that

  • when the symbol is undefined it refers to the template function tf2::fromMsg<...>(...),
  • otherwise, it refers to the non-template function tf2::fromMsg(...) which is not a specialization of the template function.

Copy link
Member

@aprotyas aprotyas left a comment

Choose a reason for hiding this comment

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

I think #427 will take a while to land, so this can be merged for now.

@aprotyas
Copy link
Member

aprotyas commented Dec 21, 2021

CI:
Repos file: https://gist.githubusercontent.com/aprotyas/c86851dae9847cc1bcf70351926c5a37/raw/94dd686808cad6b23a27280cb26629b261426080/ros2.repos
Build args: --packages-above-and-dependencies tf2
Test args: --packages-above tf2
Job: https://ci.ros2.org/job/ci_launcher/9535/

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

Note: the failing uncrustify tests were addressed in #486.

@guilyx
Copy link
Contributor

guilyx commented Jan 10, 2022

Hey @aprotyas, any update on this ?

@aprotyas
Copy link
Member

aprotyas commented Jan 10, 2022

It's been a while and the failing tests have been addressed, so I'll re-run CI:

Repos file: https://gist.githubusercontent.com/aprotyas/c86851dae9847cc1bcf70351926c5a37/raw/94dd686808cad6b23a27280cb26629b261426080/ros2.repos
Build args: --packages-above-and-dependencies tf2
Test args: --packages-above tf2
Job: https://ci.ros2.org/job/ci_launcher/9629/

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

@aprotyas
Copy link
Member

@ahcorde @clalancette can either of you take a look please?

@aprotyas
Copy link
Member

@jcmonteiro can you rebase your branch on the ros2 branch? That way de1753c will be tested as well, addressing the uncrustify failures.

@aprotyas
Copy link
Member

Re-running CI following the rebase:

Repos file: https://gist.githubusercontent.com/aprotyas/c86851dae9847cc1bcf70351926c5a37/raw/94dd686808cad6b23a27280cb26629b261426080/ros2.repos
Build args: --packages-above-and-dependencies tf2
Test args: --packages-above tf2
Job: https://ci.ros2.org/job/ci_launcher/9662/

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

@guilyx
Copy link
Contributor

guilyx commented Feb 25, 2022

@aprotyas hey, any ETA on this ?

@aprotyas
Copy link
Member

I left this hanging last time, my bad. Okay, @jcmonteiro last rebase request and then I'll run CI.

@aprotyas
Copy link
Member

Running CI:

Repos file: https://gist.githubusercontent.com/aprotyas/c86851dae9847cc1bcf70351926c5a37/raw/94dd686808cad6b23a27280cb26629b261426080/ros2.repos
Build args: --packages-above-and-dependencies tf2
Test args: --packages-above tf2
Job: https://ci.ros2.org/job/ci_launcher/10016/

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

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.

[Linking Bug - Rolling] tf2 geometry msgs converters
5 participants