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

Replace custom router with upstream zenohd #262

Open
nkoenig opened this issue Aug 9, 2024 · 11 comments
Open

Replace custom router with upstream zenohd #262

nkoenig opened this issue Aug 9, 2024 · 11 comments
Assignees

Comments

@nkoenig
Copy link
Contributor

nkoenig commented Aug 9, 2024

We'd like to remove the custom router in this repo with the upstream zenohd router.

Context from #261:

Ah, cool. Here is some context. Back in late 2023, we actually didn't have a custom router, and we were just using the router as provided by the upstream Zenoh project. The major problem with that was that it required us to build Zenoh twice: once for building the zenoh-c bindings, and once so we got access to the router. Since building Zenoh is pretty slow, this was less than ideal. So we introduced the custom router here. But it isn't ideal, because while it supports most of the features of the zenohd router, it doesn't support all of them. And that leads to confusion.

My best idea on how to fix this is to change https://github.com/eclipse-zenoh/zenoh-c so that it can (optionally) also build the router. Then in the vendor package here, we would turn on that option, and also install the router in the correct place so that ros2 run could find it. There may be other solutions, but that was the one I came up with.

Does that make sense?

Originally posted by @clalancette in #261 (comment)

@Timple
Copy link
Contributor

Timple commented Aug 9, 2024

Does the original router suffer from the same keyboard capture issues?

This might come across a bit lazy, but there's good traction going on! I won't have access to a computer soon however

@nkoenig
Copy link
Contributor Author

nkoenig commented Aug 9, 2024

I'm in the process of wrapping my head around the code and requirements.

  1. I believe this is the upstream zenohd router.
  2. I think the CMakeLists.txt in zenoh-c needs to be modified to support (optionally) building the zenohd router?
  3. If Implement Zenoh ament #2 is correct, then we'd have to modify the zenoh_c_vendor CMakeLists.txt to pass in the correct build option.
  4. Finally, install the router using the regular ROS approach so that ros2 run can run the router.

Before going down this route, I just want to make sure this is compatible with @codebot suggestion of making something like a ros-rolling-zenohd-autostart package. See #231 (comment).

@nkoenig
Copy link
Contributor Author

nkoenig commented Aug 9, 2024

Does the original router suffer from the same keyboard capture issues?

I don't believe so. The upstream zenohd router doesn't seem to have a similar keyboard/console interface. https://github.com/eclipse-zenoh/zenoh/blob/main/zenohd/src/main.rs. It's my first time looking at rust code though.

@clalancette
Copy link
Collaborator

  1. I believe this is the upstream zenohd router.
  2. I think the CMakeLists.txt in zenoh-c needs to be modified to support (optionally) building the zenohd router?
  3. If Implement Zenoh ament #2 is correct, then we'd have to modify the zenoh_c_vendor CMakeLists.txt to pass in the correct build option.
  4. Finally, install the router using the regular ROS approach so that ros2 run can run the router.

Yep, this is exactly what I was thinking of.

Before going down this route, I just want to make sure this is compatible with @codebot suggestion of making something like a ros-rolling-zenohd-autostart package. See #231 (comment).

So, while I love the idea that @codebot suggested, further conversation has turned up a problem with it. The basic problem is that packages like ros-rolling-foo are, by design, installed in /opt/ros/rolling. But a package like ros-rolling-zenohd-autostart would install into /etc/systemd. What happens when you have both ros-rolling-zenohd-autostart and ros-jazzy-zenohd-autostart installed? Most package managers won't allow two different packages to "own" the same file, and even if they did, which one would "win" in the case that they were different? Put another way, on startup, would systemd choose to start up the Rolling version of zenohd, or the Jazzy one?

All of that said, I don't think the answer to those questions affects this too much. Whether we are attempting to launch the custom version of zenohd, or the upstream version of zenohd, we still have the same problem.

@codebot
Copy link
Member

codebot commented Aug 9, 2024

I haven't done it myself, so I don't speak with great confidence, but I think both apt and systemd have a way of defining conflicting packages/subsystems, using a notion of exclusivity or uniqueness of some sort. I think we'd need to have the ros-FOO-zenohd-autostart package be not automatically included in ros-FOO-desktop and people would need to choose when either creating (or copy-pasting) their apt install command line, between installing ros-rolling-zenohd-autostart and ros-jazzy-zenohd-autostart. Hopefully if an impossible collection of packages is requested, the apt error messages would be reasonable to explain that both of the *-autostart packages cannot be simultaneously installed.

@nkoenig
Copy link
Contributor Author

nkoenig commented Aug 15, 2024

Right, debian supports the notion of conflicting packages, like this. I'm not as familiar with systemd.

@nkoenig
Copy link
Contributor Author

nkoenig commented Aug 16, 2024

I started to work on patching zenoh-c to support compilation of the zenohd router. I'm at the point where I could use some guidance. The approach I'm taking seems too hacky, and I started to have some questions.

Questions

  1. Is modification of the zenoh-c to support generation of the zenohd router the right way to go? For example, it seems a bit odd to create the zenohd executable in a repo that's designed for the zenoh C api interfaces.
    a. I understand the desire to reduce compile time. Compiling the zenoh takes a bit over 1 min. Is 2-3 min compile time (once for zenoh-c and once for zenohd) that bad?
  2. There is a debian install of the zenohd router. Why don't we just use that?

My AWESOME approach
Here is the latest diff of my approach.

It's using ExternalProject_Add, instead of Cargo because I couldn't find a way to compile zenohd from an external project via Cargo. I'm also not experienced with Cargo.

This will successfully build and install zenoh-d and the necessary plugins, but...

This approach will require more adjustments to handle windows and mac (mostly when it comes to installing the files).

Finally, there are two plugins required by zenohd, libzenoh_plugin_rest and libzenoh_plugin_storage_manager. This has escalated to into facilitating finding these plugins at runtime. Since we'd be installing these plugins in a non-standard place, then we'd have to start using a zenohd configuration system like what's found here. Ultimately, you couldn't just ros2 run zenohd. You'd have to pass in some configuration.

So...am I going down the right path here? It seems like we should create a new repo/package that is just dedicated toward building, installing, and configuring the zenohd router for use in ROS2 or use the upstream zenohd router.

@clalancette
Copy link
Collaborator

1. Is modification of the `zenoh-c` to support generation of the `zenohd` router the right way to go?

Modification of zenoh-c is right, but I don't believe what you currently have is the right direction. In particular, we shouldn't need to use ExternalProject_add there; that project is already pulling in the Zenoh sources to be able to compile Zenoh-C. What I think we need to do is to (somehow) pass through a feature flag to Cargo to tell it to build the router along with everything else.

  • There is a debian install of the zenohd router. Why don't we just use that?

The main reason is that we have a policy against using third-party APT repositories when delivering ROS binaries. So while that might be convenient now, it means we wouldn't be able to release this onto the ROS buildfarm later.

a. I understand the desire to reduce compile time. Compiling the zenoh takes a bit over 1 min. Is 2-3 min compile time (once for zenoh-c and once for zenohd) that bad?

When we were doing this late last year, it was far longer than that. Maybe something has changed in the meantime (or you just have a faster machine). Still, like with all projects, compile times on Zenoh are only going to go up, so I think we should find a way to only compile it once.

@JEnoch
Copy link
Contributor

JEnoch commented Aug 22, 2024

Modification of zenoh-c is right, but I don't believe what you currently have is the right direction. In particular, we shouldn't need to use ExternalProject_add there; that project is already pulling in the Zenoh sources to be able to compile Zenoh-C. What I think we need to do is to (somehow) pass through a feature flag to Cargo to tell it to build the router along with everything else.

I gave a try to this. The Cargo way would be to add in zenoh-c Cargo.toml optional dependencies to zenohd and possibly it's plugins (zenoh-plugin-restand zenoh-plugin-storages), which are binaries artifacts (binary executable and shared libraries).
Unfortunately artifact dependencies is currently an unstable feature requiring a specific -Z bindeps flag of cargo. Such flags are only available in nightly Rust/cargo versions, not in 1.75.

@JEnoch
Copy link
Contributor

JEnoch commented Aug 22, 2024

Another way to build zenohd from the sources already pulled by cargo is the following:

  1. Get the commit id of zenoh dependency:
    cargo tree --manifest-path ./build/zenoh_c_vendor/zenoh_c_vendor-prefix/src/zenoh_c_vendor/Cargo.toml -i zenoh -f "{p} {f}" --depth 0
    This gives such a result with github URL:
    zenoh v0.11.0-dev (https://github.com/eclipse-zenoh/zenoh.git?branch=main#93f93d2d) auth_pubkey,auth_usrpwd,default,shared-memory,transport_compression,transport_multilink,transport_quic,transport_tcp,transport_tls,transport_udp,transport_unixsock-stream,transport_ws,unstable,zenoh-shm
    
  2. Make cargo to build and install zenohd using the same URL that have already been pulled by cargo when building zenoh-c into a ~/.cargo/git/checkouts/zenoh-<some_hash>/<commit_id>/ directory:
    cargo install --target-dir ./build/zenoh_c_vendor/zenoh_c_vendor-prefix/src/zenoh_c_vendor-build/release/target --root ./build/zenoh_c_vendor/ --git https://github.com/eclipse-zenoh/zenoh.git?branch=main#93f93d2d zenohd
    zenohd binary will be installed in ./build/zenoh_c_vendor/bin/

I set the --target-dir option to the build directory of zenoh-c with the hope that cargo re-use some of the intermediate artifacts built with zenoh-c. However I'm not sure it's working considering the amount of dependencies it seems to re-build for zenohd...

Also, I can't find a similar way to build the Zenoh plugins which are shared libraries. But they are not required for rmw_zenoh.

@nkoenig
Copy link
Contributor Author

nkoenig commented Aug 22, 2024

When we were doing this late last year, it was far longer than that. Maybe something has changed in the meantime (or you just have a faster machine). Still, like with all projects, compile times on Zenoh are only going to go up, so I think we should find a way to only compile it once.

I have just a laptop with an i7-12800H with 32GB of ram. I get the extra build time, but it'll likely come with the cost of extra code maintenance.

I set the --target-dir option to the build directory of zenoh-c with the hope that cargo re-use some of the intermediate artifacts built with zenoh-c. However I'm not sure it's working considering the amount of dependencies it seems to re-build for zenohd...

This was also a thing I couldn't fix. I wasn't able to compile zenoh-c and then have zenohd reuse the build artifacts. The end result is compiling parts twice.

I'll keep poking away at it.

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

No branches or pull requests

5 participants