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

Use z_check and z_drop instead of zc_liveliness_XX #69

Merged
merged 6 commits into from
Nov 23, 2023

Conversation

JEnoch
Copy link
Contributor

@JEnoch JEnoch commented Nov 20, 2023

Following eclipse-zenoh/zenoh-c#195 the workaround described here can be rolled back.

The API was indeed missing to call z_check() on a zc_owned_liveliness_token_t.
The undefined symbol error calling z_drop() was probably an build environment issue.

Copy link
Member

@Yadunund Yadunund left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the macros @JEnoch! I think there might have been a search + replace error which added an extra ( after z_drop.

rmw_zenoh_cpp/src/rmw_zenoh.cpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/rmw_zenoh.cpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/rmw_zenoh.cpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/rmw_zenoh.cpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/rmw_zenoh.cpp Outdated Show resolved Hide resolved
JEnoch and others added 5 commits November 20, 2023 16:13
Co-authored-by: Yadu <[email protected]>
Signed-off-by: Julien Enoch <[email protected]>
Co-authored-by: Yadu <[email protected]>
Signed-off-by: Julien Enoch <[email protected]>
Co-authored-by: Yadu <[email protected]>
Signed-off-by: Julien Enoch <[email protected]>
Co-authored-by: Yadu <[email protected]>
Signed-off-by: Julien Enoch <[email protected]>
Co-authored-by: Yadu <[email protected]>
Signed-off-by: Julien Enoch <[email protected]>
@JEnoch
Copy link
Contributor Author

JEnoch commented Nov 20, 2023

Oups! Sorry about that... Fixed!

@JEnoch JEnoch requested a review from Yadunund November 20, 2023 15:14
@Yadunund
Copy link
Member

I'm still seeing the same undefined symbol error at runtime

/opt/ros/iron/lib/demo_nodes_cpp/listener: symbol lookup error: /home/yadunund/ws_rmw/install/rmw_zenoh_cpp/lib/librmw_zenoh_cpp.so: undefined symbol: _Z6z_dropI27zc_owned_liveliness_token_tEN15zenoh_drop_typeIT_E4typeEPS2_

In fact, on MacOS i'm unable to even compile with apple clang + ld linker

ld: Undefined symbols:
  zenoh_drop_type<zc_owned_liveliness_token_t>::type z_drop<zc_owned_liveliness_token_t>(zc_owned_liveliness_token_t*), referenced from:
      rcpputils::scope_exit<rmw_create_node::$_4>::~scope_exit() in rmw_zenoh.cpp.o
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [librmw_zenoh_cpp.dylib] Error 1
make[1]: *** [CMakeFiles/rmw_zenoh_cpp.dir/all] Error 2
make: *** [all] Error 2
make: INTERNAL: Exiting with 11 jobserver tokens available; should be 10!
---
Failed   <<< rmw_zenoh_cpp [13.8s, exited with code 2]

@clalancette
Copy link
Collaborator

/opt/ros/iron/lib/demo_nodes_cpp/listener: symbol lookup error: /home/yadunund/ws_rmw/install/rmw_zenoh_cpp/lib/librmw_zenoh_cpp.so: undefined symbol: Z6z_dropI27zc_owned_liveliness_token_tEN15zenoh_drop_typeIT_E4typeEPS2

Yeah, I'm seeing this as well with this PR in place. Not sure what is going on there.

@JEnoch
Copy link
Contributor Author

JEnoch commented Nov 20, 2023

Weird! The builds with both iron-ros-base and rolling-ros-base Docker images are passing here in Actions, as well as on my Ubuntu 22.04/Rolling VM.
Could it be related to compiler/linker version ?
Mine are gcc version 11.4.0 (Ubuntu 11.4.0-1ubuntu1~22.04) and GNU ld (GNU Binutils for Ubuntu) 2.38

@Yadunund
Copy link
Member

Weird! The builds with both iron-ros-base and rolling-ros-base Docker images are passing here in Actions, as well as on my Ubuntu 22.04/Rolling VM. Could it be related to compiler/linker version ? Mine are gcc version 11.4.0 (Ubuntu 11.4.0-1ubuntu1~22.04) and GNU ld (GNU Binutils for Ubuntu) 2.38

To clarify, on Ubuntu 22.04 + Iron/Rolling, the packages build without any errors. But the linker error is thrown when the z_drop() is called on the liveliness token when we cleanup the node.
To reproduce this,

cd ~/ws_rmw
source install/setup.bash
export RMW_IMPLEMENTATION=rmw_zenoh_cpp
ros2 run demo_nodes_cpp listener

## Then terminal the node with Ctrl+C

/opt/ros/iron/lib/demo_nodes_cpp/listener: symbol lookup error: /home/yadunund/ws_rmw/install/rmw_zenoh_cpp/lib/librmw_zenoh_cpp.so: undefined symbol: _Z6z_dropI27zc_owned_liveliness_token_tEN15zenoh_drop_typeIT_E4typeEPS2_

@JEnoch
Copy link
Contributor Author

JEnoch commented Nov 22, 2023

Oh! OK, thanks for the clarification.
I managed to reproduce, and also saw the issue has actually been fixed in zenoh-c by eclipse-zenoh/zenoh-c#197.
The error disappeared for me after updating zenoh-c.

Copy link
Member

@Yadunund Yadunund left a comment

Choose a reason for hiding this comment

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

Tested with the latest zenoh-c and works great! Thanks for following up!

@Yadunund Yadunund merged commit 8569898 into ros2:rolling Nov 23, 2023
5 checks passed
Yadunund added a commit that referenced this pull request Jan 12, 2024
* Use z_check and z_drop instead of zc_liveliness_XX

* Update rmw_zenoh_cpp/src/rmw_zenoh.cpp

Co-authored-by: Yadu <[email protected]>
Signed-off-by: Julien Enoch <[email protected]>

* Update rmw_zenoh_cpp/src/rmw_zenoh.cpp

Co-authored-by: Yadu <[email protected]>
Signed-off-by: Julien Enoch <[email protected]>

* Update rmw_zenoh_cpp/src/rmw_zenoh.cpp

Co-authored-by: Yadu <[email protected]>
Signed-off-by: Julien Enoch <[email protected]>

* Update rmw_zenoh_cpp/src/rmw_zenoh.cpp

Co-authored-by: Yadu <[email protected]>
Signed-off-by: Julien Enoch <[email protected]>

* Update rmw_zenoh_cpp/src/rmw_zenoh.cpp

Co-authored-by: Yadu <[email protected]>
Signed-off-by: Julien Enoch <[email protected]>

---------

Signed-off-by: Julien Enoch <[email protected]>
Co-authored-by: Yadu <[email protected]>
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.

3 participants