-
Notifications
You must be signed in to change notification settings - Fork 43
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 a zenoh session to run the router #101
Use a zenoh session to run the router #101
Conversation
This eliminates the need for zenohd_vendor, which we also remove here. Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
This should work for both Linux and Windows, though only tested on Linux so far. Signed-off-by: Chris Lalancette <clalancette@gmail.com>
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.
The code here works great. Did we come to a conclusion regarding tradeoffs between rust vs C based impl as discussed here?
If we want to stick to the C approach, I think it would be nice to refactor detail/zenoh_config.hpp
to be flexible enough to return configs for sessions or routers. Perhaps even look for an envar for the router config as well.
As of right now, I don't think we need any router features that we can't get via the configuration file. Given how far along we are in the implementation, it seems to me that we probably won't need it; the one possible exception is security, which we haven't thought about at all. The good news is that if we merge this, then we have both options in the history; we have the ability to run the Rust-based one (the commits prior to this), and we'll have this one. So worst case, we could always revert this and go back to the Rust-based router.
Yeah, that would work. That way the user could also configure the router, which is one of our long-term goals anyway. I'll take a look at doing that. |
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.
Thanks for clarifying. Happy to merge first and work on reusing the config generator code in a separate PR. Maybe we can open a ticket so we don't forget.
Yep, good call. I've opened up #102 to track that. |
The main benefit here is that we stop vendoring zenohd, which should improve our compile times. While in here, we also rewrite the keyboard handling so it will work on both Linux and Windows (though only tested on Linux so far).
This is mostly the work of @francocipollone ; I just did the keyboard handling part. This should fix #64